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
Low-friction workflow to export/import tests from web-platform-tests in WebKit (@foolip
).
The export workflow should address a solution for:
Philip Jägenstedt
This sounds like the PR will be merged after the
WebKit patch lands, but the current rule is before.
Pablo Saavedra
See my comment here: github.com/web-platform-tests/wpt-pr-bot/issues/100#issuecomment-550056150Right 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:
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:
$ 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
):
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.
The best case for export from WebKit to WPT is an integration using bot actions for each of these events:
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.Philip Jägenstedt
Yes, definitely.
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.
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
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:
>>> 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+'
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:
>>> 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+'
export-w3c-tests
scripts a part of some polish needed identifiedexport-w3c-tests
controls if there is already created PR for a particular bug?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:
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.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:
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.
For the case of the import from WPT to WebKit:
Simon Fraser is working on a patch to support <link rel=>
in WebKit:
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.
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.
Automate our import/export workflow. The bot will need to do a few things:
import-w3c-tests
)layout-test-results.zip
to the issuelayout-test-results.zip
):
cq?
(later this could be cq+
)Source/WebCore/features.json
for the features that have a contact and put it on CC).import-w3c-tests
are very outdated right