-
-
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
Changes to Selenium/WebDriver module for ongoing compatibility #4609
base: main
Are you sure you want to change the base?
Conversation
…bility Co-authored with @kiview, this PR started out with some minor (hacky) workarounds but evolved into a rethink of what features are sensible for us to keep on trying to make work given: * existence of Selenium 4.0.0, with some breaking API changes * some mutable tags being used for Selenium Docker images, and the (IMHO correct) recommendations by the Selenium team that more exactly pinnable tags should be used in our use case. Very WIP - all of this could change. Primary changes: - Selenium 4.0.0 compatibility fixes - Remove in-code workaround for Chrome GPU issue, which created coupling to a Selenium 3.x API - Remove support for Selenium 2.x, as supporting 3 major versions of the library becomes infeasible - Use non-debug docker images automatically for Selenium 4.x, as `-debug` images are not required (and apparently not available for Firefox) - Remove `provided` scope dependencies, since we already require some Selenium JARs to be on the classpath. Gradle `implementation` scope is used despite Selenium classes being in our API - this is a slight and deliberate abuse, but seems better than us potentially forcing an upgrade of Selenium by updating our dependency version. - TODO: update docs - TODO: further testing of the assumption that this will not have real impacts to users. - Steps to phase out implicit docker image detection and to remove API methods that are coupled to Selenium classes - Deprecate constructors that do not supply a user-pinnable Docker image - Deprecate constructors/methods that involve Capabilities and RemoteWebDriver - Suggest alternative approaches for obtaining a WebDriver instance, predicated on future removal of APIs that are coupled to Selenium classes - Have *not* deprecated the `determineClasspathSeleniumVersion` convenience method that can be used to identify Selenium version on the classpath, as this may be something that users want to use - TODO: plenty of docs, examples and migration notes - TODO: specific documentation around best-practices for selecting a pinnable Selenium image - TODO: phase out implicit use of Chrome
Will fix #4593. |
Co-authored-by: Anna Chernyshova <anna@atomicjar.com>
Co-authored-by: Anna Chernyshova <anna@atomicjar.com>
Co-authored-by: Anna Chernyshova <anna@atomicjar.com>
Co-authored-by: Anna Chernyshova <anna@atomicjar.com>
Co-authored-by: Anna Chernyshova <anna@atomicjar.com>
Co-authored-by: Anna Chernyshova <anna@atomicjar.com>
modules/selenium/src/main/java/org/testcontainers/containers/BrowserWebDriverContainer.java
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.
As for the library user, merge request looks good to me. I've tested this branch snapshot on my project. Backwards compatibility is preserved - just upgrading the version works smoothly with Selenium 4.x. After resolving the deprecation warnings everything continues to work.
Thank you @orange-buffalo - we really appreciate you taking the time to try this out and give us feedback. |
modules/selenium/src/main/java/org/testcontainers/containers/BrowserWebDriverContainer.java
Outdated
Show resolved
Hide resolved
/** | ||
* @deprecated please use {@link BrowserWebDriverContainer#BrowserWebDriverContainer(DockerImageName)} | ||
*/ | ||
@Deprecated |
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.
note that, if we deprecate this, there are no constructors that would not set recordingMode = SKIP
by default.
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.
Isn't this even in line with our current docs?
https://www.testcontainers.org/modules/webdriver_containers/#recording-videos
By default, no videos will be recorded. However, you can instruct Testcontainers to capture videos for all tests or just for failing tests.
the method signature of `ChromeOptions#addArguments` has changed: testcontainers/testcontainers-java#4593 (comment) until testcontainers/testcontainers-java#4609 is merged and released, testcontainers artifacts should be installed from jitpack instead of maven so that changes in this open pull request are included.
|
||
if (browserName.equals("chrome")) { | ||
if (supportsVncWithoutDebugImage) { | ||
return CHROME_IMAGE; |
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.
Surely .withTag(...)
is missing?
With #4914 being merged, this PR acts mostly as a reference for future refactorings and planned deprecations. |
Co-authored with @kiview and @GannaChernyshova, this PR started out with some minor (hacky) workarounds but evolved into a rethink of what features are sensible for us to keep on trying to make work given:
Very WIP - all of this could change. It may be that we're being too opinionated in deprecation of some of our more opinionated features..!
Primary changes:
Selenium 4.0.0 compatibility fixes
-debug
images are not required (and apparently not available for Firefox)Remove
provided
scope dependencies, since we already require some Selenium JARs to be on the classpath. Gradleimplementation
scope is used despite Selenium classes being in our API - this is a slight and deliberate abuse, but seems better than us potentially forcing an upgrade of Selenium by updating our dependency version.Steps to phase out implicit docker image detection and to remove API methods that are coupled to Selenium classes
determineClasspathSeleniumVersion
convenience method that can be used to identify Selenium version on the classpath, as this may be something that users want to useSteps to follow up on in future PR