wpt icon indicating copy to clipboard operation
wpt copied to clipboard

Remove import assertions tests

Open nicolo-ribaudo opened this issue 1 year ago • 7 comments

All the removes tests have an equivalent test using import attributes (with):

with keyword assert keyword
./import-attributes ./import-assertions
./json-module ./json-module-assertions
./css-module ./css-module-assertions

The assert syntax is only supported by Chrome, which:

Safari only supports with, and Firefox supports neither but is implementing with.

Given that the set of platforms supporting assert is a subset of those that support with, having both syntax variants doesn't help with coverage. The difference between with and assert is purely syntactical (it's like testing fetch("http://example.com") and fetch(`http://example.com`)). test262 still tests both syntaxes, given that there are some parsing differences between them.

nicolo-ribaudo avatar May 02 '24 09:05 nicolo-ribaudo

I hope that eventually we can add a historical test for the assert syntax (i.e., to pass you need to not support it).

This is a good idea -- I'll submit one as soon as the proposal actually gets to stage 4 :)

nicolo-ribaudo avatar May 02 '24 09:05 nicolo-ribaudo

Were these directories copied and edited, or how can we be confident the coverage is the same? Have there been any changes to the directories since that copying?

foolip avatar May 02 '24 09:05 foolip

They were copied and edited in https://github.com/web-platform-tests/wpt/commit/b62c015e63c9bca18ab31c6739d1e0588b5bd299 (replacing assert with with).

The other commits to any of those folders (both with and assert versions) were:

  • https://github.com/web-platform-tests/wpt/commit/577d22542f1321249653d695626f05aa42320ff7, https://github.com/web-platform-tests/wpt/commit/ac7ea8fb67727236ae4ddf2ec2d4922c47287b76, and https://github.com/web-platform-tests/wpt/commit/6593bded6f610c2616e53075d42b51d9566a7f54, which fixed some changes I missed while copy-pasting the tests
  • https://github.com/web-platform-tests/wpt/commit/bcae71ef7480ab3ac284b22f7c3768ff366f9136 and https://github.com/web-platform-tests/wpt/commit/e3365d8440796f310ed38d4de3d4789fb611bb96, applied to both the with and assert variants

nicolo-ribaudo avatar May 02 '24 10:05 nicolo-ribaudo

I can reproduce the CI error locally, but I'm also getting it on master

nicolo-ribaudo avatar May 02 '24 13:05 nicolo-ribaudo

Thanks @nicolo-ribaudo, it sounds like things are in sync then.

@syg would removing these tests be a problem for Chromium/V8? Do we depend on them to avoid regressions?

foolip avatar May 02 '24 15:05 foolip

@syg would removing these tests be a problem for Chromium/V8? Do we depend on them to avoid regressions?

@foolip Removing it is premature IMO. We've agreed to deprecate and remove support for import assertions in Chrome M126. What I'm doing now is to land CLs that changes the import assertions flag from default enabled to default disabled, but not removing code and tests until M126 hits stable for a milestone or two and we get the final evidence that it is indeed a web compatible change.

So my planned timeline is:

  1. In M126, land V8 and chromium CLs that changes --harmony-import-assertions to default disabled without removing code and tests. Move the current import assertion tests to a virtual test suite that passes the flag explicitly.
  2. Wait for M126 to hit stable for a milestone or two.
  3. If there are no web incompatibility bugs, remove code and tests.
  4. Else, assess severity of breakage, possibly flipping flag back on and go back to TC39.

syg avatar May 02 '24 15:05 syg

Marking this as draft for now, I'll come back in a couple months :)

nicolo-ribaudo avatar May 02 '24 16:05 nicolo-ribaudo

assert has been fully removed in the July 2024 TC39 meeting.

nicolo-ribaudo avatar Aug 22 '24 09:08 nicolo-ribaudo

From the mutation event removal I know that Chrome M126 is in the past, but maybe @syg could explicitly confirm he's okay with this?

annevk avatar Aug 22 '24 10:08 annevk

Yep, confirm that this is good to land. After this V8 will remove code for supporting assert.

syg avatar Aug 22 '24 20:08 syg