-
Notifications
You must be signed in to change notification settings - Fork 76
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
Async Runner with Message Channel #458
Async Runner with Message Channel #458
Conversation
fd6dd12
to
110c94a
Compare
@rniwa - I believe we addressed all the comments. Anything else missing that I can help with? |
resources/shared/test-invoker.mjs
Outdated
export const TEST_INVOKER_LOOKUP = { | ||
__proto__: null, | ||
raf: RAFTestInvoker, | ||
}; | ||
|
||
export const ASYNC_TEST_INVOKER_LOOKUP = { |
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.
Do we really need a separate lookup table? Now that timer-based measurement is gone,
why don't we just have a single lookup table with "raf" and "async" as measurement methods.
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.
cleaned up!
resources/shared/test-runner.mjs
Outdated
@@ -41,7 +52,7 @@ export class TestRunner { | |||
} | |||
performance.mark(syncStartLabel); | |||
const syncStartTime = performance.now(); | |||
this.#test.run(this.#page); | |||
await this._runSyncStep(this.test, this.page); |
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 seems to have a side effect of always pushing the measurement end until when a promise resolves.
I don't think we want to do that for rAF based measurement method (since that can affect the score)
so maybe we should check the return value of this function and await conditionally?
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.
More concretely, the following code logs: 1, 2, 3:
(async function() { console.log(1); await (() => { })(); console.log(3); })(); console.log(2);
whereas the following code logs 1, 3, 2:
(async function() { console.log(1); (() => { })(); console.log(3); })(); console.log(2);
i.e. await
will always delay the execution of the continuation until the end of the current micro-task. While that's highly desirable for async measurement, I don't think we want to affect rAF measurement.
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 made it a bit simpler and just passing along a type that we can check to see if we need to await or not
cleaned up some stuff from the developer menu integration. Previously i only checked if the suite.type is "async", which always returned false, since we removed the async type from all tests. Now I am checking both: suite.type === "async" or "params.useAsyncSteps" |
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.
Great. Thank you for addressing all the review comments!
As discussed in our sync, here is a combination of the AsyncRunner and Message Channel pr.
Relevant code snippet (udpated):
For testing purposes I kept an explicit type of "async" in the test file, but I opted all default suites into using it.
Initial testing shows that it should also fix the react specific issue.
Other issue solved by this pr: #83