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.

Sign up for GitHub

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

Jump to bottom

tests: smoke request count assertion #12325

Merged
merged 10 commits into from
Apr 15, 2021
Merged

tests: smoke request count assertion #12325

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
abf7245
tests: smoke request count assertion
adamraine Apr 2, 2021
b4ddf5a
change
adamraine Apr 6, 2021
3fd2d05
Update lighthouse-cli/test/smokehouse/report-assert.js
adamraine Apr 7, 2021
5e71ba8
only serial
adamraine Apr 9, 2021
f42b2f4
review
adamraine Apr 13, 2021
e578427
nits
adamraine Apr 13, 2021
f1b4ab3
Merge branch 'master' into smoke-request-assertion
adamraine Apr 13, 2021
46ea590
require
adamraine Apr 13, 2021
1096452
add dbw
adamraine Apr 13, 2021
9bb6c76
comments
adamraine Apr 15, 2021
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
26 changes: 26 additions & 0 deletions lighthouse-cli/test/fixtures/static-server.js
View file
Open in desktop
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,32 @@ class Server {
this._dataTransformer = fn;
}

clearRequestUrls() {
return this._requestUrls = [];
}

/**
* @return {string[]}
*/
getRequestUrls() {
return this._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
12 changes: 12 additions & 0 deletions lighthouse-cli/test/smokehouse/report-assert.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
const cloneDeep = require('lodash.clonedeep');
const log = require('lighthouse-logger');
const LocalConsole = require('./lib/local-console.js');
const {server} = require('../fixtures/static-server.js');

const NUMBER_REGEXP = /(?:\d|\.)+/.source;
const OPS_REGEXP = /<=?|>=?|\+\/-|±/.source;
Expand Down Expand Up @@ -254,6 +255,16 @@ function collateResults(localConsole, actual, expected) {
return makeComparison(auditName + ' audit', actualResult, expectedResult);
});

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

@brendankenny brendankenny Apr 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

it seems like this should be run outside of the assert module and the result passed in, like the other parts of actual. smokehouse.js is a natural place to do that, but that wouldn't work for the bundled version run for smokerider (which doesn't use static-server). Almost needs to be part of SmokehouseOptions, similar to the lighthouseRunner callback?

(I don't love that idea, but I'm not sure of a better one)

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 •
edited
Loading

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 •
edited
Loading

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:

lighthouse/lighthouse-cli/test/smokehouse/smokehouse.js

Lines 55 to 56 in 802947d

// 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 •
edited
Loading

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 +274,7 @@ function collateResults(localConsole, actual, expected) {
},
runtimeErrorAssertion,
runWarningsAssertion,
...requestCountAssertion,
...artifactAssertions,
...auditAssertions,
];
Expand Down
25 changes: 17 additions & 8 deletions lighthouse-cli/test/smokehouse/smokehouse.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ const cliLighthouseRunner = require('./lighthouse-runners/cli.js').runLighthouse
const getAssertionReport = require('./report-assert.js');
const LocalConsole = require('./lib/local-console.js');
const ConcurrentMapper = require('./lib/concurrent-mapper.js');
const {server} = require('../fixtures/static-server.js');

/* eslint-disable no-console */

Expand Down Expand Up @@ -117,14 +118,19 @@ async function runSmokeTestDefn(concurrentMapper, smokeTestDefn, defnOptions) {
const {id, config: configJson, expectations} = smokeTestDefn;
const {concurrency, lighthouseRunner, retries, isDebug} = defnOptions;

const individualTests = expectations.map(expectation => ({
requestedUrl: expectation.lhr.requestedUrl,
configJson,
expectation,
lighthouseRunner,
retries,
isDebug,
}));
const individualTests = expectations.map(expectation => {
if (expectation.networkRequests && concurrency > 1) {
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 we still want to be able to run in parallel locally, at least. Maybe move this check to pruneExpectations() in report-assert.js and remove expectation.networkRequests if !process.env.CI && concurrency > 1 (to enforce doing the extra checks in CI), otherwise throw this error?

Copy link
Member Author

Choose a reason for hiding this comment

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

Is there a way to know concurrency in pruneExpectations?

Copy link
Member

Choose a reason for hiding this comment

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

no, it would have to be passed down in reportOptions (either directly or as a shouldTestNetworkRequests or whatever)

throw new Error(`Test must be run serially to assert network requests (${id})`);
}
return {
requestedUrl: expectation.lhr.requestedUrl,
configJson,
expectation,
lighthouseRunner,
retries,
isDebug,
};
});

// Loop sequentially over expectations, comparing against Lighthouse run, and
// reporting result.
Expand Down Expand Up @@ -190,6 +196,9 @@ async function runSmokeTest(testOptions) {
localConsole.log(`Retrying run (${i} out of ${retries} retries)...`);
}

// Ensure server only records URLs fetched for this run.
server.clearRequestUrls();
adamraine marked this conversation as resolved.
Show resolved Hide resolved

// Run Lighthouse.
try {
result = await lighthouseRunner(requestedUrl, configJson, {isDebug});
Expand Down
15 changes: 15 additions & 0 deletions lighthouse-cli/test/smokehouse/test-definitions/perf/expectations.js
View file
Open in desktop
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
1 change: 1 addition & 0 deletions types/smokehouse.d.ts
View file
Open in desktop
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?: any;
Copy link
Member

Choose a reason for hiding this comment

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

the big question here...what do we actually want to assert about these? :)

Copy link
Member Author

Choose a reason for hiding this comment

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

From the issue, it sounds like the best thing to assert is the number of requests.

If we just did networkRequestCount or something, there would be no way to add a _minChromiumVersion for specific network request assertions.

Copy link
Member

Choose a reason for hiding this comment

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

ah, got it. Maybe networkRequests?: Array<unknown> then to make it clearer but we won't have to bother typing the requests saved from static-server?

Or could do networkRequests?: {length: number} if we wanted to be explicit about what we expect from test authors right now.

Both options would be forward compatible if for some reason we wanted to move to specific object shapes later (networkRequest?: Array<{url: string}> or whatever).

}

export interface TestDfn {
Expand Down