From d2fbd59fd01293733e23383202bd24175b2add66 Mon Sep 17 00:00:00 2001 From: Connor Clark Date: Tue, 21 May 2024 15:16:29 -0700 Subject: [PATCH 1/3] core(network): fix time units in network quiet calc --- core/computed/metrics/interactive.js | 4 ++-- core/gather/driver/network-monitor.js | 6 +++--- core/test/computed/metrics/interactive-test.js | 4 ++-- core/test/gather/driver/network-monitor-test.js | 16 ++++++++-------- 4 files changed, 15 insertions(+), 15 deletions(-) diff --git a/core/computed/metrics/interactive.js b/core/computed/metrics/interactive.js index 90f67633d927..2526490d025b 100644 --- a/core/computed/metrics/interactive.js +++ b/core/computed/metrics/interactive.js @@ -187,6 +187,6 @@ export {InteractiveComputed as Interactive}; /** * @typedef TimePeriod - * @property {number} start - * @property {number} end + * @property {number} start In ms. + * @property {number} end In ms. */ diff --git a/core/gather/driver/network-monitor.js b/core/gather/driver/network-monitor.js index 8c6dc73f22f8..0167e3442fc4 100644 --- a/core/gather/driver/network-monitor.js +++ b/core/gather/driver/network-monitor.js @@ -202,6 +202,7 @@ class NetworkMonitor extends NetworkMonitorEventEmitter { /** * Finds all time periods where the number of inflight requests is less than or equal to the * number of allowed concurrent requests. + * The time periods returned are in ms. * @param {Array} requests * @param {number} allowedConcurrentRequests * @param {number=} endTime @@ -215,10 +216,9 @@ class NetworkMonitor extends NetworkMonitorEventEmitter { if (UrlUtils.isNonNetworkProtocol(request.protocol)) return; if (request.protocol === 'ws' || request.protocol === 'wss') return; - // convert the network timestamp to ms - timeBoundaries.push({time: request.networkRequestTime * 1000, isStart: true}); + timeBoundaries.push({time: request.networkRequestTime, isStart: true}); if (request.finished) { - timeBoundaries.push({time: request.networkEndTime * 1000, isStart: false}); + timeBoundaries.push({time: request.networkEndTime, isStart: false}); } }); diff --git a/core/test/computed/metrics/interactive-test.js b/core/test/computed/metrics/interactive-test.js index 216ef384286d..569c1621e55f 100644 --- a/core/test/computed/metrics/interactive-test.js +++ b/core/test/computed/metrics/interactive-test.js @@ -37,8 +37,8 @@ function generateNetworkRecords(partialRecords, timeOrigin) { statusCode: item.statusCode || 200, requestMethod: item.requestMethod || 'GET', finished: typeof item.finished === 'undefined' ? true : item.finished, - networkRequestTime: (item.start + timeOriginInMs) / 1000, - networkEndTime: item.end === -1 ? -1 : (item.end + timeOriginInMs) / 1000, + networkRequestTime: item.start + timeOriginInMs, + networkEndTime: item.end === -1 ? -1 : item.end + timeOriginInMs, }; return /** @type {LH.Artifacts.NetworkRequest} */ (record); }); diff --git a/core/test/gather/driver/network-monitor-test.js b/core/test/gather/driver/network-monitor-test.js index 80337d65cb94..10ee3658e5f9 100644 --- a/core/test/gather/driver/network-monitor-test.js +++ b/core/test/gather/driver/network-monitor-test.js @@ -388,9 +388,9 @@ describe('NetworkMonitor', () => { const periods = NetworkMonitor.findNetworkQuietPeriods(records, 0); expect(periods).toEqual([ - {start: 1000, end: 2000}, - {start: 3000, end: 4000}, - {start: 5000, end: Infinity}, + {start: 1, end: 2}, + {start: 3, end: 4}, + {start: 5, end: Infinity}, ]); }); @@ -404,7 +404,7 @@ describe('NetworkMonitor', () => { ]; const periods = NetworkMonitor.findNetworkQuietPeriods(records, 2); - expect(periods).toEqual([{start: 1500, end: Infinity}]); + expect(periods).toEqual([{start: 1.5, end: Infinity}]); }); it('should handle unfinished requests', () => { @@ -421,9 +421,9 @@ describe('NetworkMonitor', () => { const periods = NetworkMonitor.findNetworkQuietPeriods(records, 2); expect(periods).toEqual([ - {start: 1500, end: 2000}, - {start: 3000, end: 4000}, - {start: 5000, end: 5500}, + {start: 1.5, end: 2}, + {start: 3, end: 4}, + {start: 5, end: 5.5}, ]); }); @@ -435,7 +435,7 @@ describe('NetworkMonitor', () => { ]; const periods = NetworkMonitor.findNetworkQuietPeriods(records, 0); - expect(periods).toEqual([{start: 1000, end: Infinity}]); + expect(periods).toEqual([{start: 1, end: Infinity}]); }); it('should handle iframe requests', () => { From ad96656b8cff523c3565cbc711277871de13e9e5 Mon Sep 17 00:00:00 2001 From: Paul Irish Date: Tue, 21 May 2024 19:07:46 -0700 Subject: [PATCH 2/3] network-monitor test values should all be millisecondy --- .../gather/driver/network-monitor-test.js | 64 +++++++++---------- 1 file changed, 32 insertions(+), 32 deletions(-) diff --git a/core/test/gather/driver/network-monitor-test.js b/core/test/gather/driver/network-monitor-test.js index 10ee3658e5f9..e41dfdf11418 100644 --- a/core/test/gather/driver/network-monitor-test.js +++ b/core/test/gather/driver/network-monitor-test.js @@ -381,61 +381,61 @@ describe('NetworkMonitor', () => { it('should find the 0-quiet periods', () => { const records = [ - record({networkRequestTime: 0, networkEndTime: 1}), - record({networkRequestTime: 2, networkEndTime: 3}), - record({networkRequestTime: 4, networkEndTime: 5}), + record({networkRequestTime: 0, networkEndTime: 1000}), + record({networkRequestTime: 2000, networkEndTime: 3000}), + record({networkRequestTime: 4000, networkEndTime: 5000}), ]; const periods = NetworkMonitor.findNetworkQuietPeriods(records, 0); expect(periods).toEqual([ - {start: 1, end: 2}, - {start: 3, end: 4}, - {start: 5, end: Infinity}, + {start: 1000, end: 2000}, + {start: 3000, end: 4000}, + {start: 5000, end: Infinity}, ]); }); it('should find the 2-quiet periods', () => { const records = [ - record({networkRequestTime: 0, networkEndTime: 1.5}), - record({networkRequestTime: 0, networkEndTime: 2}), - record({networkRequestTime: 0, networkEndTime: 2.5}), - record({networkRequestTime: 2, networkEndTime: 3}), - record({networkRequestTime: 4, networkEndTime: 5}), + record({networkRequestTime: 0, networkEndTime: 1500}), + record({networkRequestTime: 0, networkEndTime: 2000}), + record({networkRequestTime: 0, networkEndTime: 2500}), + record({networkRequestTime: 2000, networkEndTime: 3000}), + record({networkRequestTime: 4000, networkEndTime: 5000}), ]; const periods = NetworkMonitor.findNetworkQuietPeriods(records, 2); - expect(periods).toEqual([{start: 1.5, end: Infinity}]); + expect(periods).toEqual([{start: 1500, end: Infinity}]); }); it('should handle unfinished requests', () => { const records = [ - record({networkRequestTime: 0, networkEndTime: 1.5}), - record({networkRequestTime: 0, networkEndTime: 2}), - record({networkRequestTime: 0, networkEndTime: 2.5}), - record({networkRequestTime: 2, networkEndTime: 3}), - record({networkRequestTime: 2}), - record({networkRequestTime: 2}), - record({networkRequestTime: 4, networkEndTime: 5}), - record({networkRequestTime: 5.5}), + record({networkRequestTime: 0, networkEndTime: 1500}), + record({networkRequestTime: 0, networkEndTime: 2000}), + record({networkRequestTime: 0, networkEndTime: 2500}), + record({networkRequestTime: 2000, networkEndTime: 3000}), + record({networkRequestTime: 2000}), + record({networkRequestTime: 2000}), + record({networkRequestTime: 4000, networkEndTime: 5000}), + record({networkRequestTime: 5500}), ]; const periods = NetworkMonitor.findNetworkQuietPeriods(records, 2); expect(periods).toEqual([ - {start: 1.5, end: 2}, - {start: 3, end: 4}, - {start: 5, end: 5.5}, + {start: 1500, end: 2000}, + {start: 3000, end: 4000}, + {start: 5000, end: 5500}, ]); }); it('should ignore data URIs', () => { const records = [ - record({networkRequestTime: 0, networkEndTime: 1}), - record({networkRequestTime: 0, networkEndTime: 2, url: 'data:image/png;base64,', + record({networkRequestTime: 0, networkEndTime: 1000}), + record({networkRequestTime: 0, networkEndTime: 2000, url: 'data:image/png;base64,', protocol: 'data'}), ]; const periods = NetworkMonitor.findNetworkQuietPeriods(records, 0); - expect(periods).toEqual([{start: 1, end: Infinity}]); + expect(periods).toEqual([{start: 1000, end: Infinity}]); }); it('should handle iframe requests', () => { @@ -443,12 +443,12 @@ describe('NetworkMonitor', () => { finished: false, url: 'https://iframe.com', documentURL: 'https://iframe.com', - responseHeadersEndTime: 1.2, + responseHeadersEndTime: 1200, }; const records = [ - record({networkRequestTime: 0, networkEndTime: 1}), - record({networkRequestTime: 0, networkEndTime: 1.2, ...iframeRequest}), + record({networkRequestTime: 0, networkEndTime: 1000}), + record({networkRequestTime: 0, networkEndTime: 1200, ...iframeRequest}), ]; const periods = NetworkMonitor.findNetworkQuietPeriods(records, 0); @@ -460,12 +460,12 @@ describe('NetworkMonitor', () => { const quicRequest = { finished: false, responseHeaders: [{name: 'ALT-SVC', value: 'hq=":49288";quic="1,1abadaba,51303334,0"'}], - timing: /** @type {*} */ ({receiveHeadersEnd: 1.28}), + timing: /** @type {*} */ ({receiveHeadersEnd: 1280}), }; const records = [ - record({networkRequestTime: 0, networkEndTime: 1}), - record({networkRequestTime: 0, networkEndTime: 2, ...quicRequest}), + record({networkRequestTime: 0, networkEndTime: 1000}), + record({networkRequestTime: 0, networkEndTime: 2000, ...quicRequest}), ]; const periods = NetworkMonitor.findNetworkQuietPeriods(records, 0); From ef08fb6105af244207755655535016dc6376f881 Mon Sep 17 00:00:00 2001 From: Paul Irish Date: Tue, 21 May 2024 19:22:59 -0700 Subject: [PATCH 3/3] add a test that would have failed before this fix --- core/gather/driver/network-monitor.js | 2 +- core/test/gather/driver/network-monitor-test.js | 7 +++++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/core/gather/driver/network-monitor.js b/core/gather/driver/network-monitor.js index 0167e3442fc4..814c357a3703 100644 --- a/core/gather/driver/network-monitor.js +++ b/core/gather/driver/network-monitor.js @@ -205,7 +205,7 @@ class NetworkMonitor extends NetworkMonitorEventEmitter { * The time periods returned are in ms. * @param {Array} requests * @param {number} allowedConcurrentRequests - * @param {number=} endTime + * @param {number=} endTime In ms * @return {Array<{start: number, end: number}>} */ static findNetworkQuietPeriods(requests, allowedConcurrentRequests, endTime = Infinity) { diff --git a/core/test/gather/driver/network-monitor-test.js b/core/test/gather/driver/network-monitor-test.js index e41dfdf11418..faa66ca424ff 100644 --- a/core/test/gather/driver/network-monitor-test.js +++ b/core/test/gather/driver/network-monitor-test.js @@ -392,6 +392,13 @@ describe('NetworkMonitor', () => { {start: 3000, end: 4000}, {start: 5000, end: Infinity}, ]); + // Same thing but verifying these numbers round trip without a problem. + expect(periods).toEqual([ + // The time between the first two, and so on… + {start: records[0].networkEndTime, end: records[1].networkRequestTime}, + {start: records[1].networkEndTime, end: records[2].networkRequestTime}, + {start: records[2].networkEndTime, end: Infinity}, + ]); }); it('should find the 2-quiet periods', () => {