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

Implement test executor for Chrome using CDP (ExecutorCDP) #9647

Closed
gsnedders opened this issue Feb 23, 2018 · 22 comments
Closed

Implement test executor for Chrome using CDP (ExecutorCDP) #9647

gsnedders opened this issue Feb 23, 2018 · 22 comments
Assignees
Labels
infra priority:roadmap wptrunner The automated test runner, commonly called through ./wpt run

Comments

@gsnedders
Copy link
Member

cc/ @jgraham @foolip @kereliuk

ChromeDriver is maintained out of the main Chromium tree, and doesn't always work with ToT (or even Canary, and occasionally Beta).

If we want to make it possible to always run against latest Chromium, we could use the Chrome DevTools Protocol, whose stable versions should work in ToT. 1.3 (from Chromium 64) should suffice for running all but wdspec tests; 1.2 (from Chromium 54) should suffice for running testharness.js tests (but not, I think reftests).

This should, hopefully, also decrease the amount of complexity and ability for raciness that we currently have. In theory, I think, it would also then be possible for run-webkit-tests to use wptrunner.

@gsnedders gsnedders added infra wptrunner The automated test runner, commonly called through ./wpt run labels Feb 23, 2018
@jgraham
Copy link
Contributor

jgraham commented Feb 23, 2018

So one question I have is why the stability properties of wptrunner built on CDP are different to the stability properties of Chromdriver built on CDP.

That said, I think this may be an interesting idea that's worth pursuing.

@gsnedders
Copy link
Member Author

So one question I have is why the stability properties of wptrunner built on CDP are different to the stability properties of Chromdriver built on CDP.

In short, ChromeDriver uses much more than the guaranteed stable subset of CDP, and that can and does change.

@gsnedders
Copy link
Member Author

Also, to note, we'd still need the WebDriver (or, as is, Selenium) backend for testharness.js and reftests for Sauce, Edge, and in the future Safari.

@jgraham
Copy link
Contributor

jgraham commented Feb 23, 2018

I guess the more detailed question was "is the stable subset of CDP enough to implement all the testdriver features, or only enough for non-testdriver uses?"

@kereliuk
Copy link
Contributor

In short, ChromeDriver uses much more than the guaranteed stable subset of CDP, and that can and does change.

There was discussion about this before but no action as far as I know. But I am optimistic that at least some of they key things we would need for testdriver are basically stable, or will be maintained if they become unstable.

I'm finishing up the anti-selenium webdriver executor stuff right now, but in terms of difficulty I'm guessing it shouldn't be too much different than that.

@foolip
Copy link
Member

foolip commented Feb 25, 2018

@gsnedders, can you file specific issues for "doesn't always work with ToT (or even Canary, and occasionally Beta)" and "raciness that we currently have".

The fix isn't necessarily to abandon ChromeDriver, but could just as well be to fix these problem with ChromeDriver.

@kereliuk, unless you have concrete plans to add an ExecutorCDP or similar, or think we definitely should, I think we should close this issue, or it'll just sit around as a "maybe" indefinitely.

@gsnedders
Copy link
Member Author

@foolip #6986 is the most prominent case I can think of; the raciness I think I was thinking of the issues we have in run-webkit-tests.

@foolip
Copy link
Member

foolip commented Feb 26, 2018

Is the idea that we could potentially control content_shell using a CDP runner? That sounds like it might be possible, but if CDP works then ChromeDriver itself might also work. I'll leave it to @kereliuk to react to that :)

@gsnedders
Copy link
Member Author

@foolip Yeah, that was the other bit of thinking about whether it'd be possible to use wptrunner from run-webkit-tests.

@Hexcles
Copy link
Member

Hexcles commented Feb 28, 2018

What are the benefits of using wptrunner in run-webkit-tests?

For maintainability, there aren't many special cases to handle WPT in run-webkit-tests. Blink needs a finder (to find tests to run given path patterns) and a runner for layout tests anyway. And if we switch to wptrunner, we may remove some special cases but still need some other glue code (e.g. to process the output).

As for raciness, are you thinking about the webfont screenshot issue? Yes, by using wptrunner & CDP, we'd end up using the same hack (two rAFs after font.ready) in content_shell, which would likely reduce the flakiness a lot, but that sounds a rather convoluted workaround...

@gsnedders
Copy link
Member Author

What are the benefits of using wptrunner in run-webkit-tests?

I was mostly thinking avoiding ending up with subtly different test execution semantics.

