Consolidate testcontainer used by WebDriver test #112
Conversation
Previously, we hard coded the version of the docker image for provisioning Chrome browser and WebDriver server because the version of the Chrome browser has to match the version of the webdriver client, otherwise the screenshot test will fail. Changing to use BrowserWebDriverContainer can delegate the match to the library itself because it chooses the correct docker image version based on the WebDriver version on our classpath.
hstonec
force-pushed
the
sc/consolidate-testcontainers
branch
from
42fada6
to
d64ecf4
Compare
|
Please don't push to the |
There was a problem hiding this comment.
Reviewed 3 of 3 files at r1.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @jianglai)
There was a problem hiding this comment.
Reviewed 3 of 3 files at r1.
Reviewable status:
There was a problem hiding this comment.
Reviewed 3 of 3 files at r1.
Reviewable status:
There was a problem hiding this comment.
Reviewed 1 of 3 files at r1.
Reviewable status: 1 of 3 files reviewed, 2 unresolved discussions (waiting on @hstonec and @jianglai)
core/src/test/java/google/registry/webdriver/README.md, line 26 at r1 (raw file):
-webkit-transition
Why removing this flag?
core/src/test/java/google/registry/webdriver/WebDriverPlusScreenDifferRule.java, line 61 at r1 (raw file):
20
Is this change to suppress test flakiness? If so you can mention the change in the PR description as well?
There was a problem hiding this comment.
Reviewed 2 of 3 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @hstonec)
This is to supress the test flakness after switching to use BrowserWebDriverContainer to provision browser and webdriver service.
hstonec
force-pushed
the
sc/consolidate-testcontainers
branch
from
d64ecf4
to
262eccd
Compare
There was a problem hiding this comment.
Reviewable status: 2 of 3 files reviewed, 2 unresolved discussions (waiting on @jianglai)
core/src/test/java/google/registry/webdriver/README.md, line 26 at r1 (raw file):
Previously, jianglai (Lai Jiang) wrote…
-webkit-transitionWhy removing this flag?
This is just some weird metadata I don't know why and when it got generated. It is useless and breaks the parsing of this paragraph. See here https://github.com/google/nomulus/tree/master/core/src/test/java/google/registry/webdriver
core/src/test/java/google/registry/webdriver/WebDriverPlusScreenDifferRule.java, line 61 at r1 (raw file):
Previously, jianglai (Lai Jiang) wrote…
20Is this change to suppress test flakiness? If so you can mention the change in the PR description as well?
Done.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r3.
Reviewable status:
core/src/test/java/google/registry/webdriver/README.md, line 26 at r1 (raw file):
Previously, hstonec (Shicong Huang) wrote…
This is just some weird metadata I don't know why and when it got generated. It is useless and breaks the parsing of this paragraph. See here https://github.com/google/nomulus/tree/master/core/src/test/java/google/registry/webdriver
LoL of course it breaks parsing, it is outside of the triple backtick quote :)
core/src/test/java/google/registry/webdriver/WebDriverPlusScreenDifferRule.java, line 61 at r1 (raw file):
Previously, hstonec (Shicong Huang) wrote…
Done.
Remember to include the added comment when you squash and merge.
Previously, we hard coded the version of the docker image for
provisioning Chrome browser and WebDriver server because the
version of the Chrome browser has to match the version of
the webdriver client, otherwise the screenshot test will fail.
Changing to use BrowserWebDriverContainer can delegate the match
to the library itself because it chooses the correct docker image
version based on the WebDriver version on our classpath.
This change is