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

tests: smoke request count assertion #12325

Merged
merged 10 commits into from
Apr 15, 2021
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions lighthouse-cli/test/fixtures/static-server.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ class Server {
this._server = http.createServer(this._requestHandler.bind(this));
/** @type {(data: string) => string=} */
this._dataTransformer = undefined;
/** @type {string[]} */
this._requestUrls = [];
}

getPort() {
Expand Down Expand Up @@ -62,8 +64,30 @@ class Server {
this._dataTransformer = fn;
}

/**
* @return {string[]}
*/
takeRequestUrls() {
const requestUrls = this._requestUrls;
this._requestUrls = [];
return requestUrls;
}

/**
* @param {http.IncomingMessage} request
*/
_updateRequestUrls(request) {
if (['/favicon.ico', '/robots.txt'].includes(request.url)) return;
adamraine marked this conversation as resolved.
Show resolved Hide resolved
this._requestUrls.push(request.url);
}

/**
* @param {http.IncomingMessage} request
* @param {http.ServerResponse} response
*/
_requestHandler(request, response) {
const requestUrl = parseURL(request.url);
this._updateRequestUrls(request);
const filePath = requestUrl.pathname;
const queryString = requestUrl.search && parseQueryString(requestUrl.search.slice(1));
let absoluteFilePath = path.join(this.baseDir, filePath);
Expand Down
6 changes: 5 additions & 1 deletion lighthouse-cli/test/smokehouse/frontends/smokehouse-bin.js
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,11 @@ async function begin() {
const invertMatch = argv.invertMatch;
const testDefns = getDefinitionsToRun(allTestDefns, requestedTestIds, {invertMatch});

const options = {jobs, retries, isDebug: argv.debug, lighthouseRunner};
const takeNetworkRequestUrls = () => {
return server.takeRequestUrls();
};

const options = {jobs, retries, isDebug: argv.debug, lighthouseRunner, takeNetworkRequestUrls};

let isPassing;
try {
Expand Down
34 changes: 28 additions & 6 deletions lighthouse-cli/test/smokehouse/report-assert.js
Original file line number Diff line number Diff line change
Expand Up @@ -157,13 +157,13 @@ function makeComparison(name, actualResult, expectedResult) {
* @param {LocalConsole} localConsole
* @param {LH.Result} lhr
* @param {Smokehouse.ExpectedRunnerResult} expected
* @param {boolean|undefined} isSync
*/
function pruneExpectations(localConsole, lhr, expected) {
function pruneExpectations(localConsole, lhr, expected, isSync) {
const userAgent = lhr.environment.hostUserAgent;
const userAgentMatch = /Chrome\/(\d+)/.exec(userAgent); // Chrome/85.0.4174.0
if (!userAgentMatch) throw new Error('Could not get chrome version.');
const actualChromeVersion = Number(userAgentMatch[1]);

/**
* @param {*} obj
*/
Expand Down Expand Up @@ -199,14 +199,25 @@ function pruneExpectations(localConsole, lhr, expected) {
}

const cloned = cloneDeep(expected);

if (!isSync && expected.networkRequests) {
adamraine marked this conversation as resolved.
Show resolved Hide resolved
const msg = 'Network requests should only be asserted on tests run synchronously';
if (process.env.CI) {
throw new Error(msg);
} else {
localConsole.log(`${msg}, pruning expectation: ${JSON.stringify(expected.networkRequests)}`);
delete cloned.networkRequests;
}
}

pruneNewerChromeExpectations(cloned);
return cloned;
}

/**
* Collate results into comparisons of actual and expected scores on each audit/artifact.
* @param {LocalConsole} localConsole
* @param {{lhr: LH.Result, artifacts: LH.Artifacts}} actual
* @param {{lhr: LH.Result, artifacts: LH.Artifacts, networkRequests: string[]}} actual
* @param {Smokehouse.ExpectedRunnerResult} expected
* @return {Comparison[]}
*/
Expand Down Expand Up @@ -254,6 +265,16 @@ function collateResults(localConsole, actual, expected) {
return makeComparison(auditName + ' audit', actualResult, expectedResult);
});

/** @type {Comparison[]} */
const requestCountAssertion = [];
if (expected.networkRequests) {
requestCountAssertion.push(makeComparison(
'Requests',
actual.networkRequests,
expected.networkRequests
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think it'd be good to exercise the actual assertions within the PR.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added some expectations to perf.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added some expectations to perf.

dbw might be one of the best to add expectations to since the page tries to be bad at everything and the smoke test runs the full default config. It's also just the one smoke test, so shouldn't be much work to add.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dbw is two smoke tests run in parallel, one smoke test is an external URL.

The smoke test should still work if we adjust our requirement from "Tests run synchronously" to "Only one test using the static server at a time". Definitely doable, but it might add a bit of complexity.

Copy link
Member

@brendankenny brendankenny Apr 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yeah, I meant just the first one anyways. Agreed it's not worth the complexity (at least for now) to add networkRequests to the second one

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is unfortunately still a problem. Under the current implementation, the expectation would be pruned/throw because dbw is run in parallel. We need to change the definition (adding complexity) for the expectation to be actually be asserted.

Copy link
Member

@brendankenny brendankenny Apr 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I see, you're talking about runSerially so we've been talking about slightly different things this whole time, sorry :/

You have the isSync value based on concurrency, which is the combination of runSerially with the --jobs flag/option:

// If defn is set to `runSerially`, we'll run its tests in succession, not parallel.
const concurrency = testDefn.runSerially ? 1 : jobs;

so it'll be pruned in a the usual local runs, but in CI we run with -j=1 for vm performance reasons (and maybe still coverage collection issues?) so concurrency will still be 1 and it shouldn't be pruned there.

Copy link
Member

@brendankenny brendankenny Apr 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that I write that, maybe it'll be annoying when editing dbw to only find out about failures in CI unless you happen to run with -j=1 🤷

If ever there was a smoke test perfect for #11506 it would be dbw, though. Should we just runSerially: true it and take the hit on slower smoke tests but keep things simpler?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we just runSerially: true it and take the hit on slower smoke tests but keep things simpler?

That's what I'm thinking, it shouldn't be too bad because there are only two tests.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just added dbw.

));
}

return [
{
name: 'final url',
Expand All @@ -263,6 +284,7 @@ function collateResults(localConsole, actual, expected) {
},
runtimeErrorAssertion,
runWarningsAssertion,
...requestCountAssertion,
...artifactAssertions,
...auditAssertions,
];
Expand Down Expand Up @@ -334,15 +356,15 @@ function assertLogString(count) {
/**
* Log all the comparisons between actual and expected test results, then print
* summary. Returns count of passed and failed tests.
* @param {{lhr: LH.Result, artifacts: LH.Artifacts}} actual
* @param {{lhr: LH.Result, artifacts: LH.Artifacts, networkRequests: string[]}} actual
* @param {Smokehouse.ExpectedRunnerResult} expected
* @param {{isDebug?: boolean}=} reportOptions
* @param {{isDebug?: boolean, isSync?: boolean}=} reportOptions
* @return {{passed: number, failed: number, log: string}}
*/
function report(actual, expected, reportOptions = {}) {
const localConsole = new LocalConsole();

expected = pruneExpectations(localConsole, actual.lhr, expected);
expected = pruneExpectations(localConsole, actual.lhr, expected, reportOptions.isSync);
const comparisons = collateResults(localConsole, actual, expected);

let correctCount = 0;
Expand Down
47 changes: 32 additions & 15 deletions lighthouse-cli/test/smokehouse/smokehouse.js
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ async function runSmokehouse(smokeTestDefns, smokehouseOptions = {}) {
jobs = DEFAULT_CONCURRENT_RUNS,
retries = DEFAULT_RETRIES,
lighthouseRunner = cliLighthouseRunner,
takeNetworkRequestUrls = () => [],
} = smokehouseOptions;
assertPositiveInteger('jobs', jobs);
assertNonNegativeInteger('retries', retries);
Expand All @@ -54,7 +55,7 @@ async function runSmokehouse(smokeTestDefns, smokehouseOptions = {}) {
for (const testDefn of smokeTestDefns) {
// If defn is set to `runSerially`, we'll run its tests in succession, not parallel.
const concurrency = testDefn.runSerially ? 1 : jobs;
const options = {concurrency, lighthouseRunner, retries, isDebug};
const options = {concurrency, lighthouseRunner, retries, isDebug, takeNetworkRequestUrls};
const result = runSmokeTestDefn(concurrentMapper, testDefn, options);
smokePromises.push(result);
}
Expand Down Expand Up @@ -110,21 +111,25 @@ function assertNonNegativeInteger(loggableName, value) {
* once all are finished.
* @param {ConcurrentMapper} concurrentMapper
* @param {Smokehouse.TestDfn} smokeTestDefn
* @param {{concurrency: number, retries: number, lighthouseRunner: Smokehouse.LighthouseRunner, isDebug?: boolean}} defnOptions
* @param {{concurrency: number, retries: number, lighthouseRunner: Smokehouse.LighthouseRunner, isDebug?: boolean, takeNetworkRequestUrls: () => string[]}} defnOptions
* @return {Promise<SmokehouseResult>}
*/
async function runSmokeTestDefn(concurrentMapper, smokeTestDefn, defnOptions) {
const {id, config: configJson, expectations} = smokeTestDefn;
const {concurrency, lighthouseRunner, retries, isDebug} = defnOptions;
const {concurrency, lighthouseRunner, retries, isDebug, takeNetworkRequestUrls} = defnOptions;

const individualTests = expectations.map(expectation => ({
requestedUrl: expectation.lhr.requestedUrl,
configJson,
expectation,
lighthouseRunner,
retries,
isDebug,
}));
const individualTests = expectations.map(expectation => {
return {
requestedUrl: expectation.lhr.requestedUrl,
configJson,
expectation,
lighthouseRunner,
retries,
isDebug,
isSync: concurrency === 1,
takeNetworkRequestUrls,
};
});

// Loop sequentially over expectations, comparing against Lighthouse run, and
// reporting result.
Expand Down Expand Up @@ -171,14 +176,23 @@ function purpleify(str) {
/**
* Run Lighthouse in the selected runner. Returns `log`` for logging once
* all tests in a defn are complete.
* @param {{requestedUrl: string, configJson?: LH.Config.Json, expectation: Smokehouse.ExpectedRunnerResult, lighthouseRunner: Smokehouse.LighthouseRunner, retries: number, isDebug?: boolean}} testOptions
* @param {{requestedUrl: string, configJson?: LH.Config.Json, expectation: Smokehouse.ExpectedRunnerResult, lighthouseRunner: Smokehouse.LighthouseRunner, retries: number, isDebug?: boolean, isSync?: boolean, takeNetworkRequestUrls: () => string[]}} testOptions
* @return {Promise<{passed: number, failed: number, log: string}>}
*/
async function runSmokeTest(testOptions) {
// Use a buffered LocalConsole to keep logged output so it's not interleaved
// with other currently running tests.
const localConsole = new LocalConsole();
const {requestedUrl, configJson, expectation, lighthouseRunner, retries, isDebug} = testOptions;
const {
requestedUrl,
configJson,
expectation,
lighthouseRunner,
retries,
isDebug,
isSync,
takeNetworkRequestUrls,
} = testOptions;

// Rerun test until there's a passing result or retries are exhausted to prevent flakes.
let result;
Expand All @@ -192,14 +206,17 @@ async function runSmokeTest(testOptions) {

// Run Lighthouse.
try {
result = await lighthouseRunner(requestedUrl, configJson, {isDebug});
result = {
...await lighthouseRunner(requestedUrl, configJson, {isDebug}),
...{networkRequests: takeNetworkRequestUrls()},
adamraine marked this conversation as resolved.
Show resolved Hide resolved
};
} catch (e) {
logChildProcessError(localConsole, e);
continue; // Retry, if possible.
}

// Assert result.
report = getAssertionReport(result, expectation, {isDebug});
report = getAssertionReport(result, expectation, {isDebug, isSync});
if (report.failed) {
localConsole.log(`${report.failed} assertion(s) failed.`);
continue; // Retry, if possible.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,9 @@
*/
module.exports = [
{
networkRequests: {
length: 9,
},
lhr: {
requestedUrl: 'http://localhost:10200/preload.html',
finalUrl: 'http://localhost:10200/preload.html',
Expand Down Expand Up @@ -62,6 +65,9 @@ module.exports = [
},
},
{
networkRequests: {
length: 8,
},
lhr: {
requestedUrl: 'http://localhost:10200/perf/perf-budgets/load-things.html',
finalUrl: 'http://localhost:10200/perf/perf-budgets/load-things.html',
Expand Down Expand Up @@ -140,6 +146,9 @@ module.exports = [
},
},
{
networkRequests: {
length: 5,
},
lhr: {
requestedUrl: 'http://localhost:10200/perf/fonts.html',
finalUrl: 'http://localhost:10200/perf/fonts.html',
Expand Down Expand Up @@ -168,6 +177,9 @@ module.exports = [
},
},
{
networkRequests: {
length: 3,
},
artifacts: {
TraceElements: [
{
Expand Down Expand Up @@ -291,6 +303,9 @@ module.exports = [
},
},
{
networkRequests: {
length: 2,
},
lhr: {
requestedUrl: 'http://localhost:10200/perf/frame-metrics.html',
finalUrl: 'http://localhost:10200/perf/frame-metrics.html',
Expand Down
3 changes: 3 additions & 0 deletions types/smokehouse.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
export type ExpectedRunnerResult = {
lhr: ExpectedLHR,
artifacts?: Partial<Record<keyof LH.Artifacts, any>>
networkRequests?: {length: number};
}

export interface TestDfn {
Expand All @@ -41,6 +42,8 @@
retries?: number;
/** A function that runs Lighthouse with the given options. Defaults to running Lighthouse via the CLI. */
lighthouseRunner?: LighthouseRunner;
/** A function that gets a list of URLs requested to the server since the last fetch. */
takeNetworkRequestUrls?: () => string[];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to be required? It's different than the other options, but if it's not provided then the perf smoke tests will always fail.

Another option would be to prune networkRequests if no takeNetworkRequestUrls is provided, but that is slightly more prone to accidentally not passing it in and the tests silently accepting that and passing anyways

A big question would be if @connorjclark feels like getting the network requests is doable with smokerider. If yes then required seems good. If not, we could go either way (e.g. it could still be required but smokerider could pass in a no-op function and prune out the networkRequests itself in the lib.js frontend).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

currently for smokerider the static server is run in a separate process, so takeNetworkRequestUrls wouldn't work. looking at the code again I see no reason for it being a different process, so should be possible to make this work.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made it required for now.

}

export interface SmokehouseLibOptions extends SmokehouseOptions {
Expand Down