-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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): add base fraggle rock snapshot runner #11748
Changes from all commits
14fdb68
a79b8a7
7525e87
a7a860d
13ba78c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,82 @@ | ||
/** | ||
* @license Copyright 2020 The Lighthouse Authors. All Rights Reserved. | ||
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0 | ||
* Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License. | ||
*/ | ||
'use strict'; | ||
|
||
const Driver = require('./gather/driver.js'); | ||
const Runner = require('../runner.js'); | ||
const Config = require('../config/config.js'); | ||
|
||
/** | ||
* @param {LH.Gatherer.GathererInstance} gatherer | ||
* @return {gatherer is LH.Gatherer.FRGathererInstance} | ||
*/ | ||
function isFRGatherer(gatherer) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are all FR gatherers going to support snapshot mode? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, not all, see full compat table for which ones, but the next step after #11759 is the TODO that's here about using gatherer.meta to annotate whether a gatherer supports it or not. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The name There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Excellent, that's exactly what it's supposed to be doing :) purely a type guard on whether a gatherer instance supports the FR gatherer properties.
I consider this function not detecting snapshot support at all which is why the TODO is there. Any suggestion for alternative wording there to make it clearer? Snapshot detection support is 2-3 PRs away from being fixed and won't affect any results in the meantime. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this issue has more to do with me misunderstanding the comment on L59 than the comment's wording. It's fine to leave as is :) |
||
// TODO(FR-COMPAT): use configuration on gatherer.meta to detect interface compatibility | ||
return gatherer.name === 'Accessibility'; | ||
} | ||
|
||
/** @param {{page: import('puppeteer').Page, config?: LH.Config.Json}} options */ | ||
async function snapshot(options) { | ||
const config = new Config(options.config); | ||
const driver = new Driver(options.page); | ||
await driver.connect(); | ||
|
||
const url = await options.page.url(); | ||
|
||
return Runner.run( | ||
async () => { | ||
/** @type {LH.BaseArtifacts} */ | ||
const baseArtifacts = { | ||
fetchTime: new Date().toJSON(), | ||
LighthouseRunWarnings: [], | ||
URL: {requestedUrl: url, finalUrl: url}, | ||
Timing: [], | ||
Stacks: [], | ||
settings: config.settings, | ||
// TODO(FR-COMPAT): convert these to regular artifacts | ||
HostFormFactor: 'mobile', | ||
TestedAsMobileDevice: true, | ||
HostUserAgent: 'unknown', | ||
patrickhulce marked this conversation as resolved.
Show resolved
Hide resolved
|
||
NetworkUserAgent: 'unknown', | ||
BenchmarkIndex: 0, | ||
InstallabilityErrors: {errors: []}, | ||
traces: {}, | ||
devtoolsLogs: {}, | ||
WebAppManifest: null, | ||
PageLoadError: null, | ||
}; | ||
|
||
const gatherers = (config.passes || []) | ||
.flatMap(pass => pass.gatherers); | ||
|
||
/** @type {Partial<LH.GathererArtifacts>} */ | ||
const artifacts = {}; | ||
|
||
for (const {instance} of gatherers) { | ||
// TODO(FR-COMPAT): use configuration on gatherer.meta to detect snapshot support | ||
if (!isFRGatherer(instance)) continue; | ||
|
||
/** @type {keyof LH.GathererArtifacts} */ | ||
const artifactName = instance.name; | ||
const artifact = await Promise.resolve() | ||
patrickhulce marked this conversation as resolved.
Show resolved
Hide resolved
|
||
.then(() => instance.afterPass({driver})) | ||
.catch(err => err); | ||
|
||
artifacts[artifactName] = artifact; | ||
} | ||
|
||
return /** @type {LH.Artifacts} */ ({...baseArtifacts, ...artifacts}); // Cast to drop Partial<> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I assume you're already thinking about how these types are mostly useless (sometimes worse than useless) in a world where we can't usually assume they're all actually there... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Important to note this section was just copied from our existing gathering so it's not a degradation :) I have not invested significant time on ideating how to improve the types here, no. I don't really consider the pre-existing temporary lie of |
||
}, | ||
{ | ||
url, | ||
config, | ||
} | ||
); | ||
} | ||
|
||
module.exports = { | ||
snapshot, | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,40 @@ | ||
<!doctype html> | ||
<!-- | ||
* Copyright 2020 The Lighthouse Authors. All Rights Reserved. | ||
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0 | ||
* Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License. | ||
--> | ||
<html lang="en"> | ||
<head> | ||
<title>Interactive Onclick Tester</title> | ||
<meta charset="utf-8"> | ||
<meta name="viewport" content="width=device-width, initial-scale=1.0, minimum-scale=1.0"> | ||
</head> | ||
<body> | ||
Hello, Fraggle Rock! | ||
|
||
This page has no accessibility violations until the mouse is clicked. | ||
|
||
<template id="click-target"> | ||
<button>Click to add violations</button> | ||
</template> | ||
|
||
<template id="violations"> | ||
<input type="text" /> | ||
</template> | ||
|
||
<script> | ||
function addTemplate(selector) { | ||
/** @type {HTMLTemplateElement} */ | ||
const template = document.querySelector(selector); | ||
document.body.appendChild(template.content.cloneNode(true)); | ||
} | ||
|
||
document.addEventListener('click', () => { | ||
addTemplate('#violations'); | ||
}); | ||
|
||
addTemplate('#click-target'); | ||
</script> | ||
</body> | ||
</html> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,84 @@ | ||
/** | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if this ends up expanding to a bunch of new smoke tests, are you thinking about moving them somewhere else/called somehow else instead of in the midst of all the (mostly) unit tests? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. called somehow else, yes definitely 👍 moved somewhere else, no wasn't planning on it. |
||
* @license Copyright 2020 The Lighthouse Authors. All Rights Reserved. | ||
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0 | ||
* Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License. | ||
*/ | ||
'use strict'; | ||
|
||
/* eslint-env jest */ | ||
|
||
const path = require('path'); | ||
const lighthouse = require('../../fraggle-rock/api.js'); | ||
const puppeteer = require('puppeteer'); | ||
const StaticServer = require('../../../lighthouse-cli/test/fixtures/static-server.js').Server; | ||
|
||
jest.setTimeout(90_000); | ||
|
||
describe('Fraggle Rock API', () => { | ||
/** @type {InstanceType<StaticServer>} */ | ||
let server; | ||
/** @type {import('puppeteer').Browser} */ | ||
let browser; | ||
/** @type {import('puppeteer').Page} */ | ||
let page; | ||
/** @type {string} */ | ||
let serverBaseUrl; | ||
|
||
beforeAll(async () => { | ||
server = new StaticServer(); | ||
await server.listen(0, '127.0.0.1'); | ||
serverBaseUrl = `http://localhost:${server.getPort()}`; | ||
browser = await puppeteer.launch({ | ||
headless: true, | ||
}); | ||
}); | ||
|
||
beforeEach(async () => { | ||
page = await browser.newPage(); | ||
}); | ||
|
||
afterEach(async () => { | ||
await page.close(); | ||
}); | ||
|
||
afterAll(async () => { | ||
await browser.close(); | ||
await server.close(); | ||
}); | ||
|
||
describe('snapshot', () => { | ||
beforeEach(() => { | ||
server.baseDir = path.join(__dirname, '../fixtures/fraggle-rock/snapshot-basic'); | ||
}); | ||
|
||
it('should compute accessibility results on the page as-is', async () => { | ||
await page.goto(`${serverBaseUrl}/onclick.html`); | ||
// Wait for the javascript to run | ||
await page.waitForSelector('button'); | ||
await page.click('button'); | ||
// Wait for the violations to appear | ||
await page.waitForSelector('input'); | ||
|
||
const result = await lighthouse.snapshot({page}); | ||
if (!result) throw new Error('Lighthouse failed to produce a result'); | ||
|
||
const {lhr} = result; | ||
const accessibility = lhr.categories.accessibility; | ||
expect(accessibility.score).toBeLessThan(1); | ||
|
||
const auditResults = accessibility.auditRefs.map(ref => lhr.audits[ref.id]); | ||
const irrelevantDisplayModes = new Set(['notApplicable', 'manual']); | ||
const applicableAudits = auditResults | ||
.filter(audit => !irrelevantDisplayModes.has(audit.scoreDisplayMode)); | ||
|
||
const erroredAudits = applicableAudits | ||
.filter(audit => audit.score === null); | ||
expect(erroredAudits).toHaveLength(0); | ||
|
||
const failedAuditIds = applicableAudits | ||
.filter(audit => audit.score !== null && audit.score < 1) | ||
.map(audit => audit.id); | ||
expect(failedAuditIds).toContain('label'); | ||
}); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,9 +4,12 @@ | |
* Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License. | ||
*/ | ||
|
||
import Gatherer = require('../lighthouse-core/gather/gatherers/gatherer.js'); | ||
import Audit = require('../lighthouse-core/audits/audit.js'); | ||
|
||
interface ClassOf<T> { | ||
new (): T; | ||
} | ||
|
||
declare global { | ||
module LH { | ||
module Config { | ||
|
@@ -41,12 +44,13 @@ declare global { | |
path: string; | ||
options?: {}; | ||
} | { | ||
implementation: typeof Gatherer; | ||
implementation: ClassOf<Gatherer.GathererInstance>; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What does changing this do? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. see #11748 (comment) |
||
options?: {}; | ||
} | { | ||
instance: InstanceType<typeof Gatherer>; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is working around an old bug and should be able to just be |
||
instance: Gatherer.GathererInstance; | ||
options?: {}; | ||
} | Gatherer | typeof Gatherer | string; | ||
} | Gatherer.GathererInstance | ClassOf<Gatherer.GathererInstance> | string; | ||
|
||
|
||
export interface CategoryJson { | ||
title: string | IcuMessage; | ||
|
@@ -87,8 +91,8 @@ declare global { | |
} | ||
|
||
export interface GathererDefn { | ||
implementation?: typeof Gatherer; | ||
instance: InstanceType<typeof Gatherer>; | ||
implementation?: ClassOf<Gatherer.GathererInstance>; | ||
instance: Gatherer.GathererInstance; | ||
path?: string; | ||
} | ||
|
||
|
@@ -125,4 +129,4 @@ declare global { | |
} | ||
|
||
// empty export to keep file a module | ||
export {} | ||
export {}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -51,6 +51,21 @@ declare global { | |
trace?: Trace; | ||
} | ||
|
||
type PhaseResult_ = void|LH.GathererArtifacts[keyof LH.GathererArtifacts] | ||
export type PhaseResult = PhaseResult_ | Promise<PhaseResult_> | ||
|
||
export interface GathererInstance { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same here. The js class is the base class and the interface. Duplicating it here doesn't seem to do more with the type and adds to maintenance/distancing types from jsdocs, etc There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Decoupling the implementation and the interface is the goal here to distinguish the new from the old and differentiate which gatherers support which models of execution in a relatively trivial and lightweight way. Is there a counter proposal in there I'm missing for how incremental transition for gatherers will be supported? I'm happy to move the docs to the interface if that's what you're asking. |
||
name: keyof LH.GathererArtifacts; | ||
beforePass(context: LH.Gatherer.PassContext): PhaseResult; | ||
pass(context: LH.Gatherer.PassContext): PhaseResult; | ||
afterPass(context: LH.Gatherer.PassContext, loadData: LH.Gatherer.LoadData): PhaseResult; | ||
} | ||
|
||
export interface FRGathererInstance { | ||
name: keyof LH.GathererArtifacts; | ||
afterPass(context: FRTransitionalContext): PhaseResult; | ||
} | ||
|
||
namespace Simulation { | ||
export type GraphNode = import('../lighthouse-core/lib/dependency-graph/base-node').Node; | ||
export type GraphNetworkNode = _NetworkNode; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one day we should split this off into a
smoke-static-server-entry.js
or something ..