interop icon indicating copy to clipboard operation
interop copied to clipboard

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

Open dandclark opened this issue 9 months ago • 4 comments

The tests added for https://github.com/web-platform-tests/interop/issues/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 avatar Feb 27 '25 19:02 dandclark

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 https://github.com/web-platform-tests/wpt-metadata/commit/c6956f04c6a6e2a9286ed4c48db9929b8a1f976b and https://github.com/web-platform-tests/wpt-metadata/commit/7a1f96e419694683ac451b32d401d35dfef30ab4. I'm curious as to why these tests were added in the first place — they don't seem at all related?

gsnedders avatar Feb 27 '25 22:02 gsnedders

CC @asutherland @edenchuang

jgraham avatar Mar 06 '25 10:03 jgraham

The actions proposed make sense. Apologies since I'm probably the source of the mismatch between selection of tests and the phrasing in https://github.com/web-platform-tests/interop/issues/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 https://github.com/web-platform-tests/interop/issues/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.

asutherland avatar Mar 07 '25 02:03 asutherland

We discussed this at the last Interop meeting and agreed it's the right way forward. Here are the PRs to swap the tests: https://github.com/web-platform-tests/wpt-metadata/pull/7471 https://github.com/web-platform-tests/wpt-metadata/pull/7470

Note that the tests labeled in https://github.com/web-platform-tests/wpt-metadata/pull/7470 were already updated to the correct behavior here: https://github.com/web-platform-tests/wpt/pull/51200

dandclark avatar Mar 25 '25 20:03 dandclark

PRs are merged, so closing this. Thanks everyone!

dandclark avatar Apr 03 '25 16:04 dandclark