core(fr): align navigation-runner with legacy gather-runner #12478
Conversation
patrickhulce
force-pushed
the
fr_navigation_parity
branch
from
5629641
to
8cc3abc
Compare
|
|
||
| const baseArtifacts = await getBaseArtifacts(config, driver); | ||
| baseArtifacts.URL.requestedUrl = requestedUrl; | ||
|
|
||
| await prepare.prepareTargetForNavigationMode(driver, config.settings); |
There was a problem hiding this comment.
Are we leaving this check out then or is it still TBD?
lighthouse/lighthouse-core/gather/gather-runner.js
Lines 155 to 157 in c12959f
There was a problem hiding this comment.
I'm intending on leaving it out as it's unrealistic to enforce in a puppeteer context, but I like leaving the TODO in as a callout. We'll probably end up converting it to a warning when we make a pass over our leftover TODOs soon
| log.error('NavigationRunner', localizedMessage, navigationContext.requestedUrl); | ||
|
|
||
| return {artifacts: {}, warnings: [...warnings, pageLoadError.friendlyMessage], pageLoadError}; | ||
| } else { |
There was a problem hiding this comment.
ack
I intentionally added the else to make explicit the two paths this can take. IMO, without the else it's very easy to miss the return and think that the else bit is executed even in the case of a pageLoadError. I'm all for the early return pattern to avoid nesting but when it's a one-line and/or early check where the intention is clearer :) Convinced? or still prefer no else?
There was a problem hiding this comment.
I still prefer no else but I won't die on this hill, it's your choice.
| break; | ||
| } | ||
|
|
||
| // TODO(FR-COMPAT): merge RunWarnings between navigations |
There was a problem hiding this comment.
What's preventing us from doing this now?
There was a problem hiding this comment.
PR size, the handling of runwarnings has a small cascade of other changes that already fit in well with the base artifacts cleanup I have left and was planning on putting up today.
| it.todo('should reset storage'); | ||
| it.todo('should not reset storage when skipped'); |
There was a problem hiding this comment.
The reset storage tests seem useful at least.
There was a problem hiding this comment.
agreed, but these are all tested in prepare-test.js now, I put these placeholders here when the runner was first added to capture what gather-runner did at the time, and didn't update as those pieces were stripped out of gather-runner into submodules.
Or were you commenting that an integration test where the preparation module isn't mocked would be preferred?
There was a problem hiding this comment.
I was thinking these applied to the code in _cleanup as well.
There was a problem hiding this comment.
Ah gotcha, for _cleanup! Yes good catch 👍
Co-authored-by: Adam Raine <6752989+adamraine@users.noreply.github.com>
patrickhulce
force-pushed
the
fr_navigation_parity
branch
from
b8325c0
to
b6cec29
Compare
| const artifacts = await awaitArtifacts(phaseState.artifactState); | ||
| return {artifacts, warnings, pageLoadError: undefined}; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * @param {NavigationContext} navigationContext |
|
Summary
Won't work until #12469 is merged, butthis brings fraggle rock up to snuff with gather runner and makes use of all the refactoring over the past month! 🎉Related Issues/PRs
ref #11313