-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make recorder .flv videos scrollable (#512) #3180
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot, I have added some discussion point.
Looking forward to getting this quality of life fix merged 🙂
core/src/main/java/org/testcontainers/containers/VncRecordingContainer.java
Outdated
Show resolved
Hide resolved
...s/selenium/src/test/java/org/testcontainers/junit/ChromeRecordingWebDriverContainerTest.java
Outdated
Show resolved
Hide resolved
...s/selenium/src/test/java/org/testcontainers/junit/ChromeRecordingWebDriverContainerTest.java
Outdated
Show resolved
Hide resolved
...s/selenium/src/test/java/org/testcontainers/junit/ChromeRecordingWebDriverContainerTest.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/testcontainers/containers/VncRecordingContainer.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/testcontainers/containers/VncRecordingContainer.java
Outdated
Show resolved
Hide resolved
...s/selenium/src/test/java/org/testcontainers/junit/ChromeRecordingWebDriverContainerTest.java
Outdated
Show resolved
Hide resolved
...s/selenium/src/test/java/org/testcontainers/junit/ChromeRecordingWebDriverContainerTest.java
Outdated
Show resolved
Hide resolved
Some tests don't pass : due to ExpectedConditions.visibilityOfAllElementsLocatedBy(By.cssSelector("#search h3") that is not met, but when using just I will try find out why and commit a fix |
The tests were fixed by changing the Css Selector to look for parent element
|
4b75c5e
to
f76e59a
Compare
|
...s/selenium/src/test/java/org/testcontainers/junit/ChromeRecordingWebDriverContainerTest.java
Outdated
Show resolved
Hide resolved
...s/selenium/src/test/java/org/testcontainers/junit/ChromeRecordingWebDriverContainerTest.java
Outdated
Show resolved
Hide resolved
I've merged from master, as we've had a problem with GitHub Actions that needed to be fixed in the master branch. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good, just had some small remarks.
@rnorth
Can you have a final look?
core/src/main/java/org/testcontainers/utility/TestcontainersConfiguration.java
Outdated
Show resolved
Hide resolved
...s/selenium/src/test/java/org/testcontainers/junit/ChromeRecordingWebDriverContainerTest.java
Outdated
Show resolved
Hide resolved
...s/selenium/src/test/java/org/testcontainers/junit/ChromeRecordingWebDriverContainerTest.java
Outdated
Show resolved
Hide resolved
...s/selenium/src/test/java/org/testcontainers/junit/ChromeRecordingWebDriverContainerTest.java
Outdated
Show resolved
Hide resolved
...s/selenium/src/test/java/org/testcontainers/junit/ChromeRecordingWebDriverContainerTest.java
Outdated
Show resolved
Hide resolved
...s/selenium/src/test/java/org/testcontainers/junit/ChromeRecordingWebDriverContainerTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rnorth Can you have a final look?
core/src/main/java/org/testcontainers/containers/VncRecordingContainer.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We had a discussion offline, and decided that we can't risk changing the default emitted file format without warning.
I'll take an action to amend this PR, adding a parameter for format which will default to FLV. In 1.16.0 we'll change the default to MP4.
Sorry that this came as a late decision.
@rnorth sounds good.
Just note that it does not suffice to change the extension |
…RecordingWebDriverContainerTest.java
Thanks everyone for letting me contribute to this awesome project, hope it will be useful to the community 😃 |
@oussamabadr thank you so much for contributing it! 💯 Expect it to be released very soon :) |
1.16.0 got released. However I was unable to spot the change in the release notes. |
@AB-xdev The default is FLV, but you can specify MP4 as well. |
Thank you for the ultra-fast feedback 👍 Backstory: |
@AB-xdev we forgot to switch the default in 1.16.0, sorry 😅 |
No problem. Just a question: Do you still want to switch or not? |
@AB-xdev we do! It is the right default value and will make many users' lives easier, just we need to do it in a "major" release, to not confuse our users. So, 1.17.0 then 😅 |
I have written an implementation to fix the scrollable video using ffmpeg command, which will also reduce the video size as mentioned by @leonard84
I have also opened a related pull Request testcontainers/vnc-recorder#4 by adding ffmpeg to the docker image, which (I saw) necessary to avoid using another container and also simplify the fix.
Just to mention that some test won't pass untill the new vnc-recorder docker image is available with tag 1.2.0.
(Thank to @kiview for his assitance during hack-commit-push (last june) which make contributing to testcontainers more clear for me.)
Fixes #512.