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

core: include finished state on hidden network-requests audit #10530

Merged
merged 3 commits into from
Apr 1, 2020
Merged

core: include finished state on hidden network-requests audit #10530

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
52531e9
core: include finished state on hidden network-requests audit
brendankenny Mar 31, 2020
4d7dfe1
replace old test with new one
brendankenny Mar 31, 2020
7267eb8
false
brendankenny Mar 31, 2020
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
4 changes: 4 additions & 0 deletions lighthouse-cli/test/smokehouse/test-definitions/byte-efficiency/expectations.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,19 +235,23 @@ const expectations = [
items: [
{
url: 'http://localhost:10200/byte-efficiency/gzip.html',
finished: true,
},
{
url: 'http://localhost:10200/byte-efficiency/script.js?gzip=1',
transferSize: '1100 +/- 100',
resourceSize: '53000 +/- 1000',
finished: true,
},
{
url: 'http://localhost:10200/byte-efficiency/script.js',
transferSize: '53200 +/- 1000',
resourceSize: '53000 +/- 1000',
finished: true,
},
{
url: 'http://localhost:10200/favicon.ico',
finished: true,
Copy link
Member Author

@brendankenny brendankenny Mar 31, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

it's nice to have smoke coverage, but clearly we're not too worried about covering this audit's entire output given the assertions here (and two other places where we assert just details.items.length), so this could be removed if it feels excessive

Copy link
Collaborator

Choose a reason for hiding this comment

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

I like the idea of asserting these somewhere so seems fine 🤷‍♂

},
],
},
Expand Down
4 changes: 3 additions & 1 deletion lighthouse-core/audits/network-requests.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ class NetworkRequests extends Audit {
url: URL.elideDataURI(record.url),
startTime: timeToMs(record.startTime),
endTime: timeToMs(record.endTime),
finished: record.finished,
transferSize: record.transferSize,
resourceSize: record.resourceSize,
statusCode: record.statusCode,
Expand All @@ -62,6 +63,7 @@ class NetworkRequests extends Audit {
};
});

// NOTE(i18n): this audit is only for debug info in the LHR and does not appear in the report.
Copy link
Member Author

@brendankenny brendankenny Mar 31, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

the other options are:

  • actually i18n this file. Many of these column names already exist in our strings, but some ('Status Code', 'MIME Type', etc) do not and will have to be translated. Seems not worth it?
  • remove the headings and make this debugdata, which it really already is. Seems like there's benefit to having it be the common table format, though, and the headings kind of serve as documentation for the columns?

I'm fine with any of these.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I like what we have here, continue to leave it un-i18n'd 👍

