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

Add Async Runner for News Site Next Testing #442

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
7 changes: 4 additions & 3 deletions resources/suite-runner.mjs
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import { TestRunner } from "./test-runner.mjs";
import { TEST_RUNNER_LOOKUP } from "./test-runner.mjs";
import { WarmupSuite } from "./benchmark-runner.mjs";

// FIXME: Create AsyncSuiteRunner subclass.
// FIXME: Create RemoteSuiteRunner subclass.
export class SuiteRunner {
#frame;
Expand Down Expand Up @@ -53,7 +52,8 @@ export class SuiteRunner {
if (this.#client?.willRunTest)
await this.#client.willRunTest(this.#suite, test);

const testRunner = new TestRunner(this.#frame, this.#page, this.#params, this.#suite, test, this._recordTestResults);
const testRunnerClass = TEST_RUNNER_LOOKUP[this.#suite.type ?? "default"];
const testRunner = new testRunnerClass(this.#frame, this.#page, this.#params, this.#suite, test, this._recordTestResults);
await testRunner.runTest();
}
performance.mark(suiteEndLabel);
Expand Down Expand Up @@ -100,5 +100,6 @@ class RemoteSuiteRunner extends SuiteRunner {}
export const SUITE_RUNNER_LOOKUP = {
__proto__: null,
default: SuiteRunner,
async: SuiteRunner,
remote: RemoteSuiteRunner,
Copy link
Contributor Author

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.

};
44 changes: 43 additions & 1 deletion resources/test-invoker.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,24 @@ export class TimerTestInvoker extends TestInvoker {
}
}

export class RAFTestInvoker extends TestInvoker {
class AsyncTimerTestInvoker extends TestInvoker {
Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Contributor

Choose a reason for hiding this comment

The 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)
Expand All @@ -33,7 +50,9 @@ export class RAFTestInvoker extends TestInvoker {
this._scheduleCallbacks(resolve);
});
}
}

class RAFTestInvoker extends BaseRAFTestInvoker {
_scheduleCallbacks(resolve) {
requestAnimationFrame(() => this._syncCallback());
requestAnimationFrame(() => {
Expand All @@ -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,
};
97 changes: 96 additions & 1 deletion resources/test-runner.mjs
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;
Expand All @@ -18,6 +18,30 @@ export class TestRunner {
this.#frame = frame;
}
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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.
Note: we did not include the latest framework versions based on this groups feedback... so there is precedence for going equally the other way.


get frame() {
return this.#frame;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.
just more boilerplate..

Copy link
Contributor

Choose a reason for hiding this comment

The 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

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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? 🤷

Copy link
Contributor

Choose a reason for hiding this comment

The 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;
Expand Down Expand Up @@ -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,
};
43 changes: 43 additions & 0 deletions resources/tests.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -894,6 +894,49 @@ Suites.push({
],
});

Suites.push({
name: "NewsSite-Next-Async",
url: "resources/newssite/news-next/dist/index.html#/home",
tags: ["experimental", "newssite", "language"],
type: "async",
disabled: true,
async prepare(page) {
await page.waitForElement("#navbar-dropdown-toggle");
},
tests: [
new BenchmarkTestStep("NavigateToUS", async (page) => {
for (let i = 0; i < 25; i++) {
page.querySelector("#navbar-dropdown-toggle").click();
page.layout();
page.querySelector("#navbar-dropdown-toggle").click();
page.layout();
}
page.querySelector("#navbar-navlist-us-link").click();
page.layout();
}),
new BenchmarkTestStep("NavigateToWorld", async (page) => {
for (let i = 0; i < 25; i++) {
page.querySelector("#navbar-dropdown-toggle").click();
page.layout();
page.querySelector("#navbar-dropdown-toggle").click();
page.layout();
}
page.querySelector("#navbar-navlist-world-link").click();
page.layout();
}),
new BenchmarkTestStep("NavigateToPolitics", async (page) => {
for (let i = 0; i < 25; i++) {
page.querySelector("#navbar-dropdown-toggle").click();
page.layout();
page.querySelector("#navbar-dropdown-toggle").click();
page.layout();
}
page.querySelector("#navbar-navlist-politics-link").click();
page.layout();
}),
],
});

Suites.push({
name: "NewsSite-Nuxt",
url: "resources/newssite/news-nuxt/dist/index.html",
Expand Down
Loading