core(metric): add start time check to graph computation #11870
Conversation
…rder to catch network nodes that have a negative end time
LukasAuerMstage
requested review from
adamraine
and removed request for
a team
|
|
|
I have a hard time reproducing these pretty significant differences between expected and actual LCP values that get reported by the failed tests as my local results are much more subtle and similar. Nevertheless, some improvements in the LCP values are to be expected i suppose. |
adamraine
left a comment
adamraine
left a comment
There was a problem hiding this comment.
Could you add a unit test for this as well?
Co-authored-by: Adam Raine <[email protected]>
adamraine
left a comment
adamraine
left a comment
There was a problem hiding this comment.
Nice work on the test cases, look good overall.
| * @license Copyright 2020 The Lighthouse Authors. All Rights Reserved. | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0 | ||
| * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License. | ||
| */ |
There was a problem hiding this comment.
We shouldn't need a new file for these tests. You should add them to https://github.com/GoogleChrome/lighthouse/blob/master/lighthouse-core/test/computed/metrics/lantern-first-contentful-paint-test.js
|
|
||
| describe('Exclude unfinished resources from graph', () => { | ||
| it('shouldn\'t be included in the graph if start time is past the paint timestamp', async () => { | ||
| const networkRecords = [ |
There was a problem hiding this comment.
You can create a test trace and devtools log using these functions:
const createTestTrace = require('../../create-test-trace.js');
const networkRecordsToDevtoolsLog = require('../../network-records-to-devtools-log.js');Keep in mind that createTestTrace adds an FCP event automatically.
Check out https://github.com/GoogleChrome/lighthouse/blob/master/lighthouse-core/test/audits/third-party-facades-test.js for a usage example.
| const { | ||
| optimisticGraph, | ||
| pessimisticGraph, | ||
| } = getComputedGraphs( |
There was a problem hiding this comment.
Instead of creating getComputedGraphs as a test function, request the metric directly:
const result = await LanternFirstContentfulPaint.request({trace, devtoolsLog, settings}, context)`Get the graphs from result.optimisticGraph and result.pessimisticGraph.
Check out https://github.com/GoogleChrome/lighthouse/blob/master/lighthouse-core/test/computed/metrics/lantern-first-contentful-paint-test.js for usage reference.
| const NetworkRequest = require('../../lib/network-request.js'); | ||
| const BaseNode = require('../../lib/dependency-graph/base-node.js'); | ||
| const PageDependencyGraph = require('../../computed/page-dependency-graph.js'); | ||
| const LanternLCP = require('../../computed/metrics/lantern-largest-contentful-paint.js'); |
There was a problem hiding this comment.
This change affects the lantern FCP graph directly, it makes more sense to use that:
const LanternFirstContentfulPaint = require('../../../computed/metrics/lantern-first-contentful-paint.js');3 participants
Summary
This PR adds an additional check for whether the node startTime is beyond the paint timestamp, optional to the existing check for the endTime, to exclude nodes that have a startTime beyond the timestamp but a negative endTime.
The fix is required because otherwise certain nodes, such as prefetch resource hints, might be incorrectly added to the lcp graph.
Related Issues/PRs
#11856