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.

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

core(fr): collect devtoolsLogs on pageLoadError #12980

Merged
merged 2 commits into from
Aug 25, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion .github/workflows/smoke.yml
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ jobs:

- run: sudo apt-get install xvfb
- name: yarn smoke --fraggle-rock
run: xvfb-run --auto-servernum yarn smoke --debug --fraggle-rock -j=1 --retries=2 --invert-match pwa offline oopif errors-infinite-loop errors-expired-ssl perf-diagnostics-third-party
run: xvfb-run --auto-servernum yarn smoke --debug --fraggle-rock -j=1 --retries=2 --invert-match pwa offline oopif perf-diagnostics-third-party

# Fail if any changes were written to source files.
- run: git diff --exit-code
Expand Down
7 changes: 1 addition & 6 deletions lighthouse-cli/test/smokehouse/report-assert.js
Original file line number Diff line number Diff line change
Expand Up @@ -290,12 +290,7 @@ function collateResults(localConsole, actual, expected) {
}

return [
{
name: 'final url',
actual: actual.lhr.finalUrl,
expected: expected.lhr.finalUrl,
equal: actual.lhr.finalUrl === expected.lhr.finalUrl,
},
makeComparison('final url', actual.lhr.finalUrl, expected.lhr.finalUrl),
runtimeErrorAssertion,
runWarningsAssertion,
...requestCountAssertion,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,12 @@ const infiniteLoop = {
artifacts: {
PageLoadError: {code: 'PAGE_HUNG'},
devtoolsLogs: {
'pageLoadError-defaultPass': NONEMPTY_ARRAY,
'pageLoadError-defaultPass': {...NONEMPTY_ARRAY, _legacyOnly: true},
'pageLoadError-default': {...NONEMPTY_ARRAY, _fraggleRockOnly: true},
},
traces: {
// Fraggle Rock treats traces as regular artifacts which are not collected on error.
'_legacyOnly': true,
'pageLoadError-defaultPass': {traceEvents: NONEMPTY_ARRAY},
},
},
Expand All @@ -46,9 +49,12 @@ const infiniteLoop = {
const expiredSsl = {
lhr: {
requestedUrl: 'https://expired.badssl.com',
finalUrl: 'https://expired.badssl.com/',
finalUrl: /(expired.badssl.com|chrome-error)/,
patrickhulce marked this conversation as resolved.
Show resolved Hide resolved
runtimeError: {code: 'INSECURE_DOCUMENT_REQUEST'},
runWarnings: ['The URL you have provided does not have a valid security certificate. net::ERR_CERT_DATE_INVALID'],
runWarnings: Object.defineProperty([
/expired.badssl.*redirected to chrome-error:/, // This warning was not provided in legacy reports.
'The URL you have provided does not have a valid security certificate. net::ERR_CERT_DATE_INVALID',
], '_fraggleRockOnly', {value: true, enumerable: true}),
audits: {
'first-contentful-paint': {
scoreDisplayMode: 'error',
Expand All @@ -59,9 +65,12 @@ const expiredSsl = {
artifacts: {
PageLoadError: {code: 'INSECURE_DOCUMENT_REQUEST'},
devtoolsLogs: {
'pageLoadError-defaultPass': NONEMPTY_ARRAY,
'pageLoadError-defaultPass': {...NONEMPTY_ARRAY, _legacyOnly: true},
'pageLoadError-default': {...NONEMPTY_ARRAY, _fraggleRockOnly: true},
},
traces: {
// Fraggle Rock treats traces as regular artifacts which are not collected on error.
'_legacyOnly': true,
'pageLoadError-defaultPass': {traceEvents: NONEMPTY_ARRAY},
},
},
Expand Down
19 changes: 12 additions & 7 deletions lighthouse-core/fraggle-rock/gather/navigation-runner.js
Original file line number Diff line number Diff line change
Expand Up @@ -105,9 +105,9 @@ async function _navigate(navigationContext) {
/**
* @param {NavigationContext} navigationContext
* @param {PhaseState} phaseState
* @return {Promise<Array<LH.Artifacts.NetworkRequest> | undefined>}
* @return {Promise<{devtoolsLog: LH.DevtoolsLog, records: Array<LH.Artifacts.NetworkRequest>} | undefined>}
*/
async function _collectNetworkRecords(navigationContext, phaseState) {
async function _collectNetworkData(navigationContext, phaseState) {
const devtoolsLogArtifactDefn = phaseState.artifactDefinitions.find(
definition => definition.gatherer.instance.meta.symbol === DevtoolsLog.symbol
);
Expand All @@ -119,7 +119,7 @@ async function _collectNetworkRecords(navigationContext, phaseState) {

const devtoolsLog = await phaseState.artifactState.getArtifact[devtoolsLogArtifactId];
const records = await NetworkRecords.request(devtoolsLog, navigationContext);
return records;
return {devtoolsLog, records};
}

/**
Expand All @@ -137,12 +137,12 @@ async function _computeNavigationResult(
) {
const {navigationError, finalUrl} = navigateResult;
const warnings = [...setupResult.warnings, ...navigateResult.warnings];
const networkRecords = await _collectNetworkRecords(navigationContext, phaseState);
const pageLoadError = networkRecords
const networkData = await _collectNetworkData(navigationContext, phaseState);
const pageLoadError = networkData
? getPageLoadError(navigationError, {
url: finalUrl,
loadFailureMode: navigationContext.navigation.loadFailureMode,
networkRecords,
networkRecords: networkData.records,
})
: navigationError;

Expand All @@ -151,10 +151,15 @@ async function _computeNavigationResult(
const localizedMessage = i18n.getFormatted(pageLoadError.friendlyMessage, locale);
log.error('NavigationRunner', localizedMessage, navigationContext.requestedUrl);

/** @type {Partial<LH.GathererArtifacts>} */
const artifacts = {};
const pageLoadErrorId = `pageLoadError-${navigationContext.navigation.id}`;
if (networkData) artifacts.devtoolsLogs = {[pageLoadErrorId]: networkData.devtoolsLog};

return {
finalUrl,
pageLoadError,
artifacts: {},
artifacts,
warnings: [...warnings, pageLoadError.friendlyMessage],
};
} else {
Expand Down
5 changes: 1 addition & 4 deletions lighthouse-core/gather/driver/navigation.js
Original file line number Diff line number Diff line change
Expand Up @@ -142,10 +142,7 @@ function getNavigationWarnings(navigation) {

if (navigation.timedOut) warnings.push(str_(UIStrings.warningTimeout));

if (
!URL.equalWithExcludedFragments(requestedUrl, finalUrl) &&
!finalUrl.startsWith('chrome-error://')
) {
if (!URL.equalWithExcludedFragments(requestedUrl, finalUrl)) {
warnings.push(str_(UIStrings.warningRedirected, {
requested: requestedUrl,
final: finalUrl,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -336,7 +336,7 @@ describe('NavigationRunner', () => {

const {artifacts, pageLoadError} = await run(navigation);
expect(pageLoadError).toBeInstanceOf(LighthouseError);
expect(artifacts).toEqual({});
expect(artifacts).toEqual({devtoolsLogs: {'pageLoadError-default': expect.any(Array)}});
});

it('cleans up throttling before getArtifact', async () => {
Expand Down
4 changes: 2 additions & 2 deletions lighthouse-core/test/gather/driver/navigation-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -234,9 +234,9 @@ describe('.getNavigationWarnings()', () => {
expect(warnings).toHaveLength(0);
});

it('does not add a url mismatch warning for failed navigations', () => {
it('adds a url mismatch warning for failed navigations', () => {
const finalUrl = 'chrome-error://chromewebdata/';
const warnings = getNavigationWarnings({...normalNavigation, finalUrl});
expect(warnings).toHaveLength(0);
expect(warnings).toHaveLength(1);
});
});
2 changes: 1 addition & 1 deletion types/smokehouse.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ declare global {
interface ExpectedLHR {
audits: Record<string, any>;
requestedUrl: string;
finalUrl: string;
finalUrl: string | RegExp;
runWarnings?: Array<string|RegExp>;
runtimeError?: {
code?: any;
Expand Down