Bug 203783 - Stop parsing reftest.list files
Summary: Stop parsing reftest.list files
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sam Sneddon [:gsnedders]
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-11-02 10:44 PDT by Simon Fraser (smfr)
Modified: 2020-12-17 15:21 PST (History)
10 users (show)

See Also:


Attachments
Patch (15.20 KB, patch)
2020-12-17 14:02 PST, Sam Sneddon [:gsnedders]
no flags Details | Formatted Diff | Diff
Patch (15.24 KB, patch)
2020-12-17 14:18 PST, Sam Sneddon [:gsnedders]
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2019-11-02 10:44:45 PDT
Code in _parse_reftest_list() in port/base.py (very odd that this is in port!) parses reftest.list files when those exist, and uses that to decide what reference to use for a test.

reftest.list seems to be a vestige of Mozilla-imported tests (https://developer.mozilla.org/en-US/docs/Mozilla/QA/web-platform-tests#Metadata). WPT doesn't use these lists any more and instead uses <link rel="match"> from the tests themselves.

In addition, when we import WPT I think we have to clone a bunch of shared references into lots of identical -expected.html, which we would not if we respected <link rel="match">
Comment 1 Simon Fraser (smfr) 2019-11-02 11:12:24 PDT
retest.list is only present in:
LayoutTests % find . -name reftest.list                    
./imported/w3c/web-platform-tests/encoding/reftest.list
./media/track/opera/track/webvtt/rendering/reftest/reftest.list

We need to re-import encoding tests to get rid of the first one. Maybe we can migrate to WPT media/track tests to eliminate the second?
Comment 2 Carlos Alberto Lopez Perez 2020-02-13 16:38:01 PST
I'm updating the imported WPT encoding tests in bug 207644
I encountered unexpected failures with those tests when updating them and then I realized there where old -ref.html and new -expected.html references so I found about this reftest.list thing :)

I'm deleting there the imported/w3c/web-platform-tests/encoding/reftest.list file and moving the references affected to use the usual convention of -expected.html

It seems support for reftest.list was introduced in bug 66837
Comment 3 Sam Sneddon [:gsnedders] 2020-12-17 04:47:44 PST
(In reply to Simon Fraser (smfr) from comment #1)
> We need to re-import encoding tests to get rid of the first one. Maybe we
> can migrate to WPT media/track tests to eliminate the second?

We should be able to; zcorpan's WebVTT tests were added to WPT long ago.
Comment 4 Sam Sneddon [:gsnedders] 2020-12-17 06:16:00 PST
Looking into this, it seems like we _already_ don't run anything from reftest.list; I think ./Tools/Scripts/webkit-patch print-baselines is the only command that actually ends up running the code path that actually parses them!

So I don't think we need to block on finally importing the WebVTT tests from WPT, given we aren't losing any test coverage if we remove them?
Comment 5 Sam Sneddon [:gsnedders] 2020-12-17 13:18:37 PST
Ah, finally got to the bottom of this. https://trac.webkit.org/changeset/136459/webkit added the Opera tests, _but_ https://trac.webkit.org/changeset/101727/webkit had meant we never consider file in a directory called "reftest" as a tests (look at skipped_directories).

So we've literally never run anything from ./media/track/opera/track/webvtt/rendering/reftest/reftest.list, which I guess means we have less test coverage than we thought. Thus we don't need to worry about lost coverage from dropping support for this!
Comment 6 Sam Sneddon [:gsnedders] 2020-12-17 14:02:37 PST
Created attachment 416466 [details]
Patch
Comment 7 Jonathan Bedard 2020-12-17 14:11:37 PST
Comment on attachment 416466 [details]
Patch

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

> Tools/ChangeLog:10
> +        We do still have one reftest.list file, media/track/opera/track/webvtt/rendering/reftest/reftest.list, but as this is in a reftest directory we don't run any of the tests in it (and never have). See the above bug for more detail about that history.

Should probably be 2 lines.
Comment 8 Sam Sneddon [:gsnedders] 2020-12-17 14:18:43 PST
Created attachment 416467 [details]
Patch
Comment 9 EWS 2020-12-17 15:20:16 PST
Committed r270954: <https://trac.webkit.org/changeset/270954>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 416467 [details].
Comment 10 Radar WebKit Bug Importer 2020-12-17 15:21:20 PST
<rdar://problem/72443355>