Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
dholbert opened this issue Mar 13, 2024 · 2 comments · Fixed by web-platform-tests/wpt#45091
Labels
test-change-proposal Proposal to add or remove tests for an interop area

Comments

@dholbert
Copy link

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 dholbert added the test-change-proposal Proposal to add or remove tests for an interop area label Mar 13, 2024
dholbert added a commit to dholbert/wpt that referenced this issue Mar 13, 2024
… results.

Fixes web-platform-tests/interop#646

This patch improves WPT grid-shorthand-serialization.html to better distinguish
between the `cssText` function-param vs. the serialization of that string that
the test reads back from the style system.

As of this patch, we'll now use the function-param (not its serialization) when
generating the subtest "name" value that we pass to the test(...) function.
This makes for more consistent output and removes the possibility for
browser-specific serialization-quirks to influence the subtest-name, as
discussed in web-platform-tests/interop#646

The test continues to use the serialized value (under a new/clearer variable
name) in the assert_true() expression, though; this way, if the test fails, it
remains easy to see the actual strings that are involved in the `indexOf`
operation that's causing the test-failure.
@dholbert
Copy link
Author

See PR web-platform-tests/wpt#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 added a commit to web-platform-tests/wpt that referenced this issue Mar 13, 2024
… results. (#45091)

Fixes web-platform-tests/interop#646

This patch improves WPT grid-shorthand-serialization.html to better distinguish
between the `cssText` function-param vs. the serialization of that string that
the test reads back from the style system.

As of this patch, we'll now use the function-param (not its serialization) when
generating the subtest "name" value that we pass to the test(...) function.
This makes for more consistent output and removes the possibility for
browser-specific serialization-quirks to influence the subtest-name, as
discussed in web-platform-tests/interop#646

The test continues to use the serialized value (under a new/clearer variable
name) in the assert_true() expression, though; this way, if the test fails, it
remains easy to see the actual strings that are involved in the `indexOf`
operation that's causing the test-failure.
@dholbert
Copy link
Author

(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.)

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Mar 15, 2024
…o avoid spurious MISSING test results., a=testonly

Automatic update from web-platform-tests
Use clearer variable names in grid WPT to avoid spurious MISSING test results. (#45091)

Fixes web-platform-tests/interop#646

This patch improves WPT grid-shorthand-serialization.html to better distinguish
between the `cssText` function-param vs. the serialization of that string that
the test reads back from the style system.

As of this patch, we'll now use the function-param (not its serialization) when
generating the subtest "name" value that we pass to the test(...) function.
This makes for more consistent output and removes the possibility for
browser-specific serialization-quirks to influence the subtest-name, as
discussed in web-platform-tests/interop#646

The test continues to use the serialized value (under a new/clearer variable
name) in the assert_true() expression, though; this way, if the test fails, it
remains easy to see the actual strings that are involved in the `indexOf`
operation that's causing the test-failure.
--

wpt-commits: 227a77f3eac2653c34706d578bdd709b3897e962
wpt-pr: 45091
jamienicol pushed a commit to jamienicol/gecko that referenced this issue Mar 17, 2024
…o avoid spurious MISSING test results., a=testonly

Automatic update from web-platform-tests
Use clearer variable names in grid WPT to avoid spurious MISSING test results. (#45091)

Fixes web-platform-tests/interop#646

This patch improves WPT grid-shorthand-serialization.html to better distinguish
between the `cssText` function-param vs. the serialization of that string that
the test reads back from the style system.

As of this patch, we'll now use the function-param (not its serialization) when
generating the subtest "name" value that we pass to the test(...) function.
This makes for more consistent output and removes the possibility for
browser-specific serialization-quirks to influence the subtest-name, as
discussed in web-platform-tests/interop#646

The test continues to use the serialized value (under a new/clearer variable
name) in the assert_true() expression, though; this way, if the test fails, it
remains easy to see the actual strings that are involved in the `indexOf`
operation that's causing the test-failure.
--

wpt-commits: 227a77f3eac2653c34706d578bdd709b3897e962
wpt-pr: 45091
BruceDai pushed a commit to BruceDai/wpt that referenced this issue Mar 25, 2024
… results. (web-platform-tests#45091)

Fixes web-platform-tests/interop#646

This patch improves WPT grid-shorthand-serialization.html to better distinguish
between the `cssText` function-param vs. the serialization of that string that
the test reads back from the style system.

As of this patch, we'll now use the function-param (not its serialization) when
generating the subtest "name" value that we pass to the test(...) function.
This makes for more consistent output and removes the possibility for
browser-specific serialization-quirks to influence the subtest-name, as
discussed in web-platform-tests/interop#646

The test continues to use the serialized value (under a new/clearer variable
name) in the assert_true() expression, though; this way, if the test fails, it
remains easy to see the actual strings that are involved in the `indexOf`
operation that's causing the test-failure.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test-change-proposal Proposal to add or remove tests for an interop area
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant