core(fr): convert additional base artifacts #12594
Conversation
| const HostUserAgent = (await getBrowserVersion(driver.defaultSession)).userAgent; | ||
|
|
||
| // Whether Lighthouse was run on a mobile device (i.e. not on a desktop machine). | ||
| const HostFormFactor = | ||
| HostUserAgent.includes('Android') || HostUserAgent.includes('Mobile') ? 'mobile' : 'desktop'; | ||
|
|
||
| const BenchmarkIndex = await getBenchmarkIndex(driver.executionContext); |
There was a problem hiding this comment.
Should we keep this here so we don't need to provide a starting value for legacy base artifacts?
There was a problem hiding this comment.
might want to rebase this on top of https://github.com/GoogleChrome/lighthouse/pull/12510/files which fixes up most of our base artifact usage in FR
| @@ -114,21 +114,13 @@ describe('NavigationRunner', () => { | |||
|
|
|||
| it('should collect base artifacts', async () => { | |||
| const {baseArtifacts} = await runner._setup({driver, config, requestedUrl}); | |||
| expect(baseArtifacts).toMatchObject({HostUserAgent: 'Chrome', URL: {requestedUrl}}); | |||
There was a problem hiding this comment.
HostUserAgent is not initialized in _setup anymore
lighthouse-core/test/fraggle-rock/gather/navigation-runner-test.js
Outdated
Show resolved
Hide resolved
| export type PhaseArtifact = | | ||
| LH.GathererArtifacts[keyof LH.GathererArtifacts] | | ||
| LH.Artifacts['devtoolsLogs'] | | ||
| LH.Artifacts['traces'] | | ||
| LH.Artifacts['WebAppManifest'] | | ||
| LH.Artifacts['InstallabilityErrors'] | | ||
| LH.Artifacts['Stacks']; | ||
| export type PhaseResultNonPromise = void|PhaseArtifact |
There was a problem hiding this comment.
Only difference here should be NetworkUserAgent
There was a problem hiding this comment.
HostFormFactor/HostUserAgent look great!
would you be OK rebasing and landing on top of https://github.com/GoogleChrome/lighthouse/pull/12510/files? I was holding off merging that as 8 got closer, but it solves a few of the callouts here :)
| const HostUserAgent = (await getBrowserVersion(driver.defaultSession)).userAgent; | ||
|
|
||
| // Whether Lighthouse was run on a mobile device (i.e. not on a desktop machine). | ||
| const HostFormFactor = | ||
| HostUserAgent.includes('Android') || HostUserAgent.includes('Mobile') ? 'mobile' : 'desktop'; | ||
|
|
||
| const BenchmarkIndex = await getBenchmarkIndex(driver.executionContext); |
There was a problem hiding this comment.
might want to rebase this on top of https://github.com/GoogleChrome/lighthouse/pull/12510/files which fixes up most of our base artifact usage in FR
lighthouse-core/test/fraggle-rock/gather/navigation-runner-test.js
Outdated
Show resolved
Hide resolved
|
|
adamraine
force-pushed
the
fr-more-base-artifacts
branch
from
412612c
to
3a11584
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| * @return {Promise<LH.Artifacts['HostFormFactor']>} | ||
| */ | ||
| async getArtifact(context) { | ||
| const userAgent = context.dependencies.HostUserAgent; |
There was a problem hiding this comment.
time to start a breaking changes v9 issue to merge these :)
| expect(artifacts.HostFormFactor).toEqual('desktop'); | ||
| }); | ||
|
|
||
| it('should fetch the mobile user agent', async () => { |
There was a problem hiding this comment.
we'll probably want to bring these tests back when we have the breaking change for 9.0 to merge those
Converts
HostUserAgent,HostFormFactor,andto true gatherers. This is a necessary step to convertBenchmarkIndexScriptElementswhich depends onHostFormFactor.Land after #12510