Skip to content

report: fix compat for older lighthouse reports #14617

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

Merged
merged 4 commits into from
Jan 23, 2023
Merged

report: fix compat for older lighthouse reports #14617

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9cc515b
report: fix compat for older lighthouse reports
connorjclark Dec 15, 2022
122daac
oop
connorjclark Jan 12, 2023
1ee0b01
update
connorjclark Jan 12, 2023
aab0932
Merge remote-tracking branch 'origin/main' into report-compat
connorjclark Jan 23, 2023
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
18 changes: 18 additions & 0 deletions core/util.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,10 @@ class Util {
}
}
}

// TODO: convert printf-style displayValue.
Copy link
Member

Choose a reason for hiding this comment

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

Is this TODO for this PR or later?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's an optional, p100 thing :P just something I noticed and wanted to document in the relevant place.

// Added: #5099, v3
// Removed: #6767, v4
}
}

Expand Down Expand Up @@ -216,6 +220,20 @@ class Util {
});
}

// Add some minimal stuff so older reports still work.
if (!clone.environment) {
// @ts-expect-error
clone.environment = {benchmarkIndex: 0};
}
if (!clone.configSettings.screenEmulation) {
// @ts-expect-error
clone.configSettings.screenEmulation = {};
}
if (!clone.i18n) {
// @ts-expect-error
clone.i18n = {};
}

// In 10.0, full-page-screenshot became a top-level property on the LHR.
if (clone.audits['full-page-screenshot']) {
const details = /** @type {LH.Result.FullPageScreenshot=} */ (
Expand Down
18 changes: 18 additions & 0 deletions report/renderer/util.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,10 @@ class Util {
}
}
}

// TODO: convert printf-style displayValue.
// Added: #5099, v3
// Removed: #6767, v4
}
}

Expand Down Expand Up @@ -212,6 +216,20 @@ class Util {
});
}

// Add some minimal stuff so older reports still work.
if (!clone.environment) {
// @ts-expect-error
clone.environment = {benchmarkIndex: 0};
}
if (!clone.configSettings.screenEmulation) {
// @ts-expect-error
clone.configSettings.screenEmulation = {};
}
if (!clone.i18n) {
// @ts-expect-error
clone.i18n = {};
}

