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

Enable additional encoding tests in ShadowRealms #49286

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

ptomato
Copy link
Contributor

@ptomato ptomato commented Nov 20, 2024

Follow-up to #41968. Enables the rest of the TextEncoder, TextDecoder, TextEncoderStream, and TextDecoderStream test coverage inside ShadowRealm scopes.

@@ -1,6 +1,6 @@
// META: timeout=long
// META: title=Encoding API: Fatal flag for single byte encodings
// META: timeout=long
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might look odd at first glance. META: timeout=long was listed twice.

@inexorabletash
Copy link
Member

Overall LGTM - and very welcome! I especially appreciate modernizing older tests to be .any.js. A couple things:

  • Is the change to give an absolute path for META includes (e.g. resources/encodings.js/encoding/resources/encodings.js) required? WPTs use a mix; if this is now required for shadowrealm tests then documentation somewhere would be great. (Apologies if this is a well known issue, I haven't kept up with WPT evolution for a bit.)
  • Some of the checks are because Chrome and Firefox don't support SABs with the APIs and the tests fail - those are known issues, not a problem. The timeouts because AudioWorklet doesn't support shadow realm are probably new. It would be nice to rework the tests to fail quickly rather than timing out, as timeouts for the CI infrastructure wastes resources.

Apologies for the delay in providing feedback.

@ptomato
Copy link
Contributor Author

ptomato commented Nov 26, 2024

Thanks for the review!

Is the change to give an absolute path for META includes (e.g. resources/encodings.js/encoding/resources/encodings.js) required? WPTs use a mix; if this is now required for shadowrealm tests then documentation somewhere would be great. (Apologies if this is a well known issue, I haven't kept up with WPT evolution for a bit.)

This is a good question. It does need to be a module specifier, so I believe at least ./ would be required in any case. Currently because of the way I wrote the server and harness code for shadowrealm tests, it will resolve a relative specifier relative to /resources/testharness.js. This might be able to be fixed, which I can look into, and certainly documenting it if not is a good idea. I will probably do that in a separate PR.

Some of the checks are because Chrome and Firefox don't support SABs with the APIs and the tests fail - those are known issues, not a problem. The timeouts because AudioWorklet doesn't support shadow realm are probably new. It would be nice to rework the tests to fail quickly rather than timing out, as timeouts for the CI infrastructure wastes resources.

shadowrealm-in-audioworklet tests timing out instead of failing are probably the fault of my harness code. I'll take a look at this, but I'll probably also do that in a separate PR.

@annevk
Copy link
Member

annevk commented Nov 26, 2024

When there's multiple files of the same name, such as with single-byte-decoder, it'd be helpful to have a comment at the top saying how it relates to the other one.

The TextDecoder test can run in other scopes, because TextDecoder is
exposed everywhere.
This is an alternative that doesn't require using MessageChannel, which is
not exposed everywhere.
These seem to have been missed in web-platform-tests#41968.

Includes ensuring that exports from any support files are defined on the
global object, and that support scripts are referred to with their
absolute paths from the root.
@ptomato ptomato force-pushed the shadowrealm-more-text-encoding branch from 990184d to 231112f Compare November 27, 2024 01:27
@ptomato
Copy link
Contributor Author

ptomato commented Nov 27, 2024

The relative paths and timeouts are fixed in #49386 and #49387 respectively. I've reverted the relative-to-absolute-path scripts changes in this PR for ease of review, but it'll now need to be applied on top of #49386 for the tests to pass.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants