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 loading of WPT META scripts #209

Merged
merged 1 commit into from
Feb 12, 2025
Merged

Fix loading of WPT META scripts #209

merged 1 commit into from
Feb 12, 2025

Conversation

tschneidereit
Copy link
Member

WPT tests can load helper scripts using special comments at the top. These scripts need to be loaded as though using a <script> tag, which doesn't create a new lexical scope, and hence causes let and const bindings to be visible in the global scope.

Previously, to simulate this behavior using eval, the WPT harness replaced let and const with var. While this worked surprisingly well, it doesn't always work.

This commit instead introduces an evalScript function available to the harness, which evaluates in a way more closely matching that of script tags.

WPT tests can load helper scripts using special comments at the top. These scripts need to be loaded as though using a `<script>` tag, which doesn't create a new lexical scope, and hence causes `let` and `const` bindings to be visible in the global scope.

Previously, to simulate this behavior using `eval`, the WPT harness replaced `let` and `const` with `var`. While this worked surprisingly well, it doesn't *always* work.

This commit instead introduces an `evalScript` function available to the harness, which evaluates in a way more closely matching that of script tags.

Signed-off-by: Till Schneidereit <[email protected]>
@tschneidereit
Copy link
Member Author

@andreiltd this should hopefully make the tests we talked about in #200 work

@andreiltd
Copy link
Contributor

Hey @tschneidereit. That's a great improvement, thanks! I gave it go on the form-data branch and indeed the Not Defined error is gone. 🎉 However I'm getting this error when I try to run the tests:

1: WPT server stderr: 127.0.0.1 - - [09/Feb/2025 10:31:49] code 400, message Bad request syntax ('1D9')
1: 127.0.0.1 - - [09/Feb/2025 10:31:49] "1D9" 400 -

I will need to take a closer look if that's linked to FormData changes or something else although I can see the other tests are failing in a similar way. For example:

1: Running test fetch/api/redirect/redirect-upload.h2.any.js
1: Sending request to http://127.0.0.1:7676/fetch/api/redirect/redirect-upload.h2.any.js
1: WPT server stderr: 127.0.0.1 - - [09/Feb/2025 10:31:30] code 400, message Bad request syntax ('4')
1: 127.0.0.1 - - [09/Feb/2025 10:31:30] "4" 400 -

@andreiltd
Copy link
Contributor

I think I figured this out. The 400 error is thrown when I'm not setting the Content-Length header for the Request with ReadableStream body. After fixing the tests are passing 🎉 .

@tschneidereit
Copy link
Member Author

I talked to @guybedford, and while he doesn't have the bandwidth to do a proper review, he gave his okay to merge this as-is, given that @andreiltd gave positive feedback. This also only touches test code, nothing we ship.

@tschneidereit tschneidereit merged commit 17313a6 into main Feb 12, 2025
5 checks passed
@tschneidereit tschneidereit deleted the better-wpt branch February 12, 2025 18:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants