owned this note
owned this note
Published
Linked with GitHub
In reference to:
https://github.com/web-platform-tests/wpt-pr-bot/issues/100
Related to:
https://trac.webkit.org/wiki/ImprovingInteropwithWPT
Other related documents:
https://trac.webkit.org/wiki/WPTandWebKitTests
# Goal
Low-friction workflow to export/import tests from web-platform-tests in WebKit (`@foolip`).
The export workflow should address a solution for:
* Smooth uploading WebKit patch to WPT
* Uploading of a new version of the WebKit patch to WPT when it changes
* PR approval in the WPR repository for the WebKit patches reviewed (r+)
* Merging the export PR when the WebKit patch is landed (cq+)
> [name=Philip Jägenstedt]
> This sounds like the PR will be merged after the
> WebKit patch lands, but the current rule is before.
> [name=Pablo Saavedra]
> See my comment here: github.com/web-platform-tests/wpt-pr-bot/issues/100#issuecomment-550056150
>
> Right now there are PR already landed in WK but not > merged in WPT
> cq+ means in queue so more specifically both (the WK patch
> and the WPT PR) could be landed in parallel
The import workflow should address a solution for:
* Automation of import process
* Simplification of the WPT tests importation process
* Rebasing the current imported tests
# Export W3C tests process
## Current situation in the export process
The `webkit-patch upload` command is the main supported way to upload code reviews for WebKit. Also, the `Tools/Script/export-w3c-tests` script currently implements the patch export functionality for the PR against WPT. This patch is already integrated in `webkit-patch upload`. E.g:
```sh
$ Tools/Scripts/webkit-patch upload -g HEAD --suggest-reviewers
Total errors found: 0 in 2 files
Was that diff correct? [Y/n]: y
Logging in as clopez@igalia.com...
Fetching: https://bugs.webkit.org/show_bug.cgi?id=203996&ctype=xml&excludefield=attachmentdata
The following reviewers have recently modified files in your patch:
Chris Dumez, Youenn Fablet, Dean Jackson, Antti Koivisto, Antoine Quint
Would you like to CC them? [Y/n]: y
Adding [u'cdumez@apple.com', u'youennf@gmail.com', u'dino@apple.com', u'koivisto@iki.fi', u'graouts@apple.com'] to the CC list for bug 203996
Fetching: https://bugs.webkit.org/show_bug.cgi?id=203996&ctype=xml&excludefield=attachmentdata
Assigning bug 203996 to clopez@igalia.com
Adding patch "Patch" to https://bugs.webkit.org/show_bug.cgi?id=203996
Would you like to export the web-platform-tests changes to a WPT GitHub repository? [Y/n]:
```
These scripts need some more refinements though, especially if they run on a bot (`@youennf`):
* [167911](https://bugs.webkit.org/show_bug.cgi?id=167911) – Enable two-way sync of WPT tests
* [186132](https://bugs.webkit.org/show_bug.cgi?id=186132) – export-w3c-test-changes can not export reftests
Right now, no human or bot is specifically in charge of running this script so WPT is receiving PR from any contributor. The PR should be assigned the `webkit-export` label, but this can currently only happen if the submitter has write access to WPT.
Finally, the PR is not automatically merged into WPT when the patch is landed in WebKit. Also, because exports for people who aren't WebKit or WPT reviewers should work, some piece of approving and possibly merging export PRs needs to be performed by something other than that script.
## Proposed changes in the export workflow
The best case for export from WebKit to WPT is an integration using bot actions for each of these events:
1. Uploading or requesting review on a WebKit patch creates a draft export PR
> [name=Manuel Rego Casasnovas]
> I guess that when you upload a new version of the patch the
> same PR should be updated, instead of creating a new one
> for each patch upload.
>
> [name=Philip Jägenstedt]
> Yes, definitely.
1. Getting review (r+) on the WebKit patch approves the export PR
1. The export PR is merged when the WebKit patch is about to land
Additionally, because of the possibility of reverts that bypass the commit queue and we probably do not want to block WebKit revert patch landing on WPT though. This workflow assumes **Don’t revert WPT changes and have specific handling for the relanding patches**.
### 1) Uploading or requesting review on a WebKit patch creates a draft export PR
This will still be the responsibility of the `Tools/Script/export-w3c-tests` script and uploaded with the `webkit-patch upload` like the very common usual way to push this patches.
In addition, because we still want label the PR with webkit-export. A `wpt-pr-bot` or Github Action will check the occurrences of the WebKit export of and or the `https://bugs.webkit.org/show_bug.cgi?id=` strings in the PR title and description and set the corresponding label for the opened PR.
```
For each open PR:
If PR with a bug.webkit.org bug in the description:
Set `webkit-export` label in the PR
```
### 2) Getting review (r+) on the WebKit patch approves the export PR
The wpt-pr-bot or a Github Action will check Bugzilla issue history
```
For each opened PR labeled as `webkit-export` :
Check the `changes` in `https://bugs.webkit.org/rest/bug/999999/history`:
If `r+`:
Set `Approved` in the PR
```
The Bugzilla bug *id.* is obtained from the name of the branch set in the PR `ref` (e.x: `wpt-export-for-webkit-202311` - https://api.github.com/repos/web-platform-tests/wpt/pulls/19381). Then, we could double-check the validity of the bug *id.* using the `see_also` field from the Bugzilla.
Then WPT can check the Bugzilla status easily getting this information from the Bugzilla rest API:
```python
>>> import requests
>>> r = requests.get("https://bugs.webkit.org/rest/bug/200081/history")
>>> j = r.json()
>>> j['bugs'][0]['history'][6]['changes'][1]['added']
u'review+'
```
### 3) The export PR is merged when the WebKit patch is about to land
We don't want to wait until the bug is closed because the WebKit workflow is that upstream (`@rniwa`) WPT PR must be merged before the patch is landed in WebKit side.
The `wpt-pr-bot` or a Github Action will check Bugzilla issue history
```
For each opened PR labeled as `webkit-export` :
Check the `changes` in `https://bugs.webkit.org/rest/bug/999999/history`:
If `cq+`:
PR merged
```
WPT will check the Bugzilla status from the Bugzilla rest API as well:
```python
>>> import requests
>>> r = requests.get("https://bugs.webkit.org/rest/bug/200081/history")
>>> j = r.json()
>>> j['bugs'][0]['history'][6]['changes'][1]['added']
U'queue+'
```
## Other considerations
* No changes required in the usual workflow for the WebKit developers
* No changes in the `export-w3c-tests` scripts a part of some polish needed identified
* Does `export-w3c-tests` controls if there is already created PR for a particular bug?
* We need to take the decision about implementing this with Github Actions or keep this logic in wpt-pr-bot
* Changes to WebKit trunk which don't have an export PR gets one created, approved and merged ASAP
# Import W3C tests process
## Current situation in the import process
On the other hand, the import process is implemented in the `./Tools/Scripts/import-w3c-tests` script. This is triggered by a manual process documented in:
* https://trac.webkit.org/wiki/WebKitW3CTesting
Some limitations:
* `testharness.js` tests that have all PASS still need `-expected.txt` files in WebKit. This is a more generic issue than only WPT, but affects the import process as it needs to create these `-expected.txt` files. Ideally we won’t need a `-expected.txt` if the output is all passes. Probably this needs discussion and agreement with the rest of the WebKit community, to make it happen on a general level.
* Import script doesn’t detect tests that have been moved, so you can end up with duplicated tests if you run the import process twice and a test has been renamed. Dunno how we could detect that or how is that done in Blink for example.
* Import is manual, and people only run it when they want to import a test suite for a feature, or a few tests for a specific bug or things like that. Result is that tests end up being outdated compared to WPT. Ideally there should be a bot running the import process and keeping tests up to date.
> [name=Philip Jägenstedt]
> I believe the key outstanding issue here is whether the WebKit community think there's value in requiring a human in the loop. What sort of errors would a human reviewer be looking for? There may be a bunch of yet unknown work to make import reliable enough to be trusted to run unsupervised
This script has also some issues at this moment:
* [137933](https://bugs.webkit.org/show_bug.cgi?id=137933) – import-w3c-tests should not die if it can't convert a test
* [186132](https://bugs.webkit.org/show_bug.cgi?id=186132) – export-w3c-test-changes can not export reftests
* [137968](https://bugs.webkit.org/show_bug.cgi?id=137968) – import-w3c-tests should handle parsing & converting SVG files as tests
* [185876](https://bugs.webkit.org/show_bug.cgi?id=185876) – Do not convert testharness paths in import-w3c-tests
* [136577](https://bugs.webkit.org/show_bug.cgi?id=136577) – import-w3c-tests should handle paths in ref files that originate above the test file's parent dir.
* [135930](https://bugs.webkit.org/show_bug.cgi?id=135930) – import-w3c-tests should parse JS code to look for get/setPropertyValue() and prefix properties
* [186045](https://bugs.webkit.org/show_bug.cgi?id=186045) – webkit-test-runner: Add support for the reftest-wait class name
Related to the import process is the rebasing tests logic (`@youennf`) as well. This process consists in updating the revision and reimporting all tests. It is not yet automated. A preliminary patch is available at [134767](https://bugs.webkit.org/show_bug.cgi?id=137933).
## Proposed changes in the import workflow
For the case of the import from WPT to WebKit:
1. Supporting <link rel=> for finding ref tests
1. Avoid -expected.txt files for testharness.js tests that are passing
1. Import-w3c-tests running in a bot
### 1) Supporting <link rel=> for finding ref tests
Simon Fraser is working on a patch to support `<link rel=>` in WebKit:
* Mailing list thread: https://lists.webkit.org/pipermail/webkit-dev/2019-November/030940.html
* Bug: https://bugs.webkit.org/show_bug.cgi?id=203784
That would simplify a lot the current status of things, as the WebKit import is creating `-expected` files for all the tests, and renaming reference files (which won’t be needed with this change).
The blocker for this is This patch does add some overhead for parsing each test file,
which I measured to be about 1-2 sec on a directory which took 30s to run.
WPT has recently passed an RFC <https://github.com/web-platform-tests/rfcs/blob/master/rfcs/reftest_simplification.md> trying to simplify the reftest graph (although it has not been implemented yet), which eliminates a lot of the complexities and concerns. This could eventually fix this leftover.
### 2) Avoid -expected.txt files for testharness.js tests that are passing
Chromium already does this and it seems it might be useful to simplify the importing process. Right now we have a -expected.txt for every `testharness.js` test, even when it contains only passes, if we could avoid that file in those cases we’ll be creating less files during the import process. Of course it’s still needed to have -expected.txt when there’s a mix of PASS and FAIL on the test results.
### 3) Import-w3c-tests running in a bot
Automate our import/export workflow. The bot will need to do a few things:
* Import the tests (`import-w3c-tests`)
* Upload the change to bugzilla so EWSs run it (for WebKitGTK+/WPE the bot should run them locally, as EWSs don’t run tests yet).
* The EWS will attach the `layout-test-results.zip` to the issue
* In case of EWS reports **NON SUCCESS** in the layout tests, the bot will update the test results (note that this is platform dependant) extracting info from EWSs runs `layout-test-results.zip`):
1. Parses the full_results.json summary
1. For failed tests:
* Generating -expected.txt files for testharness.js tests that have failures
* Update the -expected.txt file for the test with the -actual.txt
1. Upload again the patch to Bugzilla
* In case of **SUCCESS** running the layout tests:
* Request `cq?` (later this could be `cq+`)
* Once the patch is landed:
* Report issues found (only in case of ...) on a new Bugzilla issue (linking the revision). Ideally adding relevant people on CC if that’s possible (in Chromium there’s a file with owners, maybe in WebKit it could use `Source/WebCore/features.json` for the features that have a contact and put it on CC).
## Other considerations for the import process
* This bot must have some way to identify the reports open in order to track the evolution of each import process.
* The bot will trigger a new import process only once all the previous import issues will be landed. In the opposite case, the bot will skip the import of the new tests until the previous request were landed.
* Probably this new mechanism requires a first manual import because of the tests imported by `import-w3c-tests` are very outdated right