// In 10.0, full-page-screenshot became a top-level property on the LHR.
if (clone.audits['full-page-screenshot']) {
const details = /** @type {LH.Result.FullPageScreenshot=} */ (
Expand Down
2,332 changes: 2,332 additions & 0 deletions report/test-assets/lhr-3.0.0.json
View file
Open in desktop

Large diffs are not rendered by default.

4,754 changes: 4,754 additions & 0 deletions report/test-assets/lhr-4.3.0.json
View file
Open in desktop

Large diffs are not rendered by default.

5,994 changes: 5,994 additions & 0 deletions report/test-assets/lhr-5.0.0.json
View file
Open in desktop

Large diffs are not rendered by default.

7,222 changes: 7,222 additions & 0 deletions report/test-assets/lhr-6.0.0.json
View file
Open in desktop

Large diffs are not rendered by default.

8,679 changes: 8,679 additions & 0 deletions report/test-assets/lhr-8.5.0.json
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 2 additions & 0 deletions viewer/app/src/lighthouse-report-viewer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -340,6 +340,8 @@ export class LighthouseReportViewer {
} catch (err) {
logger.error(err.message);
}

document.dispatchEvent(new CustomEvent('lh-file-upload-test-ack'));
}

/**
Expand Down
58 changes: 48 additions & 10 deletions viewer/test/viewer-test-pptr.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ describe('Lighthouse Viewer', () => {
let browser;
/** @type {import('puppeteer').Page} */
let viewerPage;
const pageErrors = [];
let pageErrors = [];

const selectors = {
audits: '.lh-audit, .lh-metric',
Expand Down Expand Up @@ -69,16 +69,29 @@ describe('Lighthouse Viewer', () => {
});

after(async function() {
// Log any page load errors encountered in case before() failed.
// eslint-disable-next-line no-console
if (pageErrors.length > 0) console.error(pageErrors);

await Promise.all([
server.close(),
browser && browser.close(),
]);
});

beforeEach(async function() {
pageErrors = [];
});

async function ensureNoErrors() {
await viewerPage.evaluate(() => new Promise(window.requestAnimationFrame));
const theErrors = pageErrors;
pageErrors = [];
expect(theErrors).toHaveLength(0);
}

afterEach(async function() {
// Tests should call this themselves so the failure is associated with them in the test report,
// but just in case one is missed it won't hurt to repeat the check here.
await ensureNoErrors();
});

describe('Renders the flow report', () => {
before(async () => {
await viewerPage.goto(viewerUrl, {waitUntil: 'networkidle2', timeout: 30000});
Expand All @@ -88,7 +101,7 @@ describe('Lighthouse Viewer', () => {
});

it('should load with no errors', async () => {
assert.deepStrictEqual(pageErrors, []);
await ensureNoErrors();
});

it('renders the summary page', async () => {
Expand All @@ -100,7 +113,7 @@ describe('Lighthouse Viewer', () => {
);
assert.equal(scores.length, 14);

assert.deepStrictEqual(pageErrors, []);
await ensureNoErrors();
});
});

Expand All @@ -113,7 +126,7 @@ describe('Lighthouse Viewer', () => {
});

it('should load with no errors', async () => {
assert.deepStrictEqual(pageErrors, []);
await ensureNoErrors();
});

it('should contain all categories', async () => {
Expand Down Expand Up @@ -251,6 +264,30 @@ describe('Lighthouse Viewer', () => {
});
});

describe('Renders old reports', () => {
[
'lhr-3.0.0.json',
'lhr-4.3.0.json',
'lhr-5.0.0.json',
'lhr-6.0.0.json',
'lhr-8.5.0.json',
].forEach((testFilename) => {
it(`[${testFilename}] should load with no errors`, async () => {
await viewerPage.goto(viewerUrl, {waitUntil: 'networkidle2', timeout: 30000});
const fileInput = await viewerPage.$('#hidden-file-input');
const waitForAck = viewerPage.evaluate(() =>
new Promise(resolve =>
document.addEventListener('lh-file-upload-test-ack', resolve, {once: true})));
await fileInput.uploadFile(`${LH_ROOT}/report/test-assets/${testFilename}`);
Comment on lines +278 to +281
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this promise structure guarantees that the event listener is installed by the time we call fileInput.uploadFile.

Copy link
Collaborator Author

@connorjclark connorjclark Jan 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Protocol commands are executed in sequence, so why wouldn't this be alright? the callback inside the promise is a microtask, but I think even then it runs on the main thread and so would any commands called by fileInput.upload so should be all good

Copy link
Member

Choose a reason for hiding this comment

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

Protocol commands are executed in sequence

Interesting, did not know this. This is probably fine then.

await Promise.race([
waitForAck,
new Promise((resolve, reject) => setTimeout(reject, 5_000)),
]);
await ensureNoErrors();
});
});
});

describe('PSI', () => {
/** @type {Partial<puppeteer.ResponseForRequest>} */
let interceptedRequest;
Expand Down Expand Up @@ -351,7 +388,7 @@ describe('Lighthouse Viewer', () => {
expect(interceptedUrl.searchParams.getAll('category').sort()).toEqual(defaultCategories);

// No errors.
assert.deepStrictEqual(pageErrors, []);
await ensureNoErrors();

// All categories.
const categoryElementIds = await getCategoryElementsIds();
Expand Down Expand Up @@ -395,7 +432,7 @@ describe('Lighthouse Viewer', () => {
});

// No errors.
assert.deepStrictEqual(pageErrors, []);
await ensureNoErrors();
});

it('should handle errors from the API', async () => {
Expand All @@ -412,6 +449,7 @@ describe('Lighthouse Viewer', () => {
// One error.
expect(pageErrors).toHaveLength(1);
expect(pageErrors[0].message).toContain('badPsiResponse error');
pageErrors = [];
});
});
});