Update BrowserWebDriverContainer to add Selenium 4 support, drop Selenium 2 support #4914
Conversation
GannaChernyshova
requested review from
bsideup,
kiview and
rnorth
as code owners
| @@ -299,6 +278,16 @@ protected void containerIsStarted(InspectContainerResponse containerInfo) { | |||
| * @return a new Remote Web Driver instance | |||
| */ | |||
| public RemoteWebDriver getWebDriver() { | |||
There was a problem hiding this comment.
| public RemoteWebDriver getWebDriver() { | |
| public synchronized RemoteWebDriver getWebDriver() { |
There was a problem hiding this comment.
agree, updated
| driver = Unreliables.retryUntilSuccess(30, TimeUnit.SECONDS, | ||
| () -> Timeouts.getWithTimeout(10, TimeUnit.SECONDS, | ||
| () -> new RemoteWebDriver(getSeleniumAddress(), capabilities))); |
There was a problem hiding this comment.
nit: while we are on it, for the sake of readability, could you please use long lambdas here? :)
Something like:
driver = Unreliables.retryUntilSuccess(30, TimeUnit.SECONDS, () -> {
return Timeouts.getWithTimeout(10, TimeUnit.SECONDS, () -> {
return new RemoteWebDriver(getSeleniumAddress(), capabilities);
});
});
|
Maybe mark as draft, as I know you're still working on it @GannaChernyshova, @kiview? |
|
On merge, we need to add @tobiasstadler as co-author, since it was mostly based on his PRs. |
|
We removed the |
There was a problem hiding this comment.
I think that this is now incompatible with Selenium 2, right? I think as a result we need a docs update (like here)
As I understand it, we have manually tested with both Selenium 3 and 4 present on the classpath, but I guess not 2 (and IMHO we should be dropping support for 2 now).
| String browserName = capabilities == null ? BrowserType.CHROME : capabilities.getBrowserName(); | ||
|
|
||
| boolean seleniumGreaterOrEqualTo4 = new ComparableVersion(seleniumVersion).isGreaterThanOrEqualTo("4"); | ||
| switch (browserName) { | ||
| case BrowserType.CHROME: | ||
| return CHROME_IMAGE.withTag(seleniumVersion); | ||
| return (seleniumGreaterOrEqualTo4 ? CHROME_IMAGE : CHROME_DEBUG_IMAGE).withTag(seleniumVersion); | ||
| case BrowserType.FIREFOX: | ||
| return FIREFOX_IMAGE.withTag(seleniumVersion); | ||
| return (seleniumGreaterOrEqualTo4 ? FIREFOX_IMAGE : FIREFOX_DEBUG_IMAGE).withTag(seleniumVersion); | ||
| default: |
There was a problem hiding this comment.
I think the logic that we've ended up with is practically the same, but perhaps the if statements and variable names were clearer in #4609? https://github.com/testcontainers/testcontainers-java/pull/4609/files#diff-317a1df21c0090ecdecb77b75ff9e55deb8b4c56f4e6a8f084ea096e4e2e25f2R248
It's not a big deal though.
There was a problem hiding this comment.
Removed Selenium 2 support from documentation and renamed variables as in #4609. It will be good to update documentation even more (add more examples and describe some use cases) but assume it's better to do in the separate PR :)
rnorth
changed the title
|
Thank You! |
|
Thanks to you @tobiasstadler and sorry for taking some time to sort this one out. |
Based on #4670, #4609 and #4686