Skip to content
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

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

rnorth
Copy link
Member

@rnorth rnorth commented Oct 29, 2021

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:

  • 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. 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

    • 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.

  • 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
  • Steps to follow up on in future PR

    • 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

…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
@kiview
Copy link
Member

kiview commented Dec 20, 2021

Will fix #4593.

kiview and others added 8 commits December 20, 2021 13:19
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>
@kiview kiview marked this pull request as ready for review December 20, 2021 13:54
@kiview kiview changed the title WIP Proposed changes to Selenium/WebDriver module for ongoing compatibility Changes to Selenium/WebDriver module for ongoing compatibility Dec 20, 2021
@kiview kiview added modules/selenium type/deprecation Public APIs are being deprecated but not changed yet type/housekeeping labels Dec 20, 2021
orange-buffalo
orange-buffalo previously approved these changes Jan 3, 2022
Copy link
Contributor

@orange-buffalo orange-buffalo left a 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.

@rnorth
Copy link
Member Author

rnorth commented Jan 4, 2022

Thank you @orange-buffalo - we really appreciate you taking the time to try this out and give us feedback.

@rnorth rnorth added this to the next milestone Jan 5, 2022
/**
* @deprecated please use {@link BrowserWebDriverContainer#BrowserWebDriverContainer(DockerImageName)}
*/
@Deprecated
Copy link
Member

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.

Copy link
Member

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.

GannaChernyshova added a commit that referenced this pull request Jan 6, 2022
JKatzwinkel added a commit to JKatzwinkel/tla-web that referenced this pull request Jan 6, 2022
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;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Surely .withTag(...) is missing?

@kiview
Copy link
Member

kiview commented Jan 13, 2022

With #4914 being merged, this PR acts mostly as a reference for future refactorings and planned deprecations.

@bsideup bsideup modified the milestones: 1.16.3, next Jan 18, 2022
@kiview kiview removed this from the next milestone Apr 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
modules/selenium type/deprecation Public APIs are being deprecated but not changed yet type/housekeeping
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants