core(metrics): support FCP for all frames with devtools throttling #11874
Conversation
adamraine
changed the title
lighthouse-core/test/computed/metrics/first-contentful-paint-all-frames-test.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
thanks @adamraine! you really set yourself up for success with the prior work on frameTreeEvents, nice and easy addition here 👍
lighthouse-core/computed/metrics/first-contentful-paint-all-frames.js
Outdated
Show resolved
Hide resolved
| @@ -85,8 +93,8 @@ Object { | |||
| "observedFirstVisualChange": 1105, | |||
| "observedFirstVisualChangeTs": 713038128064, | |||
| "observedLargestContentfulPaint": 1122, | |||
| "observedLargestContentfulPaintAllFrames": undefined, | |||
| "observedLargestContentfulPaintAllFramesTs": undefined, | |||
| "observedLargestContentfulPaintAllFrames": 1122, | |||
There was a problem hiding this comment.
hm, why did this one change? I wouldn't have expected movement here.
There was a problem hiding this comment.
Previously, if there were no FrameTreeCommittedInBrowser events, only FCP-AF would fall back to FCP because we needed to avoid a NO_FCP error. LCP-AF didn't need to fall back because a NO_LCP_ALL_FRAMES error didn't brick a bunch of existing tests.
Now that frameTreeEvents falls back to frameEvents, LCP-AF falls back to LCP because it looks at frameTreeEvents.
lighthouse-core/test/computed/metrics/first-contentful-paint-all-frames-test.js
Show resolved
Hide resolved
| @@ -46,13 +46,16 @@ describe('Metrics: LCP from all frames', () => { | |||
| await expect(resultPromise).rejects.toThrow('NO_LCP_ALL_FRAMES'); | |||
| }); | |||
|
|
|||
| it('should fail if even if main frame LCP is available', async () => { | |||
| it('should use main frame LCP if no other frames', async () => { | |||
There was a problem hiding this comment.
ah I'm guessing this was the LCP change then? I imagine it might make querying a smidge harder than it was before, but this is consistent with FCP behavior now and I like it 👍
There was a problem hiding this comment.
Yes, this is the change in #11874 (comment)
|
oh and a smoke please 😃 |
|
@patrickhulce Is there a way to check equivalence between two fields in a smoke test? I think the best thing to test would be LCP != LCP-AF and FCP != FCP-AF. |
Not really. I would set FCP to be very far out (~10s maybe?), FCP-AF to be instant and assert |
| firstContentfulPaintAllFrames: '<1000', | ||
| largestContentfulPaint: '>1000', | ||
| largestContentfulPaintAllFrames: '<1000', | ||
| firstContentfulPaint: '>5000', |
Follow up to #11701, #11713 and #11760
A lot of test traces are incompatible with these changes because they were minified without
FrameCommittedInBrowserevents. The FCP from all frames will be set to the main frame FCP in these cases.Unlike LCP, FCP all frames will not throw when using simulated throttling because that breaks some tests. FCP all frames will fall back to main frame FCP from lantern in this case. This fallback will be removed once support for metrics in lantern is added.
Related to:
#11500
#11311