-
Notifications
You must be signed in to change notification settings - Fork 296
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
POC for basic web platform test integration #2585
base: main
Are you sure you want to change the base?
Conversation
2a42bf6
to
de0330d
Compare
allow_empty = True, | ||
), | ||
visibility = ["//visibility:public"], | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We would need to export filegroups for each of the web platform test groups we want... we don't need to export filegroups for everything since most of the wpts will never be relevant
src/workerd/api/wpt/BUILD.bazel
Outdated
], | ||
) for f in glob( | ||
["**/*.wd-test"], | ||
)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most likely should list each individually rather than using the glob here so that we can selectively import the appropriate wpt filegroups.
src/workerd/api/wpt/url-test.js
Outdated
globalThis.fetch = async(url) => { | ||
const { default: data } = await import(url); | ||
return { | ||
async json() { return data; }, | ||
}; | ||
}; | ||
|
||
globalThis.promise_test = (callback) => { | ||
callback(); | ||
}; | ||
|
||
globalThis.assert_equals = (a, b, c) => { | ||
strictEqual(a,b,c); | ||
}; | ||
|
||
globalThis.test = (callback, message) => { | ||
try { | ||
callback(); | ||
} catch (err) { | ||
const aerr = new AggregateError([err], message); | ||
globalThis.errors.push(aerr); | ||
} | ||
} | ||
|
||
globalThis.errors = []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In order to be able to run the tests we need to define the test harness methods along with a way of collecting the results. This is a bit wonky here.
src/workerd/api/wpt/url-test.js
Outdated
|
||
export const test = { | ||
async test(_, env) { | ||
const foo = await import('url-origin.any.js'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests end up running when the module is evaluated. We have to set the global test harness methods before the dynamic import, then check the globalThis.errors
when we're done. The globalThis.errors
is only specific to this test file, it is not actually part of the WPT test harness. We have to do this because the tests themselves may not actually propagate the error (in the case of url-origin.any.js we end up with a swallowed rejected promise) if we don't have this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also need a way to ignore certain errors. Possibly from a module_name.json
file with a JSON schema that conforms to our needs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'm thinking that can be a separate json module that is imported and used by the harness.
7c954b5
to
e5803d7
Compare
src/workerd/api/wpt/BUILD.bazel
Outdated
load("//:build/wd_test.bzl", "wd_test") | ||
load("//src/workerd/api/wpt:generate-tests.bzl", "gen_wpt_tests") | ||
|
||
gen_wpt_tests(glob(["**/*.js"])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some documentation comments around this would be helpful.
async test() { | ||
harness.prepare(); | ||
await import('url-constructor.any.js'); | ||
harness.validate(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Side note: Our workerd tests could benefit from having a before() { ... }
and after() { ... }
mechanism.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose we could have a pattern like...
import * as harness from 'harness';
export const urlConstructor = {
test: harness.test('url-constructor.any.js');
};
// or
export const urlConstructor = harness.test('url-constructor.any.js');
Where the harness.test(...)
handles any of the necessary boilerplate for the most typical cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah sounds like a good idea
@@ -0,0 +1,41 @@ | |||
import { strictEqual } from 'node:assert'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Side note... let's try not to forget adding the copyright headers to these (speaking to myself here mostly)
9d9af02
to
37c95b4
Compare
83f80f2
to
26ec3fc
Compare
test_name = ctx.attr.test_name, | ||
test_js = wd_relative_path(ctx.file.test_js), | ||
modules = generate_external_modules(ctx.attr.wpt_directory.files), | ||
date = "2024-10-10", # FIXME: we don't wanna be hardcoding that type of shizz |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be 2024-10-11
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we hardcode this value or will it need to be updated regularly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It cannot be hardcoded since we might introduce a change that is only available after a certain date.
This is just a proof of concept to start working through what is necessary to integrate web platform tests into wd-tests.
There are some limitations to this approach...
ShadowRealm
... otherwise there's always a possibility of the harness conflicting with the global scope, tho that is unlikely in practice