Bug 183356

Summary: Add run-web-platform-tests script
Product: WebKit Reporter: Zan Dobersek <zan>
Component: Tools / TestsAssignee: Zan Dobersek <zan>
Status: RESOLVED FIXED    
Severity: Normal CC: amal, bburg, brendan, cdumez, clopez, ews, fred.wang, glenn, jbedard, lforschler, rego, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
WIP patch
none
Patch
none
WIP
none
Patch
none
Patch
none
Patch for landing none

Description Zan Dobersek 2018-03-05 23:50:07 PST
This script would allow us to run the web-platform-tests suite.
https://github.com/w3c/web-platform-tests
Comment 1 Zan Dobersek 2018-03-05 23:55:48 PST
Created attachment 335070 [details]
WIP patch

Work-in-progress run-web-platform-tests script that supports running the W3C web-platform-tests suite for the GTK+ port.

- MiniBrowser change shouldn't be necessary, but SSL certificates have to be correctly generated.
- The Python script has to be cleaned up and segmented into testable bits.
- Path to a web-platform-tests checkout still has to be passed via --source argument, but this should instead be automatically downloaded, as is the case with the import-w3c-tests script.
- 2dcontext and WebCryptoAPI tests are enabled -- more should follow, but it's an OK start.
Comment 2 Zan Dobersek 2018-04-17 03:13:21 PDT
Created attachment 338095 [details]
Patch
Comment 3 youenn fablet 2018-04-17 15:38:27 PDT
I would tend to make the main script as a class with various methods.
That would allow to improve the readability and maybe we could then write some unit tests for these.

As a side note, the manifest.json generation is quite long iirc, so maybe it could be cached somewhere based on the revision of the repo.
Comment 4 Zan Dobersek 2018-04-20 05:21:21 PDT
Created attachment 338410 [details]
WIP

Still missing unit tests.
Comment 5 Zan Dobersek 2018-05-01 11:52:44 PDT
Created attachment 339209 [details]
Patch

Now with unittests.
Comment 6 Build Bot 2018-05-01 11:55:38 PDT
Attachment 339209 [details] did not pass style-queue:


ERROR: Tools/Scripts/webkitpy/w3c/wpt_runner_unittest.py:88:  whitespace before '}'  [pep8/E202] [5]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Zan Dobersek 2018-05-14 02:15:49 PDT
Youenn, any feedback?
Comment 8 youenn fablet 2018-05-14 02:52:59 PDT
Comment on attachment 339209 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=339209&action=review

> Tools/Scripts/webkitpy/w3c/wpt_runner.py:54
> +        top_level_directory = os.path.normpath(os.path.join(os.path.dirname(__file__), '..', '..', '..', '..'))

We usually prefer using filesystem instead of os.path here and below.

> Tools/Scripts/webkitpy/w3c/wpt_runner.py:61
> +            print '***'

Could we use _log.warning instead?

> Tools/Scripts/webkitpy/w3c/wpt_runner.py:65
> +    display_driver = port.create_driver(worker_number=0, no_timeout=True)._make_driver(pixel_tests=False)

We are not supposed to use private methods directly. I see that _make_driver and _setup_environ_for_test are used directly in webdriver_test_runner.py already though.

> Tools/Scripts/webkitpy/w3c/wpt_runner.py:133
> +            self._options.wpt_checkout = self._finder.path_from_webkit_base("WebKitBuild", "w3c-tests", "web-platform-tests")

The default checkout path could be shared with the importer and exporter.
Comment 9 youenn fablet 2018-05-14 02:54:00 PDT
Woups, my comments were uploaded to quickly.
Anyway, looks good to me. It would be nice to have some additional feedback from others.
Comment 10 Zan Dobersek 2018-05-14 06:22:51 PDT
Comment on attachment 339209 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=339209&action=review

>> Tools/Scripts/webkitpy/w3c/wpt_runner.py:54
>> +        top_level_directory = os.path.normpath(os.path.join(os.path.dirname(__file__), '..', '..', '..', '..'))
> 
> We usually prefer using filesystem instead of os.path here and below.

OK.

