wpt icon indicating copy to clipboard operation
wpt copied to clipboard

Download Chrome + ChromeDriver via CfT

Open mathiasbynens opened this issue 1 year ago • 18 comments

tools/wpt/browser.py has to be updated to support upcoming changes to the ChromeDriver release process.

The good news is this might actually simplify the current setup, since it guarantees compatible same-versioned Chrome for Testing + ChromeDriver binaries for each user-facing Chrome version that gets released.

In addition to the ChromeDriver-specific changes, it might make sense to revamp the ChromeChromiumBase logic as well to use Chrome for Testing binaries instead of the randomly-versioned Chromium binaries.

cc @foolip @DanielRyanSmith

mathiasbynens avatar May 22 '23 08:05 mathiasbynens

@mathiasbynens is there a replacement for finding a ChromeDriver binary when the user provides a Chrome binary already? Or perhaps we shouldn't support that, and require both binaries to be provided in this case?

foolip avatar May 22 '23 09:05 foolip

This scenario is currently handled here:

https://github.com/web-platform-tests/wpt/blob/a550c4213bd3317f1454989eef6fabb25ec5feff/tools/wpt/browser.py#L945-L970

foolip avatar May 22 '23 09:05 foolip

In the scenario where you already have a non-CfT Chrome binary and want to find the corresponding CfT ChromeDriver binary, you could use the JSON endpoint that lists all “known good versions”: https://github.com/GoogleChromeLabs/chrome-for-testing#json-api-endpoints With what’s currently available, you’d then have to implement the logic to find the closest version yourself.

I see you want a solution for specifically turning e.g. 114.0.5735 into 114.0.5735.35 (whatever the latest available version is within that range). I would be happy to add a dedicated JSON endpoint for that purpose to more easily enable your use case. (The JSON could be generated based on the “known good versions” data.) Update: Done. Please see the latest-patch-versions-per-build endpoints.

Please confirm if this use case is still there. I ask because, if you could simply use CfT for both the browser + driver binaries, the problem of finding corresponding binaries goes away entirely.

mathiasbynens avatar May 22 '23 10:05 mathiasbynens

But before I go and do that, please confirm if this use case is still there. I ask because, if you could simply use CfT for both the browser + driver binaries, the entire problem of finding corresponding binaries goes away entirely.

@DanielRyanSmith can you look into this? Is this code path ever used in CI currently? Would it be hit when following the docs for running tests in Chrome locally?

foolip avatar May 22 '23 11:05 foolip

@DanielRyanSmith can you look into this? Is this code path ever used in CI currently? Would it be hit when following the docs for running tests in Chrome locally?

This path is both used when running the CI and locally, but when the implementation is changed here to use CfT, this shouldn't be necessary in this form. Other browsers have compatibility with downloading specific channels (e.g. stable, dev, canary, etc.), and we can adjust the implementation to do so as well, and use the CfT endpoints to ensure the correct versions for those channels. We can also make changes as needed to ensure that installing binaries locally in separate commands (Chrome, then Chromedriver) will always match if the first binary was downloaded via CfT, which is what we do when downloading Chromium binaries from specific revisions.

As for matching a user-provided Chrome binary to a location to download a matching ChromeDriver version, I'm not certain how often that is used, but it is technically supported currently, although it still follows the dubious process we use today for matching ChromeDriver versions. I would imagine it is fine to ignore this case, and we can keep the old fallbacks for ChromeDriver matching if we really want to maintain this if it's not easy to pair with CfT. Also, the functionality for a user providing both a Chrome and ChromeDriver would work just the same.

My conclusion after glancing is that we should now have sufficient ability to make the change to using CfT. 🙂 Hopefully I'm not missing anything.

DanielRyanSmith avatar May 23 '23 00:05 DanielRyanSmith

As for matching a user-provided Chrome binary to a location to download a matching ChromeDriver version, I'm not certain how often that is used, but it is technically supported currently, although it still follows the dubious process we use today for matching ChromeDriver versions. I would imagine it is fine to ignore this case, and we can keep the old fallbacks for ChromeDriver matching if we really want to maintain this if it's not easy to pair with CfT.

Update: I’ve implemented the latest-patch-versions-per-build endpoints and documented them here: https://github.com/GoogleChromeLabs/chrome-for-testing#json-api-endpoints If you want to keep supporting this use case, you can use either of them.

My conclusion after glancing is that we should now have sufficient ability to make the change to using CfT. 🙂 Hopefully I'm not missing anything.

Exciting! Let me know if you hit any issues — I’d be happy to help.

mathiasbynens avatar May 23 '23 07:05 mathiasbynens

As for matching a user-provided Chrome binary to a location to download a matching ChromeDriver version, I'm not certain how often that is used, but it is technically supported currently, although it still follows the dubious process we use today for matching ChromeDriver versions. I would imagine it is fine to ignore this case, and we can keep the old fallbacks for ChromeDriver matching if we really want to maintain this if it's not easy to pair with CfT. Also, the functionality for a user providing both a Chrome and ChromeDriver would work just the same.