@foolip foolip changed the title Implement test executor for Chrome using CDP Implement test executor for Chrome using CDP (ExecutorCDP) Oct 26, 2018
@foolip
Copy link
Member

foolip commented Oct 26, 2018

Prototype of ExecutorCDP is on our planning for this quarter, experiment to be done by @jugglinmike.

@gsnedders
Copy link
Member Author

@jugglinmike did the prototype; we should link it from here.

@Hexcles
Copy link
Member

Hexcles commented Jan 10, 2019

https://github.com/bocoup/wpt/tree/wptrunner-cdp/tools/pyppeteer

@jugglinmike
Copy link
Contributor

At the end of 2018, we created a proof-of-concept WPT "executor" which uses the Chrome DevTools protocol to automate the browser under test. You can read about the process in this presentation from 2018-12-11.

We'd like to bring this prototype to a production-ready state and maintain it in the master branch of WPT. Here's what I believe still needs to be done:

  • Refactor as a standalone executor (instead of a re-write of ExecutorWebDriver)
  • Extend CLI to allow users to enable this
  • Resolve discrepancies in test results as listed in the aforementioned presentation
  • Implement remaining testdriver.js capabilities (pending the availability of requisite functionality in the Chrome DevTools Protocol)
  • Improve project documentation
  • Implement nominal tests

I'm not sure what meaningful tests will look like yet since the code is necessarily coupled to a very large and complex system.

Some nice-to-have improvements:

  • Use a more efficient transport mechanism (see slides for details)
  • Support Python 3 (and run tests in this environment)

@foolip
Copy link
Member

foolip commented Jun 10, 2019

@jugglinmike just for the public record, this is still on track to be merged into this repo, right?

@jugglinmike
Copy link
Contributor

This issue could definitely use an update; Thanks for the reminder, @foolip

The latest on the technical implementation is discussed in this Google Document:

https://docs.google.com/document/d/1X3EiMRQ60eOtEDCXAUhvY-c5sVKY5ntc9xxqjem-enI/edit

The completed work is available at:

https://github.com/web-platform-tests/wpt/compare/bocoup/wptrunner-cdp

This patch must be approved via the WPT RFC process. The relevant RFC is here, but it is not resolved.

@foolip
Copy link
Member

foolip commented Sep 25, 2019

The resolution of web-platform-tests/rfcs#5 was to go ahead, but the importance of completing this sort of evaporated, we didn't plan to do anything about it in Q3 and also didn't.

@jugglinmike what is the state of this now, is there work in your branch that we should make sure to get landed?

@jugglinmike
Copy link
Contributor

what is the state of this now

The patch does not merge cleanly, but I attempted to rebase just now, and the resolution is surprisingly straightforward. Of the four conflicts, one is related to a small change to the testing configuration, and three are related to the introduction of "edgechromium".

Merge conflicts aside, it will take more time to verify that it's still fully functional. Python interfaces may have changed, and there's always the risk of incompatibilities in the latest version of the Chrome DevTools protocol.

is there work in your branch that we should make sure to get landed?

I don't think there's any discrete piece that would be meaningful to merge on its own. If we merge the entire patch, we should make sure we're able to support it. To me, that would entail enabling ExecutorCDP for some CI runs and also committing to any maintenance necessary to support future modifications to the infrastructure.

@foolip
Copy link
Member

foolip commented Sep 25, 2019

@LukeZielinski what do you think, will we want to use ExecutorCDP when using wptrunner in Chromium?

@LukeZielinski
Copy link
Contributor

As of today, I don't think we'd use ExecutorCDP in wptrunner inside Chromium. One goal of integrating WPT into Chromium CI is to maintain consistency between tests run in Chromium and "upstream", so seems like using different executors wouldn't be a good idea.

@foolip
Copy link
Member

foolip commented Sep 26, 2019

Yep, and much to my surprise the test automation use cases that might require ExecutorCDP (plus some spec side abstraction matching it) haven't materialized.

At think point I think the best thing will be to close this and not land the code. This bugs me because it means I misprioritized and we did this work without a very clear use case, but doing more work on it right now would be to continue misprioritizing.

Outright standardization of CDP isn't looking very likely in the near term. Advances in WebDriver might overtake the need we saw for this, but even then we should have some clear need for bidirectional communication before revisiting this.

@foolip foolip closed this as completed Sep 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
infra priority:roadmap wptrunner The automated test runner, commonly called through ./wpt run
Projects
None yet
Development

No branches or pull requests

7 participants