search

web-platform-tests / interop

web-platform-tests Interop project
https://wpt.fyi/interop
296 stars 28 forks source link

Fix grid-shorthand-serialization.html so that test names aren't dependent on browser internals (to avoid spurious MISSING test results) #646

Closed dholbert closed 6 months ago

dholbert commented 6 months ago

Test List

https://wpt.fyi/results/css/css-grid/parsing/grid-shorthand-serialization.html

Rationale

tl;dr: Right now each browser engine is passing this test, but is also shown with 4 MISSING entries on wpt.fyi, for silly reasons. I'm proposing we fix that.

The MISSING entries are "missing" because the test results are classified based on the test(..., name); string-values; and for this particular subtest, those strings are generated dynamically based on the browser's serialization of the cssText: https://github.com/web-platform-tests/wpt/blob/50a3438fae/css/css-grid/parsing/grid-shorthand-serialization.html#L36-L40 ...and Chromium/WebKit happen to use a different serialization order for some grid subproperties than Firefox does, so they end up with a slightly different name for the subtest. (I think this is probably a bug in Chromium/WebKit; I filed bugs (Chromium bug, WebKit bug) -- but those bugs validity isn't super important here.)

We can avoid this inconsistency in the test and these spurious MISSING results by adjusting the test to use the hardcoded cssText string from the test itself in the subtest's description, which is probably a good best-practice anyway; it's awkward to have unknown UA-provided text included in the subtest name string, given that the subtest name has special significance.

dholbert commented 6 months ago

See PR https://github.com/web-platform-tests/wpt/pull/45091

I don't think this change impacts any browser's interop score, since everyone is already shown as passing 50/50 subtests here. This just avoids generating these confusing rows with MISSING test-results in each browser.

dholbert commented 6 months ago

(jgraham observed in the PR that this is just fixing a trivial bug in the test where it was clearly violating testharness.js best-practices -- so rather than bothering folks for approval on what is really an uncontroversial bug-fix with no behavior/score impact, I went ahead and merged.)