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: better times for generated network requests #14714

Merged
merged 4 commits into from
Jan 26, 2023
Merged
Show file tree
Hide file tree
Changes from 3 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
12 changes: 7 additions & 5 deletions core/test/audits/network-requests-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -71,8 +71,8 @@ describe('Network requests audit', () => {

it('should handle times correctly', async () => {
const records = [
{url: 'https://example.com/0', startTime: 15.0, endTime: 15.5},
{url: 'https://example.com/1', startTime: 15.5, endTime: -1},
{url: 'https://example.com/0', rendererStartTime: 14, startTime: 15.0, endTime: 15.5},
{url: 'https://example.com/1', rendererStartTime: 14, startTime: 15.5, endTime: -1},
];

const artifacts = {
Expand All @@ -85,11 +85,13 @@ describe('Network requests audit', () => {
const output = await NetworkRequests.audit(artifacts, {computedCache: new Map()});

expect(output.details.items).toMatchObject([{
startTime: 0,
endTime: 0.5,
rendererStartTime: 0,
startTime: 1,
endTime: 1.5,
finished: true,
}, {
startTime: 0.5,
rendererStartTime: 0,
startTime: 1.5,
endTime: undefined,
finished: true,
}]);
Expand Down
14 changes: 7 additions & 7 deletions core/test/audits/preload-lcp-image-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ describe('Performance: preload-lcp audit', () => {
priority: 'High',
isLinkPreload: false,
startTime: 0,
endTime: 0.5,
endTime: 500,
Copy link
Member Author

Choose a reason for hiding this comment

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

startTime 0 + receiveHeadersEnd 500 is well after an endTime of 0.5 :)

timing: {receiveHeadersEnd: 500},
url: rootNodeUrl,
},
Expand All @@ -57,8 +57,8 @@ describe('Performance: preload-lcp audit', () => {
resourceType: 'Document',
priority: 'High',
isLinkPreload: false,
startTime: 0.5,
endTime: 1,
startTime: 500,
endTime: 1000,
timing: {receiveHeadersEnd: 500},
url: mainDocumentNodeUrl,
},
Expand All @@ -67,8 +67,8 @@ describe('Performance: preload-lcp audit', () => {
resourceType: 'Script',
priority: 'High',
isLinkPreload: false,
startTime: 1,
endTime: 5,
startTime: 1000,
endTime: 5000,
timing: {receiveHeadersEnd: 4000},
url: scriptNodeUrl,
initiator: {type: 'parser', url: mainDocumentNodeUrl},
Expand All @@ -78,8 +78,8 @@ describe('Performance: preload-lcp audit', () => {
resourceType: 'Image',
priority: 'High',
isLinkPreload: false,
startTime: 2,
endTime: 4.5,
startTime: 2000,
endTime: 4500,
timing: {receiveHeadersEnd: 2500},
url: imageUrl,
initiator: {type: 'script', url: scriptNodeUrl},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,19 +41,19 @@ describe('Metrics: Lantern FCP', () => {
const devtoolsLog = networkRecordsToDevtoolsLog([
{
transferSize: 2000,
url: 'https://example.com/',
url: 'https://example.com/', // Main document (always included).
resourceType: 'Document',
priority: 'High',
startTime: 0,
endTime: 0.0001, // Before FCP
endTime: 1000,
timing: {sslStart: 50, sslEnd: 100, connectStart: 50, connectEnd: 100},
Comment on lines +48 to 49
Copy link
Member Author

@brendankenny brendankenny Jan 26, 2023

Choose a reason for hiding this comment

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

these timing entries aren't possible before an endTime of 0.0001, but going back to #13452, main document is always included (so doesn't need to be before FCP), and the next (incomplete) request can be any time after FCP, so these changes still hit the check added in that PR

},
{
transferSize: 2000,
url: 'https://example.com/script.js',
resourceType: 'Script',
priority: 'High',
startTime: 0.015, // After FCP
startTime: 1000, // After FCP.
endTime: -1,
timing: {sslStart: 50, sslEnd: 100, connectStart: 50, connectEnd: 100},
},
Expand Down
26 changes: 14 additions & 12 deletions core/test/gather/gatherers/inspector-issues-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,29 +5,29 @@
*/

import InspectorIssues from '../../../gather/gatherers/inspector-issues.js';
import {NetworkRequest} from '../../../lib/network-request.js';
import {createMockContext} from '../mock-driver.js';
import {flushAllTimersAndMicrotasks, timers} from '../../test-utils.js';
import {networkRecordsToDevtoolsLog} from '../../network-records-to-devtools-log.js';
import {NetworkRecorder} from '../../../lib/network-recorder.js';

/**
* @param {Partial<LH.Artifacts.NetworkRequest>=} partial
* @return {LH.Artifacts.NetworkRequest}
* @return {Partial<LH.Artifacts.NetworkRequest>}
*/
function mockRequest(partial) {
return Object.assign(new NetworkRequest(), {
Copy link
Member Author

@brendankenny brendankenny Jan 26, 2023

Choose a reason for hiding this comment

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

assigning to a new NetworkRequest makes for a weird network request (lots of -1s), and then passing it through networkRecordsToDevtoolsLog just breaks things. Instead, just use networkRecordsToDevtoolsLog and NetworkRecorder to make the NetworkRequests

return {
url: 'https://example.com',
documentURL: 'https://example.com',
finished: true,
frameId: 'frameId',
isSecure: true,
isValid: true,
parsedURL: {scheme: 'https'},
parsedURL: {scheme: 'https', host: 'example.com', securityOrigin: 'https://example.com'},
protocol: 'http/1.1',
requestMethod: 'GET',
resourceType: 'Document',
...partial,
});
};
}

/**
Expand Down Expand Up @@ -185,11 +185,12 @@ describe('_getArtifact', () => {
mockCSP(),
mockDeprecation('AuthorizationCoveredByWildcard'),
];
const networkRecords = [
const devtoolsLog = networkRecordsToDevtoolsLog([
mockRequest({requestId: '1'}),
mockRequest({requestId: '2'}),
mockRequest({requestId: '3'}),
];
]);
const networkRecords = NetworkRecorder.recordsFromLogs(devtoolsLog);

const artifact = await gatherer._getArtifact(networkRecords);

Expand Down Expand Up @@ -258,11 +259,12 @@ describe('_getArtifact', () => {
mockBlockedByResponse({request: {requestId: '5'}}),
mockBlockedByResponse({request: {requestId: '6'}}),
];
const networkRecords = [
const devtoolsLog = networkRecordsToDevtoolsLog([
mockRequest({requestId: '1'}),
mockRequest({requestId: '3'}),
mockRequest({requestId: '5'}),
];
]);
const networkRecords = NetworkRecorder.recordsFromLogs(devtoolsLog);

const artifact = await gatherer._getArtifact(networkRecords);

Expand Down Expand Up @@ -327,10 +329,10 @@ describe('FR compat (inspector-issues)', () => {
.mockEvent('Audits.issueAdded', {
issue: mockMixedContent({request: {requestId: '1'}}),
});
networkRecords = [
devtoolsLog = networkRecordsToDevtoolsLog([
mockRequest({requestId: '1'}),
];
devtoolsLog = networkRecordsToDevtoolsLog(networkRecords);
]);
networkRecords = NetworkRecorder.recordsFromLogs(devtoolsLog);
});

it('uses loadData in legacy mode', async () => {
Expand Down
14 changes: 7 additions & 7 deletions core/test/lib/network-request-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -112,9 +112,9 @@ describe('NetworkRequest', () => {
function getRequest() {
return {
rendererStartTime: 0,
startTime: 0,
endTime: 2000,
startTime: 50,
responseReceivedTime: 1000,
endTime: 2000,
Comment on lines -116 to +117
Copy link
Member Author

Choose a reason for hiding this comment

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

test change from #14574 to verify this works for that


// units = ms
responseHeaders: [
Expand All @@ -135,11 +135,11 @@ describe('NetworkRequest', () => {
const record = NetworkRecorder.recordsFromLogs(devtoolsLog)[0];

expect(record.rendererStartTime).toStrictEqual(0);
expect(record.startTime).toStrictEqual(0);
expect(record.startTime).toStrictEqual(50);
expect(record.endTime).toStrictEqual(2000);
expect(record.responseReceivedTime).toStrictEqual(1000);
expect(record.lrStatistics).toStrictEqual({
endTimeDeltaMs: -8000,
endTimeDeltaMs: -8050,
TCPMs: 5000,
requestMs: 2500,
responseMs: 2500,
Expand Down Expand Up @@ -228,7 +228,7 @@ describe('NetworkRequest', () => {
const record = NetworkRecorder.recordsFromLogs(devtoolsLog)[0];

expect(record.lrStatistics).toStrictEqual({
endTimeDeltaMs: -8000,
endTimeDeltaMs: -8050,
TCPMs: 0,
requestMs: 0,
responseMs: 10000,
Expand All @@ -246,7 +246,7 @@ describe('NetworkRequest', () => {
const record = NetworkRecorder.recordsFromLogs(devtoolsLog)[0];

expect(record.lrStatistics).toStrictEqual({
endTimeDeltaMs: -8000,
endTimeDeltaMs: -8050,
TCPMs: 1000,
requestMs: 0,
responseMs: 9000,
Expand All @@ -270,7 +270,7 @@ describe('NetworkRequest', () => {
sslStart: 35,
});
expect(lrRecord.lrStatistics).toStrictEqual({
endTimeDeltaMs: -8000,
endTimeDeltaMs: -8050,
TCPMs: 5000,
requestMs: 2500,
responseMs: 2500,
Expand Down
59 changes: 59 additions & 0 deletions core/test/network-records-to-devtools-log-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -59,4 +59,63 @@ describe('networkRecordsToDevtoolsLog', () => {

expect(roundTripRecords).toEqual(records);
});

it('should roundtrip fake network records multiple times', () => {
const records = [
{requestId: '0', url: 'http://example.com/'},
{requestId: '0:redirect', url: 'https://www.example.com/'},
{url: 'https://example.com/img.png', rendererStartTime: 14, startTime: 15, endTime: 15.5},
{url: 'https://example.com/style.css', timing: {receiveHeadersEnd: 400, sendEnd: 200}},
{url: 'https://example.com/0.js', rendererStartTime: 10000},
{url: 'https://example.com/1.js', startTime: 2000, responseReceivedTime: 3250},
{url: 'https://example.com/2.js', timing: {requestTime: 5}},
{url: 'https://example.com/3.js', endTime: -1},
{url: 'https://example.com/4.js', timing: {sendEnd: 1200}},
{},
];

const devtoolsLog = networkRecordsToDevtoolsLog(records);
const roundTripRecords = NetworkRecorder.recordsFromLogs(devtoolsLog);

const devtoolsLog2 = networkRecordsToDevtoolsLog(roundTripRecords);
const roundTripRecords2 = NetworkRecorder.recordsFromLogs(devtoolsLog2);
expect(roundTripRecords2).toMatchObject(roundTripRecords);

const devtoolsLog3 = networkRecordsToDevtoolsLog(roundTripRecords2);
const roundTripRecords3 = NetworkRecorder.recordsFromLogs(devtoolsLog3);
expect(roundTripRecords3).toMatchObject(roundTripRecords2);
expect(roundTripRecords3).toMatchObject(roundTripRecords);
expect(roundTripRecords3).toMatchObject(records);
});

it('should throw on impossible network requests', () => {
expect(() => networkRecordsToDevtoolsLog([{rendererStartTime: 5, startTime: 1}]))
.toThrow(`'rendererStartTime' (5) exceeds 'startTime' (1)`);
expect(() => networkRecordsToDevtoolsLog([{rendererStartTime: 5, endTime: 1}]))
.toThrow(`'rendererStartTime' (5) exceeds 'endTime' (1)`);

expect(() => networkRecordsToDevtoolsLog([{startTime: 1000, timing: {requestTime: 2}}]))
.toThrow(`'startTime' (1000) is not equal to 'timing.requestTime' (2 seconds)`);

/* eslint-disable max-len */
expect(() => networkRecordsToDevtoolsLog([{startTime: 1000, responseReceivedTime: 2000, timing: {sendEnd: 1200}}]))
.toThrow(`request start (1000) plus relative 'timing' value (1200) exceeds 'responseReceivedTime' (2000)`);
expect(() => networkRecordsToDevtoolsLog([{rendererStartTime: 1000, responseReceivedTime: 2000, timing: {sendEnd: 1200}}]))
.toThrow(`request start (1000) plus relative 'timing' value (1200) exceeds 'responseReceivedTime' (2000)`);
expect(() => networkRecordsToDevtoolsLog([{responseReceivedTime: 2000, timing: {requestTime: 1, sendEnd: 1200}}]))
.toThrow(`request start (1000) plus relative 'timing' value (1200) exceeds 'responseReceivedTime' (2000)`);

expect(() => networkRecordsToDevtoolsLog([{startTime: 500, endTime: 2000, timing: {connectStart: 2200}}]))
.toThrow(`request start (500) plus relative 'timing' value (2200) exceeds 'endTime' (2000)`);
expect(() => networkRecordsToDevtoolsLog([{rendererStartTime: 500, endTime: 2000, timing: {connectStart: 2200}}]))
.toThrow(`request start (500) plus relative 'timing' value (2200) exceeds 'endTime' (2000)`);
expect(() => networkRecordsToDevtoolsLog([{endTime: 2000, timing: {requestTime: 0.5, connectStart: 2200}}]))
.toThrow(`request start (500) plus relative 'timing' value (2200) exceeds 'endTime' (2000)`);

expect(() => networkRecordsToDevtoolsLog([{startTime: 500, responseReceivedTime: 2000, timing: {receiveHeadersEnd: 500}}]))
.toThrow(`request start (500) plus 'receiveHeadersEnd' (500) does not equal 'responseReceivedTime' (2000)`);
expect(() => networkRecordsToDevtoolsLog([{responseReceivedTime: 2000, timing: {requestTime: 0.5, receiveHeadersEnd: 500}}]))
.toThrow(`request start (500) plus 'receiveHeadersEnd' (500) does not equal 'responseReceivedTime' (2000)`);
/* eslint-enable max-len */
});
});
Loading