-
Notifications
You must be signed in to change notification settings - Fork 74
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
Add Async Runner for News Site Next Testing #442
base: main
Are you sure you want to change the base?
Changes from all commits
b594115
f154a4c
9459c00
d462764
d037f89
79f8450
14f1f6d
d08606d
4540937
a02d7cc
5281af4
d4fa570
764b61b
ace8c3b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,7 +24,24 @@ export class TimerTestInvoker extends TestInvoker { | |
} | ||
} | ||
|
||
export class RAFTestInvoker extends TestInvoker { | ||
class AsyncTimerTestInvoker extends TestInvoker { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we plan to use both the timer and RAF invoker for async? I had a sense that we kept the sync timer invoker around for backwards compat or non-default testing environments. I'm assuming the overall properties are quire different once we are async (potentially reducing utility of timer), and it's complex enough to reason about one implementation, so should we consider just having one and exposing it on both the default and non-default configuration? I'm open to having both but would like to find ways to reduce the amount of work if possible :). @camillobruni @smaug---- @rniwa thoughts? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd be fine to drop the setTimeout approach altogether since we haven't really discovered additional issues with it? |
||
start() { | ||
return new Promise((resolve) => { | ||
setTimeout(async () => { | ||
await this._syncCallback(); | ||
setTimeout(() => { | ||
this._asyncCallback(); | ||
requestAnimationFrame(async () => { | ||
await this._reportCallback(); | ||
resolve(); | ||
}); | ||
}, 0); | ||
}, this._params.waitBeforeSync); | ||
}); | ||
} | ||
} | ||
|
||
class BaseRAFTestInvoker extends TestInvoker { | ||
start() { | ||
return new Promise((resolve) => { | ||
if (this._params.waitBeforeSync) | ||
|
@@ -33,7 +50,9 @@ export class RAFTestInvoker extends TestInvoker { | |
this._scheduleCallbacks(resolve); | ||
}); | ||
} | ||
} | ||
|
||
class RAFTestInvoker extends BaseRAFTestInvoker { | ||
_scheduleCallbacks(resolve) { | ||
requestAnimationFrame(() => this._syncCallback()); | ||
requestAnimationFrame(() => { | ||
|
@@ -48,8 +67,31 @@ export class RAFTestInvoker extends TestInvoker { | |
} | ||
} | ||
|
||
class AsyncRAFTestInvoker extends BaseRAFTestInvoker { | ||
_scheduleCallbacks(resolve) { | ||
requestAnimationFrame(async () => { | ||
await this._syncCallback(); | ||
requestAnimationFrame(() => { | ||
setTimeout(() => { | ||
this._asyncCallback(); | ||
setTimeout(async () => { | ||
await this._reportCallback(); | ||
resolve(); | ||
}, 0); | ||
}, 0); | ||
}); | ||
}); | ||
} | ||
} | ||
|
||
export const TEST_INVOKER_LOOKUP = { | ||
__proto__: null, | ||
timer: TimerTestInvoker, | ||
raf: RAFTestInvoker, | ||
}; | ||
|
||
export const ASYNC_TEST_INVOKER_LOOKUP = { | ||
__proto__: null, | ||
timer: AsyncTimerTestInvoker, | ||
raf: AsyncRAFTestInvoker, | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
import { TEST_INVOKER_LOOKUP } from "./test-invoker.mjs"; | ||
import { TEST_INVOKER_LOOKUP, ASYNC_TEST_INVOKER_LOOKUP } from "./test-invoker.mjs"; | ||
|
||
export class TestRunner { | ||
#frame; | ||
|
@@ -18,6 +18,30 @@ export class TestRunner { | |
this.#frame = frame; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. let's not use private properties, they add needless overhead in the runner. The runner code should aim at maximum performance and minimal startup costs. Last I tested this is measurable in Firefox and Chrome. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we should restrict ourselves from using new ECMAScript features. With that argument, we should be going back to writing ES5 code without any classes. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If there is performance penalty attached to it for the test runner, then yes, we should restrict ourselves, we should aim at the lowest possible overhead there. Especially if it uses code that is not popular out in the wild. And I would even agree that we should transpile and minify to ES5 the runner code for a big part for performance reasons! It's a different story for workloads though, a good mix of old and new features is very welcome. |
||
|
||
get frame() { | ||
return this.#frame; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. since i'm extending and overriding the runTest function, I had to create getters for the private variables. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If there are now external consumers for these, should we just make them public? cc @rniwa There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm only using getters at the moment, so more boilerplate, but maybe more "secure" than just making them public? 🤷 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 to making the public and lower overhead. |
||
|
||
get page() { | ||
return this.#page; | ||
} | ||
|
||
get params() { | ||
return this.#params; | ||
} | ||
|
||
get suite() { | ||
return this.#suite; | ||
} | ||
|
||
get test() { | ||
return this.#test; | ||
} | ||
|
||
get callback() { | ||
return this.#callback; | ||
} | ||
|
||
async runTest() { | ||
// Prepare all mark labels outside the measuring loop. | ||
const suiteName = this.#suite.name; | ||
|
@@ -77,3 +101,74 @@ export class TestRunner { | |
return invoker.start(); | ||
} | ||
} | ||
|
||
export class AsyncTestRunner extends TestRunner { | ||
async runTest() { | ||
// Prepare all mark labels outside the measuring loop. | ||
const suiteName = this.suite.name; | ||
const testName = this.test.name; | ||
const syncStartLabel = `${suiteName}.${testName}-start`; | ||
const syncEndLabel = `${suiteName}.${testName}-sync-end`; | ||
const asyncStartLabel = `${suiteName}.${testName}-async-start`; | ||
const asyncEndLabel = `${suiteName}.${testName}-async-end`; | ||
|
||
let syncTime; | ||
let asyncStartTime; | ||
let asyncTime; | ||
|
||
const runSync = async () => { | ||
if (this.params.warmupBeforeSync) { | ||
performance.mark("warmup-start"); | ||
const startTime = performance.now(); | ||
// Infinite loop for the specified ms. | ||
while (performance.now() - startTime < this.params.warmupBeforeSync) | ||
continue; | ||
performance.mark("warmup-end"); | ||
} | ||
performance.mark(syncStartLabel); | ||
const syncStartTime = performance.now(); | ||
await this.test.run(this.page); | ||
const syncEndTime = performance.now(); | ||
performance.mark(syncEndLabel); | ||
|
||
syncTime = syncEndTime - syncStartTime; | ||
|
||
performance.mark(asyncStartLabel); | ||
asyncStartTime = performance.now(); | ||
}; | ||
const measureAsync = () => { | ||
const bodyReference = this.frame ? this.frame.contentDocument.body : document.body; | ||
const windowReference = this.frame ? this.frame.contentWindow : window; | ||
// Some browsers don't immediately update the layout for paint. | ||
// Force the layout here to ensure we're measuring the layout time. | ||
const height = bodyReference.getBoundingClientRect().height; | ||
windowReference._unusedHeightValue = height; // Prevent dead code elimination. | ||
|
||
const asyncEndTime = performance.now(); | ||
performance.mark(asyncEndLabel); | ||
|
||
asyncTime = asyncEndTime - asyncStartTime; | ||
|
||
if (this.params.warmupBeforeSync) | ||
performance.measure("warmup", "warmup-start", "warmup-end"); | ||
performance.measure(`${suiteName}.${testName}-sync`, syncStartLabel, syncEndLabel); | ||
performance.measure(`${suiteName}.${testName}-async`, asyncStartLabel, asyncEndLabel); | ||
}; | ||
|
||
const report = () => this.callback(this.test, syncTime, asyncTime); | ||
const invokerClass = ASYNC_TEST_INVOKER_LOOKUP[this.params.measurementMethod]; | ||
const invoker = new invokerClass(runSync, measureAsync, report, this.params); | ||
|
||
return invoker.start(); | ||
} | ||
} | ||
|
||
// FIXME: implement remote steps | ||
class RemoteTestRunner extends TestRunner {} | ||
|
||
export const TEST_RUNNER_LOOKUP = { | ||
__proto__: null, | ||
default: TestRunner, | ||
async: AsyncTestRunner, | ||
remote: RemoteTestRunner, | ||
}; |
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.
no need to create a new SuiteRunner, since only the TestRunner is different and that's dynamically selected.