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

feat: use finalDisplayedUrl #901

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 6 additions & 1 deletion packages/cli/src/assert/assert.js
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,12 @@ async function runCommand(options) {
if (budgetsFile) options = await convertBudgetsToAssertions(readBudgets(budgetsFile));

const lhrs = loadSavedLHRs().map(json => JSON.parse(json));
const uniqueUrls = new Set(lhrs.map(lhr => lhr.finalUrl));

// eslint-disable-next-line prettier/prettier
const {upgradeLhrForCompatibility} = await import('lighthouse/core/lib/lighthouse-compatibility.js');
lhrs.forEach(upgradeLhrForCompatibility);

const uniqueUrls = new Set(lhrs.map(lhr => lhr.finalDisplayedUrl));
const allResults = getAllAssertionResults(options, lhrs);
const groupedResults = _.groupBy(allResults, result => result.url);

Expand Down
2 changes: 1 addition & 1 deletion packages/cli/src/collect/node-runner.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ class LighthouseRunner {
/** @type {LH.Result} */
const lhr = JSON.parse(process.env.LHCITEST_MOCK_LHR || '{}');
lhr.requestedUrl = url;
lhr.finalUrl = url;
lhr.finalDisplayedUrl = url;
lhr.configSettings = lhr.configSettings || options.settings || {};
return Promise.resolve(JSON.stringify(lhr));
}
Expand Down
11 changes: 8 additions & 3 deletions packages/cli/src/open/open.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,13 @@ function buildCommand(yargs) {
async function runCommand(options) {
/** @type {Array<LH.Result>} */
const lhrs = loadSavedLHRs().map(lhr => JSON.parse(lhr));

// eslint-disable-next-line prettier/prettier
const {upgradeLhrForCompatibility} = await import('lighthouse/core/lib/lighthouse-compatibility.js');
lhrs.forEach(upgradeLhrForCompatibility);

/** @type {Array<Array<[LH.Result, LH.Result]>>} */
const groupedByUrl = _.groupBy(lhrs, lhr => lhr.finalUrl).map(lhrs =>
const groupedByUrl = _.groupBy(lhrs, lhr => lhr.finalDisplayedUrl).map(lhrs =>
lhrs.map(lhr => [lhr, lhr])
);
const representativeLhrs = computeRepresentativeRuns(groupedByUrl);
Expand All @@ -41,9 +46,9 @@ async function runCommand(options) {
const targetUrls = typeof options.url === 'string' ? [options.url] : options.url || [];

for (const lhr of representativeLhrs) {
if (targetUrls.length && !targetUrls.includes(lhr.finalUrl)) continue;
if (targetUrls.length && !targetUrls.includes(lhr.finalDisplayedUrl)) continue;

process.stdout.write(`Opening median report for ${lhr.finalUrl}...\n`);
process.stdout.write(`Opening median report for ${lhr.finalDisplayedUrl}...\n`);
const tmpFile = tmp.fileSync({postfix: '.html'});
fs.writeFileSync(tmpFile.name, await getHTMLReportForLHR(lhr));
await open(tmpFile.name);
Expand Down
43 changes: 35 additions & 8 deletions packages/cli/src/upload/upload.js
Original file line number Diff line number Diff line change
Expand Up @@ -273,14 +273,21 @@ async function runGithubStatusCheck(options, targetUrlMap) {
} else {
/** @type {Array<LH.Result>} */
const lhrs = loadSavedLHRs().map(lhr => JSON.parse(lhr));

// eslint-disable-next-line prettier/prettier
const {upgradeLhrForCompatibility} = await import('lighthouse/core/lib/lighthouse-compatibility.js');
lhrs.forEach(upgradeLhrForCompatibility);

/** @type {Array<Array<[LH.Result, LH.Result]>>} */
const lhrsByUrl = _.groupBy(lhrs, lhr => lhr.finalUrl).map(lhrs => lhrs.map(lhr => [lhr, lhr]));
const lhrsByUrl = _.groupBy(lhrs, lhr => lhr.finalDisplayedUrl).map(lhrs =>
lhrs.map(lhr => [lhr, lhr])
);
const representativeLhrs = computeRepresentativeRuns(lhrsByUrl);

if (!representativeLhrs.length) return print('No LHRs for status check, skipping.\n');

for (const lhr of representativeLhrs) {
const rawUrl = lhr.finalUrl;
const rawUrl = lhr.finalDisplayedUrl;
const urlLabel = getUrlLabelForGithub(rawUrl, options);
const state = 'success';
const context = getGitHubContext(urlLabel, options);
Expand Down Expand Up @@ -434,7 +441,12 @@ async function runLHCITarget(options) {

for (const lhr of lhrs) {
const parsedLHR = JSON.parse(lhr);
const url = getUrlForLhciTarget(parsedLHR.finalUrl, options);

// eslint-disable-next-line prettier/prettier
const {upgradeLhrForCompatibility} = await import('lighthouse/core/lib/lighthouse-compatibility.js');
upgradeLhrForCompatibility(parsedLHR);

const url = getUrlForLhciTarget(parsedLHR.finalDisplayedUrl, options);
const run = await api.createRun({
projectId: project.id,
buildId: build.id,
Expand All @@ -444,7 +456,7 @@ async function runLHCITarget(options) {
});

buildViewUrl.searchParams.set('compareUrl', url);
targetUrlMap.set(parsedLHR.finalUrl, buildViewUrl.href);
targetUrlMap.set(parsedLHR.finalDisplayedUrl, buildViewUrl.href);
print(`Saved LHR to ${options.serverBaseUrl} (${run.id})\n`);
}

Expand All @@ -464,15 +476,22 @@ async function runLHCITarget(options) {
async function runTemporaryPublicStorageTarget(options) {
/** @type {Array<LH.Result>} */
const lhrs = loadSavedLHRs().map(lhr => JSON.parse(lhr));

// eslint-disable-next-line prettier/prettier
const {upgradeLhrForCompatibility} = await import('lighthouse/core/lib/lighthouse-compatibility.js');
lhrs.forEach(upgradeLhrForCompatibility);

/** @type {Array<Array<[LH.Result, LH.Result]>>} */
const lhrsByUrl = _.groupBy(lhrs, lhr => lhr.finalUrl).map(lhrs => lhrs.map(lhr => [lhr, lhr]));
const lhrsByUrl = _.groupBy(lhrs, lhr => lhr.finalDisplayedUrl).map(lhrs =>
lhrs.map(lhr => [lhr, lhr])
);
const representativeLhrs = computeRepresentativeRuns(lhrsByUrl);
const targetUrlMap = new Map();

const previousUrlMap = await getPreviousUrlMap(options);

for (const lhr of representativeLhrs) {
print(`Uploading median LHR of ${lhr.finalUrl}...`);
print(`Uploading median LHR of ${lhr.finalDisplayedUrl}...`);

try {
const response = await fetch(TEMPORARY_PUBLIC_STORAGE_URL, {
Expand All @@ -483,10 +502,13 @@ async function runTemporaryPublicStorageTarget(options) {

const {success, url} = await response.json();
if (success && url) {
const urlReplaced = replaceUrlPatterns(lhr.finalUrl, options.urlReplacementPatterns);
const urlReplaced = replaceUrlPatterns(
lhr.finalDisplayedUrl,
options.urlReplacementPatterns
);
const urlToLinkTo = buildTemporaryStorageLink(url, urlReplaced, previousUrlMap);
print(`success!\nOpen the report at ${urlToLinkTo}\n`);
targetUrlMap.set(lhr.finalUrl, url);
targetUrlMap.set(lhr.finalDisplayedUrl, url);
} else {
print(`failed!\n`);
}
Expand Down Expand Up @@ -526,6 +548,11 @@ function getFileOutputPath(pattern, context) {
async function runFilesystemTarget(options) {
/** @type {Array<LH.Result>} */
const lhrs = loadSavedLHRs().map(lhr => JSON.parse(lhr));

// eslint-disable-next-line prettier/prettier
const {upgradeLhrForCompatibility} = await import('lighthouse/core/lib/lighthouse-compatibility.js');
lhrs.forEach(upgradeLhrForCompatibility);

/** @type {Array<Array<[LH.Result, LH.Result]>>} */
const lhrsByUrl = _.groupBy(lhrs, lhr => lhr.requestedUrl).map(lhrs =>
lhrs.map(lhr => [lhr, lhr])
Expand Down
2 changes: 1 addition & 1 deletion packages/cli/test/fixtures/psi/mock-psi-server.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ async function createApp() {

const url = req.query.url;
const lighthouseResult = JSON.parse(JSON.stringify(LHR));
lighthouseResult.finalUrl = url;
lighthouseResult.finalDisplayedUrl = url;
lighthouseResult.initialUrl = url;
setTimeout(() => res.json({lighthouseResult}), 2000);
});
Expand Down
2 changes: 1 addition & 1 deletion packages/cli/test/upload-url-hash.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ describe('Lighthouse CI upload filesystem reports with url hash', () => {
const fakeLhrPath = path.join(lighthouseciDir, 'lhr-12345.json');

const writeLhr = () => {
const fakeLhr = {finalUrl: 'foo.com', categories: {}, audits: {}};
const fakeLhr = {finalDisplayedUrl: 'foo.com', categories: {}, audits: {}};
fakeLhr.categories.pwa = {score: 0};
fakeLhr.categories.performance = {score: 0};
fakeLhr.audits['performance-budget'] = {score: 0};
Expand Down
4 changes: 2 additions & 2 deletions packages/cli/test/upload.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ describe('Lighthouse CI upload CLI', () => {
const fakeLhrPath = path.join(lighthouseciDir, 'lhr-12345.json');

const writeLhr = () => {
const fakeLhr = {finalUrl: 'foo.com', categories: {}, audits: {}};
const fakeLhr = {finalDisplayedUrl: 'foo.com', categories: {}, audits: {}};
fakeLhr.categories.pwa = {score: 0};
fakeLhr.categories.performance = {score: 0};
fakeLhr.audits['performance-budget'] = {score: 0};
Expand Down Expand Up @@ -125,7 +125,7 @@ describe('Lighthouse CI upload CLI', () => {

it('should upload for a build with very long URL', async () => {
const lhr = JSON.parse(fs.readFileSync(fakeLhrPath, 'utf8'));
lhr.finalUrl = `http://localhost/reall${'l'.repeat(256)}y-long-url`;
lhr.finalDisplayedUrl = `http://localhost/reall${'l'.repeat(256)}y-long-url`;
fs.writeFileSync(fakeLhrPath, JSON.stringify(lhr));

expect(await apiClient.getBuilds(project.id)).toHaveLength(1);
Expand Down
4 changes: 2 additions & 2 deletions packages/server/test/server-test-suite.js
Original file line number Diff line number Diff line change
Expand Up @@ -596,7 +596,7 @@ function runTests(state) {
describe('/:projectId/builds/:buildId/runs', () => {
const lhr = {
lighthouseVersion: '4.1.0',
finalUrl: 'https://example.com/',
finalDisplayedUrl: 'https://example.com/',
audits: {
interactive: {numericValue: 5000},
'speed-index': {numericValue: 5000},
Expand Down Expand Up @@ -697,7 +697,7 @@ function runTests(state) {
buildId: buildA.id,
url: 'https://example.com:PORT/blog',
lhr: JSON.stringify({
finalUrl: 'https://example.com/blog',
finalDisplayedUrl: 'https://example.com/blog',
lighthouseVersion: '4.2.0',
audits: {
interactive: {numericValue: 1000},
Expand Down
8 changes: 4 additions & 4 deletions packages/utils/src/assertions.js
Original file line number Diff line number Diff line change
Expand Up @@ -284,7 +284,7 @@ function getCategoryAssertionResults(auditProperty, assertionOptions, lhrs) {
* @return {boolean}
*/
function doesLHRMatchPattern(pattern, lhr) {
return new RegExp(pattern).test(lhr.finalUrl);
return new RegExp(pattern).test(lhr.finalDisplayedUrl);
}

/**
Expand Down Expand Up @@ -364,7 +364,7 @@ function resolveAssertionOptionsAndLhrs(baseOptions, unfilteredLhrs) {
: unfilteredLhrs;

// Double-check we've only got one URL to look at that should have been pre-grouped in `getAllAssertionResults`.
const uniqueURLs = new Set(lhrs.map(lhr => lhr.finalUrl));
const uniqueURLs = new Set(lhrs.map(lhr => lhr.finalDisplayedUrl));
if (uniqueURLs.size > 1) throw new Error('Can only assert one URL at a time!');

const medianLhrs = computeRepresentativeRuns([
Expand Down Expand Up @@ -399,7 +399,7 @@ function resolveAssertionOptionsAndLhrs(baseOptions, unfilteredLhrs) {
medianLhrs,
aggregationMethod,
lhrs: lhrs,
url: (lhrs[0] && lhrs[0].finalUrl) || '',
url: (lhrs[0] && lhrs[0].finalDisplayedUrl) || '',
};
}

Expand Down Expand Up @@ -456,7 +456,7 @@ function getAllAssertionResultsForUrl(baseOptions, unfilteredLhrs) {
* @return {LHCI.AssertResults.AssertionResult[]}
*/
function getAllAssertionResults(options, lhrs) {
const groupedByURL = _.groupBy(lhrs, lhr => lhr.finalUrl);
const groupedByURL = _.groupBy(lhrs, lhr => lhr.finalDisplayedUrl);

/** @type {LHCI.AssertCommand.BaseOptions[]} */
let arrayOfOptions = [options];
Expand Down
2 changes: 1 addition & 1 deletion packages/utils/src/seed-data/dataset-generator.js
Original file line number Diff line number Diff line change
Expand Up @@ -537,7 +537,7 @@ function createLoadTestDataset() {
/** @type {LH.Result} */
const lhr = JSON.parse(sourceLhr);
lhr.requestedUrl = url;
lhr.finalUrl = url;
lhr.finalDisplayedUrl = url;
for (const auditId of Object.keys(lhr.audits)) {
const multiplier = 1 + Math.random() * 0.4 - 0.2;
const audit = lhr.audits[auditId];
Expand Down
1 change: 1 addition & 0 deletions packages/utils/src/seed-data/lhr-generator.js
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,7 @@ function createLHR(pageUrl, auditDefs, prandom) {
return {
requestedUrl: pageUrl,
finalUrl: pageUrl,
finalDisplayedUrl: pageUrl,
categories,
audits,

Expand Down
20 changes: 10 additions & 10 deletions packages/utils/test/assertions.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ describe('getAllAssertionResults', () => {
beforeEach(() => {
lhrs = [
{
finalUrl: 'http://page-1.com',
finalDisplayedUrl: 'http://page-1.com',
categories: {pwa: {score: 0.5}, perf: {score: 0.1}},
audits: {
'first-contentful-paint': {
Expand All @@ -36,7 +36,7 @@ describe('getAllAssertionResults', () => {
},
},
{
finalUrl: 'http://page-1.com',
finalDisplayedUrl: 'http://page-1.com',
categories: {pwa: {score: 0.8}, perf: {score: 0.1}},
audits: {
'first-contentful-paint': {
Expand Down Expand Up @@ -402,23 +402,23 @@ describe('getAllAssertionResults', () => {
const lhrs = [
// This is the "median-run" by FCP and interactive.
{
finalUrl: 'http://example.com',
finalDisplayedUrl: 'http://example.com',
audits: {
'first-contentful-paint': {numericValue: 5000},
interactive: {numericValue: 10000},
'other-audit': {numericValue: 23000},
},
},
{
finalUrl: 'http://example.com',
finalDisplayedUrl: 'http://example.com',
audits: {
'first-contentful-paint': {numericValue: 1000},
interactive: {numericValue: 5000},
'other-audit': {numericValue: 5000},
},
},
{
finalUrl: 'http://example.com',
finalDisplayedUrl: 'http://example.com',
audits: {
'first-contentful-paint': {numericValue: 10000},
interactive: {numericValue: 15000},
Expand Down Expand Up @@ -567,7 +567,7 @@ describe('getAllAssertionResults', () => {

beforeEach(() => {
lhrWithBudget = {
finalUrl: 'http://page-1.com',
finalDisplayedUrl: 'http://page-1.com',
audits: {
'performance-budget': {
id: 'performance-budget',
Expand Down Expand Up @@ -598,7 +598,7 @@ describe('getAllAssertionResults', () => {
};

lhrWithResourceSummary = {
finalUrl: 'http://example.com',
finalDisplayedUrl: 'http://example.com',
audits: {
'resource-summary': {
details: {
Expand Down Expand Up @@ -897,7 +897,7 @@ describe('getAllAssertionResults', () => {

beforeEach(() => {
lhrWithUserTimings = {
finalUrl: 'http://example.com',
finalDisplayedUrl: 'http://example.com',
audits: {
'user-timings': {
details: {
Expand Down Expand Up @@ -994,7 +994,7 @@ describe('getAllAssertionResults', () => {
describe('URL-grouping', () => {
beforeEach(() => {
for (const lhr of [...lhrs]) {
lhrs.push({...lhr, finalUrl: 'http://page-2.com'});
lhrs.push({...lhr, finalDisplayedUrl: 'http://page-2.com'});
}
});

Expand Down Expand Up @@ -1059,7 +1059,7 @@ describe('getAllAssertionResults', () => {
describe('assertMatrix', () => {
beforeEach(() => {
for (const lhr of [...lhrs]) {
lhrs.push({...lhr, finalUrl: 'http://page-2.com'});
lhrs.push({...lhr, finalDisplayedUrl: 'http://page-2.com'});
}
});

Expand Down
4 changes: 2 additions & 2 deletions packages/utils/test/psi-client.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ describe('PSI API Client', () => {
const apiKey = 'the-key';

it('should request the url', async () => {
const lighthouseResult = {finalUrl: 'https://example.com/'};
const lighthouseResult = {finalDisplayedUrl: 'https://example.com/'};
fetchJsonMock = jest.fn().mockResolvedValue({lighthouseResult});
const fetchMock = jest.fn().mockImplementation(fetchMockImpl);
const client = new PsiClient({apiKey, fetch: fetchMock});
Expand All @@ -39,7 +39,7 @@ describe('PSI API Client', () => {
});

it('should respect provided options', async () => {
const lighthouseResult = {finalUrl: 'https://example.com/'};
const lighthouseResult = {finalDisplayedUrl: 'https://example.com/'};
fetchJsonMock = jest.fn().mockResolvedValue({lighthouseResult});
const fetchMock = jest.fn().mockImplementation(fetchMockImpl);
const runOptions = {strategy: 'desktop', categories: ['accessibility', 'pwa', 'seo']};
Expand Down
2 changes: 1 addition & 1 deletion packages/utils/test/seed-data/lhr-generator.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ describe('createLHR', () => {

expect(lhr).toMatchObject({
requestedUrl: 'http://example.com',
finalUrl: 'http://example.com',
finalDisplayedUrl: 'http://example.com',
audits: {
'seo-audit': {
title: 'Seo Audit',
Expand Down
6 changes: 3 additions & 3 deletions packages/viewer/src/ui/components/report-upload-box.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,8 @@ export function parseStringAsLhr(s) {

/** @param {LH.Result} lhrA @param {LH.Result} lhrB @return {DisplayType} */
export function computeBestDisplayType(lhrA, lhrB) {
const urlA = new URL(lhrA.finalUrl);
const urlB = new URL(lhrB.finalUrl);
const urlA = new URL(lhrA.finalDisplayedUrl);
const urlB = new URL(lhrB.finalDisplayedUrl);
if (urlA.hostname !== urlB.hostname) return 'hostname';
if (urlA.pathname !== urlB.pathname) return 'pathname';
if (urlA.search !== urlB.search) return 'path';
Expand All @@ -50,7 +50,7 @@ export function computeBestDisplayType(lhrA, lhrB) {
/** @param {{report: ReportData, displayType: DisplayType}} props */
const FilePill = props => {
const {filename, lhr} = props.report;
const url = new URL(lhr.finalUrl);
const url = new URL(lhr.finalDisplayedUrl);
const timestamp = new Date(lhr.fetchTime).toLocaleString();
const options = {
filename,
Expand Down