>> Tools/Scripts/webkitpy/w3c/wpt_runner.py:61
>> +            print '***'
> 
> Could we use _log.warning instead?

OK.

>> Tools/Scripts/webkitpy/w3c/wpt_runner.py:65
>> +    display_driver = port.create_driver(worker_number=0, no_timeout=True)._make_driver(pixel_tests=False)
> 
> We are not supposed to use private methods directly. I see that _make_driver and _setup_environ_for_test are used directly in webdriver_test_runner.py already though.

That's right, both scripts require such driver setup and I just mirrored webdriver_test_runner.py for that.

>> Tools/Scripts/webkitpy/w3c/wpt_runner.py:133
>> +            self._options.wpt_checkout = self._finder.path_from_webkit_base("WebKitBuild", "w3c-tests", "web-platform-tests")
> 
> The default checkout path could be shared with the importer and exporter.

I can move this into a single location, perhaps a new Python module under webkitpy.w3c.
Comment 11 Amal Hussein 2018-05-16 11:27:43 PDT
Hi Zan, 

I'm curious how this is different than the existing test runners? There is ongoing work to
Comment 12 Amal Hussein 2018-05-16 11:31:41 PDT
Hi Zan, 

I'm curious how this is different than the existing test runner? There is ongoing work to automate the importing, running tests and auto-updating the expectations file. There is also ongoing work to automate the export flow. 

Amal
Comment 13 Zan Dobersek 2018-05-18 01:27:41 PDT
(In reply to Amal Hussein from comment #12)
> Hi Zan, 
> 
> I'm curious how this is different than the existing test runner? There is
> ongoing work to automate the importing, running tests and auto-updating the
> expectations file. There is also ongoing work to automate the export flow. 
> 
> Amal

This work doesn't mean to replace the existing W3C testing process that's based on importing the web-platform-tests test cases and running them inside the regression testing process. At some point it might come to that, but it's far from it today (or whenever this script lands). This hasn't changed since the debate in webkit-dev:
https://lists.webkit.org/pipermail/webkit-dev/2018-February/029869.html

Value of this script is that it enables running an 'original' WPT checkout against a WebKit build. While it can't replace the current W3C testing process, I think it can serve as a simple standards compliance scoreboard, and CI bots producing that would be set up.

So to a degree, this would duplicate the testing coverage. I would still argue there's value in adopting both approaches -- the current one aims to integrate these tests into the regression testing process for the purposes of developing compliant implementations inside WebKit, while the new one would test WebKit against the official test repository. Issue I'm most worried about is how much maintenance the additional testing process would require (in terms of gardening, i.e. adjusting expectations).
Comment 14 Zan Dobersek 2018-05-31 01:29:57 PDT
Created attachment 341651 [details]
Patch
Comment 15 Build Bot 2018-05-31 01:31:28 PDT
Attachment 341651 [details] did not pass style-queue:


ERROR: Tools/Scripts/webkitpy/w3c/wpt_runner_unittest.py:88:  whitespace before '}'  [pep8/E202] [5]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 16 Carlos Alberto Lopez Perez 2018-06-11 09:54:22 PDT
Comment on attachment 341651 [details]
Patch

Patch LGTM, but I think it needs a rebase to apply on last master.
Comment 17 Zan Dobersek 2018-06-12 00:26:22 PDT
Created attachment 342519 [details]
Patch for landing
Comment 18 Build Bot 2018-06-12 00:27:42 PDT
Attachment 342519 [details] did not pass style-queue:


ERROR: Tools/Scripts/webkitpy/w3c/wpt_runner_unittest.py:88:  whitespace before '}'  [pep8/E202] [5]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 19 Zan Dobersek 2018-06-12 00:55:45 PDT
Comment on attachment 342519 [details]
Patch for landing

Clearing flags on attachment: 342519

Committed r232746: <https://trac.webkit.org/changeset/232746>
Comment 20 Zan Dobersek 2018-06-12 00:55:50 PDT
All reviewed patches have been landed.  Closing bug.
Comment 21 Radar WebKit Bug Importer 2018-06-12 00:56:27 PDT
<rdar://problem/41036622>