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

Incorrect set of tests for "Error events when a worker is blocked via CSP" #932

Open
dandclark opened this issue Feb 27, 2025 · 3 comments
Labels
focus area: Web Compat 2025 test-change-proposal Proposal to add or remove tests for an interop area

Comments

@dandclark
Copy link
Contributor

dandclark commented Feb 27, 2025

The tests added for #855, which was wrapped into the WebCompat focus area, are in an odd state.

These are the current tests: https://wpt.fyi/results/workers/constructors?label=master&label=experimental&view=interop&q=label%3Ainterop-2025-webcompat

None of them actually check the failure-due-to-CSP scenario that was #855 was raised about. They test other kinds of Worker construction failure scenarios which would need their own compatibility analyses to see whether they're feasible to change.

Meanwhile the tests that check the CSP scenario, as @gsnedders pointed out, are here: https://wpt.fyi/results/content-security-policy/worker-src?label=experimental&label=master&aligned
But, these are checking the wrong behavior, and as @LiangTheDev commented here they need to be updated to match the spec so that they check for an error event instead of the Worker constructor throwing an exception. Liang's CL https://chromium-review.googlesource.com/c/chromium/src/+/6155655 would update those tests. The result of that test update would be that the failures Firefox has in https://wpt.fyi/results/content-security-policy/worker-src?label=experimental&label=master&aligned would start passing, and those same tests would start failing in Chrome, Edge, and Safari instead.

So to align with the intent of #855 I think the right thing is to:

  1. Land the test changes in https://chromium-review.googlesource.com/c/chromium/src/+/6155655 so that the failures in https://wpt.fyi/results/content-security-policy/worker-src?label=experimental&label=master&aligned change to the opposite set of browsers.
  2. Change the WebCompat focus area tests for this issue from https://wpt.fyi/results/workers/constructors?label=master&label=experimental&view=interop&q=label%3Ainterop-2025-webcompat to https://wpt.fyi/results/content-security-policy/worker-src?label=experimental&label=master&aligned

(cc @jgraham who proposed #855)

@dandclark dandclark added the test-change-proposal Proposal to add or remove tests for an interop area label Feb 27, 2025
@gsnedders
Copy link
Member

These are the current tests: https://wpt.fyi/results/workers/constructors?label=master&label=experimental&view=interop&q=label%3Ainterop-2025-webcompat

These were labelled by @jgraham in web-platform-tests/wpt-metadata@c6956f0 and web-platform-tests/wpt-metadata@7a1f96e. I'm curious as to why these tests were added in the first place — they don't seem at all related?

@jgraham
Copy link
Contributor

jgraham commented Mar 6, 2025

CC @asutherland @edenchuang

@asutherland
Copy link

The actions proposed make sense. Apologies since I'm probably the source of the mismatch between selection of tests and the phrasing in #855 since in general I was conflating all cases of "Worker constructor synchronously throws but it should not because that violates spec and changing the spec would represent an undesirable permanent divergence as it relates to the fetch spec" as being effectively the same. But #855 is very specifically about the CSP case so even though we'd expect both sets of tests to match in behavior, it seems appropriate that only the CSP tests are associated with the given issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
focus area: Web Compat 2025 test-change-proposal Proposal to add or remove tests for an interop area
Projects
None yet
Development

No branches or pull requests

5 participants