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

WPT: Run tests with unsafeEval so they can use IOContext #3353

Merged
merged 1 commit into from
Jan 20, 2025

Conversation

npaun
Copy link
Member

@npaun npaun commented Jan 17, 2025

WPT tests are now declared as text bindings and executed with unsafeEval. This way they can do IOContext things.
This PR also fixes promise_test to store each promise created and await them all before evaluating the test results.

@npaun npaun requested review from jasnell and anonrig January 17, 2025 20:25
@npaun npaun requested review from a team as code owners January 17, 2025 20:25
src/wpt/harness.ts Show resolved Hide resolved
src/wpt/harness.ts Outdated Show resolved Hide resolved
@anonrig
Copy link
Member

anonrig commented Jan 17, 2025

What does this PR does that we couldn't do in the past? Can you add those changes to this pull-request as well. It's not possible to justify these changes without an example/test that comes with it.

@jasnell
Copy link
Member

jasnell commented Jan 17, 2025

Many of the wpt test files rely on the need to perform io at the top level scope, as you know we want to be able to run those wpt files automatically as is, without porting those to workerd syntax. This PR is the solution @npaun and I came up with to support the case. We cannot use regular eval since that is disabled, and we cannot use require(...) since those run outside of the IoContext... turns out just adding these scripts as text bindings and passing them to the unsafe eval binding makes it work.

@npaun npaun force-pushed the npaun/wpt-unsafe-eval branch from 0e9d656 to e15ea7c Compare January 17, 2025 23:45
@anonrig
Copy link
Member

anonrig commented Jan 18, 2025

Many of the wpt test files rely on the need to perform io at the top level scope, as you know we want to be able to run those wpt files automatically as is, without porting those to workerd syntax. This PR is the solution @npaun and I came up with to support the case. We cannot use regular eval since that is disabled, and we cannot use require(...) since those run outside of the IoContext... turns out just adding these scripts as text bindings and passing them to the unsafe eval binding makes it work.

Yes, but this PR doesn't add any of those new tests. It would be helpful if we at least enabled 1 test that uses this new code.

@npaun
Copy link
Member Author

npaun commented Jan 20, 2025

It's not a particularly awesome demo but there is a simple way to check this change is working.

Whereas before urlencoded-parser.any.js's Response tests would fail with:

Error: Disallowed operation called within global scope. Asynchronous I/O (ex: fetch() or connect()), setting a timeout, and generating random values are not allowed within global scope. To fix this error, perform this operation within a handler. https://developers.cloudflare.com/workers/runtime-apis/handlers/

Now they fail with:
TypeError: Non-utf-8 application/x-www-form-urlencoded body.

Which shows we can now run that part of the test code.

@npaun npaun merged commit 5e538f6 into main Jan 20, 2025
15 checks passed
@npaun npaun deleted the npaun/wpt-unsafe-eval branch January 20, 2025 05:33
@anonrig
Copy link
Member

anonrig commented Jan 20, 2025

Next time please add a test before merging that asserts the intended behavior.

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.

3 participants