Bug 234510 - Stop assuming WPT is a reftest based on existence of -ref.html file
Summary: Stop assuming WPT is a reftest based on existence of -ref.html file
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: Safari 14
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tim Nguyen (:ntim)
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-12-20 06:29 PST by Tim Nguyen (:ntim)
Modified: 2022-01-11 09:03 PST (History)
8 users (show)

See Also:


Attachments
Patch (2.95 KB, patch)
2021-12-20 07:05 PST, Tim Nguyen (:ntim)
no flags Details | Formatted Diff | Diff
Patch (4.04 KB, patch)
2021-12-20 08:31 PST, Tim Nguyen (:ntim)
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tim Nguyen (:ntim) 2021-12-20 06:29:05 PST
Only the presence of <link rel="match">/<link rel="mismatch"> indicates a test is a reftest. The script should not assume that the existence of a similarly named file with a `-ref.html` suffix means that the original test file is reftest.
Comment 2 Tim Nguyen (:ntim) 2021-12-20 07:05:58 PST
Created attachment 447601 [details]
Patch
Comment 3 Manuel Rego Casasnovas 2021-12-20 07:19:00 PST
Comment on attachment 447601 [details]
Patch

Why this was added initially? Are we going to be missing any test now (or they were actually just being imported but not working fine)?
Comment 4 Tim Nguyen (:ntim) 2021-12-20 07:48:02 PST
(In reply to Manuel Rego Casasnovas from comment #3)
> Comment on attachment 447601 [details]
> Patch
> 
> Why this was added initially? Are we going to be missing any test now (or
> they were actually just being imported but not working fine)?

Blame links to: https://github.com/WebKit/WebKit/commit/fe8645cb599fd89729602a5c992f1fa93a88f5d7

Maybe it was useful before?

Anyway, this is troublesome for the top-layer-position tests:

https://github.com/web-platform-tests/wpt/tree/master/html/semantics/interactive-elements/the-dialog-element

where we have:

top-layer-position-ref.html
top-layer-position-relative.html
top-layer-position-static.html
top-layer-position.html

The ref is for the relative & static tests, not the plain one.
Comment 5 Sam Sneddon [:gsnedders] 2021-12-20 08:02:00 PST
Comment on attachment 447601 [details]
Patch

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

(non-reviewer r-)

> Tools/Scripts/webkitpy/w3c/test_parser.py:-124
> -        elif self.is_wpt_reftest():

Removing this changes the semantics of everything that follows; notably this means things can get picked up as manual tests now when previously they were excluded for having `-ref` in the name.

Compare this with https://github.com/web-platform-tests/wpt/blob/22f29564bb82b407aeaf6507c8efffdbd51b9974/tools/manifest/sourcefile.py#L1041 where certain tests are excluded from the possibility of being picked up as manual tests as a result of this.
Comment 6 Tim Nguyen (:ntim) 2021-12-20 08:31:38 PST
Created attachment 447605 [details]
Patch
Comment 7 Sam Sneddon [:gsnedders] 2021-12-20 09:02:10 PST
Comment on attachment 447605 [details]
Patch

This looks good, aside from the lack of tests in test_parser_unittest.py for this behaviour (which we really ought to have!).
Comment 8 Tim Nguyen (:ntim) 2021-12-20 11:03:57 PST
Committed r287267 (245423@trunk): <https://commits.webkit.org/245423@trunk>
Comment 9 Radar WebKit Bug Importer 2021-12-20 11:04:16 PST
<rdar://problem/86732196>
Comment 10 Tim Nguyen (:ntim) 2021-12-20 11:05:07 PST
(In reply to Sam Sneddon [:gsnedders] from comment #7)
> Comment on attachment 447605 [details]
> Patch
> 
> This looks good, aside from the lack of tests in test_parser_unittest.py for
> this behaviour (which we really ought to have!).

I added tests in the patch I landed: https://commits.webkit.org/r287267