Implement WebDriver executor for wptrunner #12380
Conversation
wpt-pr-bot
added
infra
webdriver
wg-testing_browser
wpt
wptrunner
|
OK, so the status of running /infrastructure against Chrome is:
cc/ @foolip @JohnChen0 |
| } | ||
| } | ||
| }, | ||
| "w3c": True |
There was a problem hiding this comment.
Filed https://bugs.chromium.org/p/chromedriver/issues/detail?id=2536 about this today.
wpt-pr-bot
requested review from
andreastt,
AutomatedTester,
mjzffr,
shs96c and
whimboo
|
Safari Technology Preview seems to work fine with the new executor, based on /infrastructure. |
gsnedders
force-pushed
the
exec-webdriver
branch
from
3fd14b4
to
d403a86
Compare
|
Perhaps a good way to validate is to somehow trigger a full Taskcluster Chrome run with this enabled, and comparing the results? |
|
So at least for Safari we need the old Selenium codepath for stable Safari. Maybe Edge as well? |
|
Hmm, is this because SafariDriver and EdgeDriver only recently began supporting the WebDriver protocol? |
|
For Safari, yes. For Edge, I haven't tried yet. |
There was a problem hiding this comment.
Reviewed 9 of 11 files at r2.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @gsnedders, @AutomatedTester, @whimboo, @shs96c, @andreastt, and @mjzffr)
tools/wptrunner/wptrunner/browsers/chrome_webdriver.py, line 9 at r2 (raw file):
WebDriverRefTestExecutor) # noqa: F401 __wptrunner__ = deepcopy(chrome.__wptrunner__)
Can you create an inherit function in base.py that encapsulates this logic?
tools/wptrunner/wptrunner/executors/executorwebdriver.py, line 332 at r2 (raw file):
debug_info=debug_info) self.protocol = WebDriverProtocol(self, browser, capabilities=capabilities)
Indentation is wrong
webdriver/tests/support/fixtures.py, line 210 at r2 (raw file):
caps = copy.deepcopy(configuration["capabilities"]) if "alwaysMatch" in caps or "firstMatch" in caps:
This change seems unrelated?
There was a problem hiding this comment.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @gsnedders, @AutomatedTester, @whimboo, @shs96c, @andreastt, and @mjzffr)
tools/wptrunner/wptrunner/browsers/chrome_webdriver.py, line 9 at r2 (raw file):
Previously, jgraham wrote…
Can you create an
inheritfunction inbase.pythat encapsulates this logic?
FWIW, I was hoping we could get rid of most of this before landing by dropping the Selenium code paths.
webdriver/tests/support/fixtures.py, line 210 at r2 (raw file):
Previously, jgraham wrote…
This change seems unrelated?
We need to pass W3C-style capabilities to ChromeDriver, so executor_kwargs needs to return them.
Without this, we need to return Selenium style for wdspec (because otherwise we'll end up with caps['alwaysMatch']['alwaysMatch'] and that doesn't work at all, because it fails to match alwaysMatch) and W3C-style for all other test types.
|
Should make sure this avoids copying over #12578. |
|
FWIW, I'd much prefer having #12697 fixed so we can see /infrastructure working the same with Selenium and WebDriver. |
gsnedders
force-pushed
the
exec-webdriver
branch
from
d403a86
to
81995b8
Compare
This is awkward, because we currently don't have any way of getting the Safari version ahead-of-time (which we need because we need to change what browser module we load, because loading of executors is tightly coupled to the browser module). Even adding something to the Safari browser module (like with #12719) doesn't really help. I wonder if we should uncouple them? Probably. |
|
It's likely that the upcoming Safari stable release will have a new enough SafariDriver that we'll be able to use ExecutorWebDriver for both Safari stable and TP. I'd suggest leaving Safari on ExecutorSelenium until then. |
|
Edge stable doesn't work (and like Safari, it nowhere near works). Not actually tested RS5_RELEASE (due to ship in October), but RS_PRERELEASE definitely works. Guess we stick with Selenium for Edge and Safari, so only Chromium actually moves over by default. :( |
That is indeed unfortunate, but hopefully we'll be able to move them over and delete the Selenium executor before the end of the year? |
|
So what's the testing plan here? Get someone to run all of WPT locally with/without? Probably with both the small changes to the Selenium executor and the WebDriver one v. the merge base… We still don't have any better way to run everything, do we? :( |
|
From IRC last Friday: |
Submitting the task to Taskcluster failed. Detailscan not read a block mapping entry; a multiline key may not be an implicit key at line 8, column 9: |
|
@jgraham You said "And you can easily test it on taskcluster if you enable the TC app on a fork and then push the change to enable it there"; I can't figure out what that change to enable it is. Please send help. |
|
On TC, the differences between 067aaee and 0f43915 (i.e., moving to |
|
I'm not very worried by those changes. I would be easilly convinced that all those tests are already flaky. |
|
Going through these, the only ones that are disabled in any browser are: |
|
Bear in mind that tests that are unstable on Blink infra are not using Selenium or indeed Chrome, just running in contentshell. So there are likely to be some differences. We also don't know what the base rate of instability is running with Selenium, so it's impossible to say if this number of unstable tests is within the expectations we currently have. On your branch, sunning the above tests locally with 1:50.26 WARNING u'runner_teardown': () 1:50.45 INFO ## All results ## Which looks like a lot of these tests are just unstable. |
|
@jgraham interesting, that's much more than came back as unstable with |
|
@jgraham https://gist.github.com/084ea9f86491b84e22ce9c887694fbb7 is the result of rerunning it with an empty commit… no idea why so much changed (and so much is missing!) |
|
Ah, here's what happened: |
gsnedders
force-pushed
the
exec-webdriver
branch
from
726c83e
to
f41932b
Compare
|
Although this passed CI, TC has lots of: Filed #12876 for that. Annoying that whatever caused this is isn't caught by our current CI. |
|
@jgraham so what do you want to do here? do you want to just land this? do you want to land this with it enabled by default for Chrome (given you believe all the above to be flakiness)? |
I think a reasonable approach would be:
|
|
I think that what @foolip proposes is reasonable. I think it's hard to understand how any of the observed changes would be caused by changing the python library sending WebDriver commands; it would be different if we were seeing lots of failures in testdriver or similar. Once we enable this by default please remove the old implementation. |
|
@jgraham of the products/browsers, I presume? Selenium will still be used by Sauce. |
|
FWIW, I got the following as unstable: |
This adds edge_webdriver, chrome_webdriver, and safari_webdriver as products. Co-authored-by: Geoffrey Sneddon <me@gsnedders.com>
gsnedders
force-pushed
the
exec-webdriver
branch
from
beedba4
to
d082a8f
Compare
Yes, I mean don't maintain two implementations for specific browsers. |
Taken over #10197. Fixes #12374.
This change is