Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

core(navigation-runner): only run getArtifact phase once #15827

Merged
merged 2 commits into from
Feb 22, 2024
Merged

core(navigation-runner): only run getArtifact phase once #15827

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cc66351
core(navigation-runner): only run `getArtifact` phase once
adamraine Feb 22, 2024
1e895e4
ope
adamraine Feb 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions core/gather/gatherers/devtools-log.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,13 @@ class DevtoolsLog extends BaseGatherer {
driver.targetManager.off('protocolevent', this._onProtocolMessage);
}

/**
* @return {LH.Artifacts['DevtoolsLog']}
*/
getDebugData() {
return this._messageLog.messages;
}

/**
* @return {Promise<LH.Artifacts['DevtoolsLog']>}
*/
Expand Down
4 changes: 4 additions & 0 deletions core/gather/gatherers/trace.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,10 @@ class Trace extends BaseGatherer {
this._trace = await Trace.endTraceAndCollectEvents(driver.defaultSession);
}

getDebugData() {
return this._trace;
}

getArtifact() {
return this._trace;
}
Expand Down
34 changes: 11 additions & 23 deletions core/gather/navigation-runner.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,32 +110,20 @@ async function _navigate(navigationContext) {
* @return {Promise<{devtoolsLog?: LH.DevtoolsLog, records?: Array<LH.Artifacts.NetworkRequest>, trace?: LH.Trace}>}
*/
async function _collectDebugData(navigationContext, phaseState) {
const devtoolsLogArtifactDefn = phaseState.artifactDefinitions.find(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh boy this is a pleasant diff to witness :)

definition => definition.gatherer.instance.meta.symbol === DevtoolsLog.symbol
);
const traceArtifactDefn = phaseState.artifactDefinitions.find(
definition => definition.gatherer.instance.meta.symbol === Trace.symbol
);

const artifactDefinitions = [devtoolsLogArtifactDefn, traceArtifactDefn].filter(
/**
* @param {LH.Config.AnyArtifactDefn | undefined} defn
* @return {defn is LH.Config.AnyArtifactDefn}
*/
defn => Boolean(defn)
);
if (!artifactDefinitions.length) return {};

await collectPhaseArtifacts({...phaseState, phase: 'getArtifact', artifactDefinitions});
const getArtifactState = phaseState.artifactState.getArtifact;
let devtoolsLog;
let trace;

for (const definition of phaseState.artifactDefinitions) {
Copy link
Collaborator

@connorjclark connorjclark Feb 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did this change from a .find to a loop for some reason, or just preference?

I guess it got rid of the filter and is cleaner, jw.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's cleaner and made the type narrowing easier.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh! you did this b/c need to narrow types to use getDebugData

const {instance} = definition.gatherer;
if (instance instanceof DevtoolsLog) {
devtoolsLog = instance.getDebugData();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but... why not just getArtifact() ? types?

Copy link
Member Author

@adamraine adamraine Feb 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to separate the getArtifact logic from the getDebugInfo logic even if they happen to be the same. Right now when designing gatherers we assume that getArtifact is only run once at the end of gathering. If we were to just reuse getArtifact here, it would break that assumption.

It's really just defensive coding to prevent future regressions if the impl of dtlog/trace change.

} else if (instance instanceof Trace) {
trace = instance.getDebugData();
}
}

const devtoolsLogArtifactId = devtoolsLogArtifactDefn?.id;
const devtoolsLog = devtoolsLogArtifactId && (await getArtifactState[devtoolsLogArtifactId]);
const records = devtoolsLog && (await NetworkRecords.request(devtoolsLog, navigationContext));

const traceArtifactId = traceArtifactDefn?.id;
const trace = traceArtifactId && (await getArtifactState[traceArtifactId]);

return {devtoolsLog, records, trace};
}

Expand Down
26 changes: 20 additions & 6 deletions core/test/gather/navigation-runner-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -344,13 +344,27 @@ describe('NavigationRunner', () => {
});

it('finds page load errors in network records when available', async () => {
const {resolvedConfig, gatherers} = createMockConfig();
mocks.navigationMock.gotoURL.mockResolvedValue({mainDocumentUrl: requestedUrl, warnings: []});
mocks.navigationMock.gotoURL.mockReturnValue({
requestedUrl,
mainDocumentUrl: requestedUrl,
warnings: [],
});

const devtoolsLogInstance = new DevtoolsLogGatherer();
const traceInstance = new TraceGatherer();

// @ts-expect-error mock config
const resolvedConfig = /** @type {LH.Config.ResolvedConfig} */ ({
settings: JSON.parse(JSON.stringify(defaultSettings)),
artifacts: [
{id: 'DevtoolsLog', gatherer: {instance: devtoolsLogInstance}},
{id: 'Trace', gatherer: {instance: traceInstance}},
],
});

const devtoolsLog = networkRecordsToDevtoolsLog([{url: requestedUrl, failed: true}]);
gatherers.timespan.meta.symbol = DevtoolsLogGatherer.symbol;
gatherers.timespan.getArtifact = fnAny().mockResolvedValue(devtoolsLog);
gatherers.navigation.meta.symbol = TraceGatherer.symbol;
gatherers.navigation.getArtifact = fnAny().mockResolvedValue({traceEvents: []});
devtoolsLogInstance.getDebugData = fnAny().mockReturnValue(devtoolsLog);
traceInstance.getDebugData = fnAny().mockReturnValue({traceEvents: []});

const artifacts = await runner._navigation({
driver,
Expand Down
Loading