I would guess that people often hit this case the first time they try ./wpt run, following the instructions at https://web-platform-tests.org/running-tests/chrome.html.

If we change the instructions to use CfT by default, then maybe not many people will run into this case. We could either make it totally unsupported by throwing an error, or use latest-patch-versions-per-build to find the chromedriver version to use. Perhaps this decision can be made in code review after trying the different options.

foolip avatar May 23 '23 13:05 foolip

Adding as a data point:

We just started to use CfT for wpt.fyi testing.

jcscottiii avatar Aug 02 '23 20:08 jcscottiii

Now that Chrome 115 is stable, this has become much more urgent, since ChromeDriver downloads using the old logic are now failing: https://github.com/web-platform-tests/wpt/issues/41359 This is blocking PRs such as https://github.com/web-platform-tests/wpt/pull/41381.

mathiasbynens avatar Aug 07 '23 10:08 mathiasbynens

Something is failing, but I think the PRs are sill being tested correctly? The fallback logic seems to end up downloading a 117 build if I'm understanding the logging in https://community-tc.services.mozilla.com/tasks/ADOTvkQiTaCb1JuMbzbg-Q/runs/0/logs/live/public/logs/live.log#L621-622 correctly.

Nevertheless I'd suggest that making the download logic match reality should be a high priority, because otherwise it seems plausible that we're going to have a disruptive situation where every run starts failing in Chrome.

jgraham avatar Aug 09 '23 11:08 jgraham

(Oh I see from the other issue we're already in that state for stable runs; it's just that they don't block PRs)

jgraham avatar Aug 09 '23 11:08 jgraham

Ah https://github.com/web-platform-tests/wpt/pull/41356 is actually a draft fix here.

jgraham avatar Aug 09 '23 11:08 jgraham

#41356 Is available for review for those who are interested in taking a look before merging. 🙂

DanielRyanSmith avatar Aug 15 '23 04:08 DanielRyanSmith

The switch to allowing WPT downloading Chrome and ChromeDriver via Chrome for Testing is complete! However, there are some separate parts of this work that need to be addressed.

  • A separate debian download of Chrome Dev is still used for automated WPT runs. This is because Chrome multi-platform browser binaries were not available for download through WPT until CfT.
  • Ideally, we switch to Chrome Canary for automated runs, for more up-to-date results on tests, interop, etc.

The special download process for automated runs needs to be changed to download Chrome for Testing, and specifically download canary rather than dev.

DanielRyanSmith avatar Aug 16 '23 20:08 DanielRyanSmith

Can this now be closed given that #41356 has been merged?

whimboo avatar Aug 24 '23 08:08 whimboo

@whimboo https://github.com/web-platform-tests/wpt/issues/40119#issuecomment-1681198147 still lists a few follow-ups.

mathiasbynens avatar Aug 24 '23 08:08 mathiasbynens

FWIW I think switching to CfT for CI runs would be a big win because it would allow us to remove some additional complexity in the taskcluster setup that's only there to support the fact that we need to install a specific deb package at runtime for Chrome.

jgraham avatar Oct 24 '23 10:10 jgraham

@DanielRyanSmith do you have an update on the remaining work? I'm a bit fuzzy on what was done and what remains here.

foolip avatar Feb 17 '24 08:02 foolip

@DanielRyanSmith do you have an update on the remaining work? I'm a bit fuzzy on what was done and what remains here.

I think it's safe to close this issue. We're collecting all test results displayed on wpt.fyi using Chrome for Testing now, as well obtaining CfT with all invocations of ./wpt install or adding the --install-browser flag. There is an issue in the WPT integration tests check suite setup that is making it difficult to switch to using CfT there, but it is a separate problem that does not pertain specifically to CfT, and warrants its own issue.

DanielRyanSmith avatar Feb 25 '24 20:02 DanielRyanSmith

@sadym-chromium pointed out that tools/ci/azure/install_chrome.yml is installing non-CfT Chrome Dev using Homebrew. Is this intentional, or should we move this over to CfT as well so it aligns with the other parts of the infrastructure?

mathiasbynens avatar Apr 30 '24 09:04 mathiasbynens

I tried a basic and naive change to install Chrome using ./wpt install here. Everything seems to be detected and run as expected, with the exception of the fact that some reftests are now timing out. After running the tests again, it seems that different reftests timed out, which might be indicating flakiness. Maybe someone with more experience with these might know why this could be happening? Also, maybe this is tangentially related to issue https://github.com/web-platform-tests/wpt/issues/44571

Test results: [1] https://dev.azure.com/web-platform-tests/wpt/_build/results?buildId=122529&view=logs&j=af75b4de-b26f-5d4d-58e8-b41258076d44&t=052839da-7227-5391-fe2d-4b66ec169a0c [2] https://dev.azure.com/web-platform-tests/wpt/_build/results?buildId=122533&view=logs&j=af75b4de-b26f-5d4d-58e8-b41258076d44&t=052839da-7227-5391-fe2d-4b66ec169a0c

DanielRyanSmith avatar Apr 30 '24 21:04 DanielRyanSmith