/** @type {LH.Audit.Details.Table['headings']} */
const headings = [
{key: 'url', itemType: 'url', text: 'URL'},
Expand All @@ -72,7 +74,7 @@ class NetworkRequests extends Audit {
itemType: 'bytes',
displayUnit: 'kb',
granularity: 1,
text: 'Transfer Size', // TODO(exterkamp): i18n this when i18n'ing this file
text: 'Transfer Size',
},
{
key: 'resourceSize',
Expand Down
60 changes: 44 additions & 16 deletions lighthouse-core/test/audits/network-requests-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,28 +6,50 @@
'use strict';

const NetworkRequests = require('../../audits/network-requests.js');
const assert = require('assert');
const networkRecordsToDevtoolsLog = require('../network-records-to-devtools-log.js');

const acceptableDevToolsLog = require('../fixtures/traces/progressive-app-m60.devtools.log.json');
const cutoffLoadDevtoolsLog = require('../fixtures/traces/cutoff-load-m83.devtoolslog.json');
Copy link
Member Author

@brendankenny brendankenny Mar 31, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

introduced a new test devtoolsLog/trace of a load of the report viewer, devtools throttling, cut off at 1500ms (via --max-wait-for-load). Six network requests, three unfinished.

Seems handy to have a kind of gross load like that with relatively small fixture file sizes.


/* eslint-env jest */
describe('Network requests audit', () => {
it('should return network requests', () => {
it('should report finished and unfinished network requests', async () => {
const artifacts = {
devtoolsLogs: {
[NetworkRequests.DEFAULT_PASS]: acceptableDevToolsLog,
[NetworkRequests.DEFAULT_PASS]: cutoffLoadDevtoolsLog,
},
};

return NetworkRequests.audit(artifacts, {computedCache: new Map()}).then(output => {
assert.equal(output.score, 1);
assert.equal(output.details.items.length, 66);
assert.equal(output.details.items[0].url, 'https://pwa.rocks/');
assert.equal(output.details.items[0].startTime, 0);
assert.equal(Math.round(output.details.items[0].endTime), 280);
assert.equal(output.details.items[0].statusCode, 200);
assert.equal(output.details.items[0].transferSize, 5368);
const output = await NetworkRequests.audit(artifacts, {computedCache: new Map()});

expect(output.details.items[0]).toMatchObject({
startTime: 0,
endTime: expect.toBeApproximately(701, 0),
finished: true,
transferSize: 11358,
resourceSize: 39471,
statusCode: 200,
mimeType: 'text/html',
resourceType: 'Document',
});
expect(output.details.items[2]).toMatchObject({
startTime: expect.toBeApproximately(711, 0),
endTime: expect.toBeApproximately(1289, 0),
finished: false,
transferSize: 26441,
resourceSize: 0,
statusCode: 200,
mimeType: 'image/png',
resourceType: 'Image',
});
expect(output.details.items[5]).toMatchObject({
startTime: expect.toBeApproximately(717, 0),
endTime: expect.toBeApproximately(1296, 0),
finished: false,
transferSize: 58571,
resourceSize: 0,
statusCode: 200,
mimeType: 'application/javascript',
resourceType: 'Script',
});
});

Expand All @@ -43,9 +65,15 @@ describe('Network requests audit', () => {
},
};
const output = await NetworkRequests.audit(artifacts, {computedCache: new Map()});
assert.equal(output.details.items[0].startTime, 0);
assert.equal(output.details.items[0].endTime, 500);
assert.equal(output.details.items[1].startTime, 500);
assert.equal(output.details.items[1].endTime, undefined);

expect(output.details.items).toMatchObject([{
startTime: 0,
endTime: 500,
finished: true,
}, {
startTime: 500,
endTime: undefined,
finished: true,
}]);
});
});
1 change: 1 addition & 0 deletions lighthouse-core/test/fixtures/traces/cutoff-load-m83.devtoolslog.json
View file
Open in desktop

Large diffs are not rendered by default.

1 change: 1 addition & 0 deletions lighthouse-core/test/fixtures/traces/cutoff-load-m83.trace.json
View file
Open in desktop

Large diffs are not rendered by default.

18 changes: 18 additions & 0 deletions lighthouse-core/test/results/sample_v2.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -994,6 +994,7 @@
"url": "http://localhost:10200/dobetterweb/dbw_tester.html",
"startTime": 0,
"endTime": 640.1550000009593,
"finished": true,
"transferSize": 12640,
"resourceSize": 12519,
"statusCode": 200,
Expand All @@ -1004,6 +1005,7 @@
"url": "http://localhost:10200/dobetterweb/dbw_tester.css?delay=2000&async=true",
"startTime": 630.2950000099372,
"endTime": 2635.035000013886,
"finished": true,
"transferSize": 821,
"resourceSize": 677,
"statusCode": 200,
Expand All @@ -1014,6 +1016,7 @@
"url": "http://localhost:10200/dobetterweb/dbw_tester.css?delay=100",
"startTime": 635.496000002604,
"endTime": 1204.6590000099968,
"finished": true,
"transferSize": 821,
"resourceSize": 677,
"statusCode": 200,
Expand All @@ -1024,6 +1027,7 @@
"url": "http://localhost:10200/dobetterweb/unknown404.css?delay=200",
"startTime": 636.6400000115391,
"endTime": 1213.2910000218544,
"finished": true,
"transferSize": 139,
"resourceSize": 0,
"statusCode": 404,
Expand All @@ -1034,6 +1038,7 @@
"url": "http://localhost:10200/dobetterweb/dbw_tester.css?delay=2200",
"startTime": 638.0040000076406,
"endTime": 2849.3670000170823,
"finished": true,
"transferSize": 821,
"resourceSize": 677,
"statusCode": 200,
Expand All @@ -1044,6 +1049,7 @@
"url": "http://localhost:10200/dobetterweb/dbw_disabled.css?delay=200&isdisabled",
"startTime": 638.7899999972433,
"endTime": 1220.04100002232,
"finished": true,
"transferSize": 1108,
"resourceSize": 964,
"statusCode": 200,
Expand All @@ -1054,6 +1060,7 @@
"url": "http://localhost:10200/dobetterweb/dbw_partial_a.html?delay=200",
"startTime": 640.5979999981355,
"endTime": 1228.5180000180844,
"finished": true,
"transferSize": 736,
"resourceSize": 616,
"statusCode": 200,
Expand All @@ -1064,6 +1071,7 @@
"url": "http://localhost:10200/dobetterweb/dbw_partial_b.html?delay=200&isasync",
"startTime": 641.3450000109151,
"endTime": 1776.4320000133011,
"finished": true,
"transferSize": 733,
"resourceSize": 613,
"statusCode": 200,
Expand All @@ -1074,6 +1082,7 @@
"url": "http://localhost:10200/dobetterweb/dbw_tester.css?delay=3000&async=true",
"startTime": 642.8679999953602,
"endTime": 4216.161000018474,
"finished": true,
"transferSize": 821,
"resourceSize": 677,
"statusCode": 200,
Expand All @@ -1084,6 +1093,7 @@
"url": "http://localhost:10200/dobetterweb/dbw_tester.js",
"startTime": 644.0820000134408,
"endTime": 1792.0860000012908,
"finished": true,
"transferSize": 1703,
"resourceSize": 1552,
"statusCode": 200,
Expand All @@ -1094,6 +1104,7 @@
"url": "http://localhost:10200/dobetterweb/empty_module.js?delay=500",
"startTime": 645.529000001261,
"endTime": 1236.1859999946319,
"finished": true,
"transferSize": 144,
"resourceSize": 0,
"statusCode": 200,
Expand All @@ -1104,6 +1115,7 @@
"url": "http://localhost:10200/dobetterweb/lighthouse-480x318.jpg",
"startTime": 3951.6250000160653,
"endTime": 4779.641000000993,
"finished": true,
"transferSize": 24741,
"resourceSize": 24620,
"statusCode": 200,
Expand All @@ -1114,6 +1126,7 @@
"url": "http://localhost:10200/zone.js",
"startTime": 2849.7340000176337,
"endTime": 3961.049000005005,
"finished": true,
"transferSize": 71654,
"resourceSize": 71501,
"statusCode": 200,
Expand All @@ -1124,6 +1137,7 @@
"url": "http://ajax.googleapis.com/ajax/libs/jquery/2.1.1/jquery.min.js",
"startTime": 3874.7540000185836,
"endTime": 4796.288000012282,
"finished": true,
"transferSize": 30174,
"resourceSize": 84245,
"statusCode": 200,
Expand All @@ -1134,6 +1148,7 @@
"url": "http://localhost:10200/dobetterweb/dbw_tester.css?scriptActivated&delay=200",
"startTime": 2924.34100000537,
"endTime": 3964.233000006061,
"finished": true,
"transferSize": 821,
"resourceSize": 677,
"statusCode": 200,
Expand All @@ -1144,6 +1159,7 @@
"url": "http://localhost:10200/dobetterweb/dbw_tester.html",
"startTime": 3066.252999997232,
"endTime": 3772.7560000203084,
"finished": true,
"transferSize": 12640,
"resourceSize": 12519,
"statusCode": 200,
Expand All @@ -1154,6 +1170,7 @@
"url": "blob:http://localhost:10200/ae0eac03-ab9b-4a6a-b299-f5212153e277",
"startTime": 3829.6360000094865,
"endTime": 3968.59800000675,
"finished": true,
"transferSize": 0,
"resourceSize": 4,
"statusCode": 200,
Expand All @@ -1164,6 +1181,7 @@
"url": "http://localhost:10200/favicon.ico",
"startTime": 4967.373000021325,
"endTime": 5536.498000001302,
"finished": true,
"transferSize": 221,
"resourceSize": 95,
"statusCode": 404,
Expand Down
13 changes: 13 additions & 0 deletions lighthouse-core/test/test-utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
/* eslint-env jest */

const i18n = require('../lib/i18n/i18n.js');
const {default: {toBeCloseTo}} = require('expect/build/matchers.js');

expect.extend({
toBeDisplayString(received, expected) {
Expand All @@ -27,4 +28,16 @@ expect.extend({

return {actual, message, pass};
},

// Expose toBeCloseTo() so it can be used as an asymmetric matcher.
toBeApproximately(...args) {
Copy link
Member Author

Choose a reason for hiding this comment

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

it's really dumb you can't use toBeCloseTo in toMatchObject, so this takes care of that :)

// If called asymmetrically, a fake matcher `this` object needs to be passed
// in (see https://github.com/facebook/jest/issues/8295). There's no effect
// because it's only used for the printing of full failures, which isn't
// done for asymmetric matchers anyways.
const thisObj = (this && this.utils) ? this :
{isNot: false, promise: ''};

return toBeCloseTo.call(thisObj, ...args);
},
});
Loading