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

core(runner): independent gather and audit functions #13569

Merged
merged 22 commits into from
Feb 3, 2022
Merged

core(runner): independent gather and audit functions #13569

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
627356d
core(runner): independent gather and audit functions
adamraine Jan 14, 2022
cd390a4
+ runner state
adamraine Jan 14, 2022
90e31dc
Revert "+ runner state"
adamraine Jan 18, 2022
f7f206d
rn
adamraine Jan 18, 2022
57935d6
shared opts
adamraine Jan 18, 2022
34c1a02
log msg
adamraine Jan 18, 2022
e631034
fix tests
adamraine Jan 18, 2022
2e01dfa
runner
adamraine Jan 18, 2022
b9fffaa
lr test
adamraine Jan 18, 2022
1f22fa6
sample
adamraine Jan 18, 2022
e868c3e
Merge branch 'master' into fr-defer-audit-runner
adamraine Jan 19, 2022
e54eabb
comments
adamraine Jan 20, 2022
e4a2ca2
Merge branch 'fr-defer-audit-runner' of github.com:GoogleChrome/light…
adamraine Jan 20, 2022
291ae9a
sync
adamraine Jan 20, 2022
69d5f98
new func
adamraine Jan 20, 2022
1757426
timings
adamraine Jan 20, 2022
ec46509
Merge branch 'master' into fr-defer-audit-runner
adamraine Jan 25, 2022
900f948
core(runner): store gather timing on artifacts (#13587)
adamraine Feb 1, 2022
7c0d9e2
Merge branch 'master' into fr-defer-audit-runner
adamraine Feb 1, 2022
1b05363
Merge branch 'master' into fr-defer-audit-runner
adamraine Feb 1, 2022
7eec630
tests: timing smoke test (#13614)
adamraine Feb 2, 2022
6c961ee
Merge branch 'master' into fr-defer-audit-runner
adamraine Feb 3, 2022
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
Prev Previous commit
Next Next commit
fix tests
  • Loading branch information
@adamraine
adamraine committed Jan 18, 2022
commit e6310346f95ede2019b31ea5b5c752a661b869c1
11 changes: 7 additions & 4 deletions lighthouse-core/runner.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ class Runner {

return {lhr, artifacts, report};
} catch (err) {
throw Runner.createRunnerError(err, settings);
throw await Runner.createRunnerError(err, settings);
}
}

Expand Down Expand Up @@ -184,7 +184,7 @@ class Runner {

return artifacts;
} catch (err) {
throw Runner.createRunnerError(err, settings);
throw await Runner.createRunnerError(err, settings);
}
}

Expand Down Expand Up @@ -229,8 +229,11 @@ class Runner {
entryType: entry.entryType,
};
}).sort((a, b) => a.startTime - b.startTime);
const runnerEntry = timingEntries.find(e => e.name === 'lh:runner:run');
return {entries: timingEntries, total: runnerEntry?.duration || 0};
const gatherEntry = timingEntries.find(e => e.name === 'lh:runner:gather');
const auditEntry = timingEntries.find(e => e.name === 'lh:runner:audit');
const gatherTiming = gatherEntry?.duration || 0;
const auditTiming = auditEntry?.duration || 0;
return {entries: timingEntries, total: gatherTiming + auditTiming};
Comment on lines +239 to +243
Copy link
Member Author

@adamraine adamraine Jan 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This might encounter issues in a flows as we move forward with #13364. I don't know if a flow step is guaranteed to query the correct timing entry from the logger.

@brendankenny this might be a reason to use a Runner instance instead. We could store timing information on that instance instead of querying the logger.

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 not causing any issues right now because log.takeTimingEntries clears everything between each flow step. I'll investigate this in the next part. There's probably a way to remember timings for each step without adding state to Runner.

Copy link
Member

Choose a reason for hiding this comment

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

FWIW it's not clear what total should mean going forward. Right now it's the total of the LH run that just completed, so if run with -A, the -G timings are in the entries array, but not included in the total. That would be changed here, and I kind of prefer the old way, but I'm not sure if the total timing of -A runs is important enough to care (if e.g. you're trying to optimizing something, you're likely looking at some other timing or tool).

In a user flow, is the amount of time to gather and audit a particular step the most meaningful interpretation of "total"? Seems ok.

Should add some total tests if we're making a decision.

Also it's worth pointing out that total was a placeholder while we figured out entries and only kept it around because it was kind of useful after entries landed, so the only bar to clear here is "kind of useful" :)

}

/**
Expand Down
8 changes: 4 additions & 4 deletions lighthouse-core/test/fraggle-rock/gather/mock-driver.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,17 +146,17 @@ function createMockDriver() {
function mockRunnerModule() {
const runnerModule = {
getGathererList: jest.fn().mockReturnValue([]),
gatherAndManageArtifacts: jest.fn(),
run: jest.fn(),
audit: jest.fn(),
gather: jest.fn(),
reset,
};

jest.mock('../../../runner.js', () => runnerModule);

function reset() {
runnerModule.getGathererList.mockReturnValue([]);
runnerModule.gatherAndManageArtifacts.mockReset();
runnerModule.run.mockReset();
runnerModule.audit.mockReset();
runnerModule.gather.mockReset();
}

return runnerModule;
Expand Down
6 changes: 3 additions & 3 deletions lighthouse-core/test/fraggle-rock/gather/navigation-runner-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -483,8 +483,8 @@ describe('NavigationRunner', () => {
describe('navigation', () => {
it('should throw on invalid URL', async () => {
const runnerActual = jest.requireActual('../../../runner.js');
mockRunner.run.mockImplementation(runnerActual.run);
mockRunner.gatherAndManageArtifacts.mockImplementation(runnerActual.gatherAndManageArtifacts);
mockRunner.gather.mockImplementation(runnerActual.gather);
mockRunner.audit.mockImplementation(runnerActual.audit);

const navigatePromise = runner.navigation({
url: '',
Expand All @@ -508,7 +508,7 @@ describe('NavigationRunner', () => {
configContext,
});

expect(mockRunner.run.mock.calls[0][1]).toMatchObject({
expect(mockRunner.gather.mock.calls[0][1]).toMatchObject({
config: {
settings: settingsOverrides,
},
Expand Down
15 changes: 8 additions & 7 deletions lighthouse-core/test/fraggle-rock/gather/snapshot-runner-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,14 +66,15 @@ describe('Snapshot Runner', () => {
it('should connect to the page and run', async () => {
await snapshot({page, config});
expect(mockDriver.connect).toHaveBeenCalled();
expect(mockRunner.run).toHaveBeenCalled();
expect(mockRunner.gather).toHaveBeenCalled();
expect(mockRunner.audit).toHaveBeenCalled();
});

it('should collect base artifacts', async () => {
mockPage.url.mockResolvedValue('https://lighthouse.example.com/');

await snapshot({page, config});
const artifacts = await mockRunner.run.mock.calls[0][0]();
const artifacts = await mockRunner.gather.mock.calls[0][0]();
expect(artifacts).toMatchObject({
fetchTime: expect.any(String),
URL: {finalUrl: 'https://lighthouse.example.com/'},
Expand All @@ -82,7 +83,7 @@ describe('Snapshot Runner', () => {

it('should collect snapshot artifacts', async () => {
await snapshot({page, config});
const artifacts = await mockRunner.run.mock.calls[0][0]();
const artifacts = await mockRunner.gather.mock.calls[0][0]();
expect(artifacts).toMatchObject({A: 'Artifact A', B: 'Artifact B'});
expect(gathererA.getArtifact).toHaveBeenCalled();
expect(gathererB.getArtifact).toHaveBeenCalled();
Expand All @@ -99,7 +100,7 @@ describe('Snapshot Runner', () => {
const configContext = {settingsOverrides};
await snapshot({page, config, configContext});

expect(mockRunner.run.mock.calls[0][1]).toMatchObject({
expect(mockRunner.gather.mock.calls[0][1]).toMatchObject({
config: {
settings: settingsOverrides,
},
Expand All @@ -108,7 +109,7 @@ describe('Snapshot Runner', () => {

it('should not invoke instrumentation methods', async () => {
await snapshot({page, config});
await mockRunner.run.mock.calls[0][0]();
await mockRunner.gather.mock.calls[0][0]();
expect(gathererA.startInstrumentation).not.toHaveBeenCalled();
expect(gathererA.startSensitiveInstrumentation).not.toHaveBeenCalled();
expect(gathererA.stopSensitiveInstrumentation).not.toHaveBeenCalled();
Expand All @@ -119,7 +120,7 @@ describe('Snapshot Runner', () => {
gathererB.meta.supportedModes = ['timespan'];

await snapshot({page, config});
const artifacts = await mockRunner.run.mock.calls[0][0]();
const artifacts = await mockRunner.gather.mock.calls[0][0]();
expect(artifacts).toMatchObject({A: 'Artifact A'});
expect(artifacts).not.toHaveProperty('B');
expect(gathererB.getArtifact).not.toHaveBeenCalled();
Expand All @@ -132,7 +133,7 @@ describe('Snapshot Runner', () => {
gathererB.meta.dependencies = {ImageElements: dependencySymbol};

await snapshot({page, config});
const artifacts = await mockRunner.run.mock.calls[0][0]();
const artifacts = await mockRunner.gather.mock.calls[0][0]();
expect(artifacts).toMatchObject({A: 'Artifact A', B: 'Artifact B'});
expect(gathererB.getArtifact.mock.calls[0][0]).toMatchObject({
dependencies: {
Expand Down
17 changes: 9 additions & 8 deletions lighthouse-core/test/fraggle-rock/gather/timespan-runner-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,8 @@ describe('Timespan Runner', () => {
const timespan = await startTimespan({page, config});
await timespan.endTimespan();
expect(mockDriver.connect).toHaveBeenCalled();
expect(mockRunner.run).toHaveBeenCalled();
expect(mockRunner.gather).toHaveBeenCalled();
expect(mockRunner.audit).toHaveBeenCalled();
});

it('should prepare the target', async () => {
Expand All @@ -96,7 +97,7 @@ describe('Timespan Runner', () => {
mockPage.url.mockResolvedValue('https://end.example.com/');

await timespan.endTimespan();
const artifacts = await mockRunner.run.mock.calls[0][0]();
const artifacts = await mockRunner.gather.mock.calls[0][0]();
expect(artifacts).toMatchObject({
fetchTime: expect.any(String),
URL: {
Expand All @@ -117,7 +118,7 @@ describe('Timespan Runner', () => {
const timespan = await startTimespan({page, config, configContext});
await timespan.endTimespan();

expect(mockRunner.run.mock.calls[0][1]).toMatchObject({
expect(mockRunner.gather.mock.calls[0][1]).toMatchObject({
config: {
settings: settingsOverrides,
},
Expand All @@ -127,7 +128,7 @@ describe('Timespan Runner', () => {
it('should invoke stop instrumentation', async () => {
const timespan = await startTimespan({page, config});
await timespan.endTimespan();
await mockRunner.run.mock.calls[0][0]();
await mockRunner.gather.mock.calls[0][0]();
expect(gathererA.stopSensitiveInstrumentation).toHaveBeenCalled();
expect(gathererB.stopSensitiveInstrumentation).toHaveBeenCalled();
expect(gathererA.stopInstrumentation).toHaveBeenCalled();
Expand All @@ -137,7 +138,7 @@ describe('Timespan Runner', () => {
it('should collect timespan artifacts', async () => {
const timespan = await startTimespan({page, config});
await timespan.endTimespan();
const artifacts = await mockRunner.run.mock.calls[0][0]();
const artifacts = await mockRunner.gather.mock.calls[0][0]();
expect(artifacts).toMatchObject({A: 'Artifact A', B: 'Artifact B'});
});

Expand All @@ -147,7 +148,7 @@ describe('Timespan Runner', () => {

const timespan = await startTimespan({page, config});
await timespan.endTimespan();
const artifacts = await mockRunner.run.mock.calls[0][0]();
const artifacts = await mockRunner.gather.mock.calls[0][0]();
expect(artifacts).toMatchObject({A: artifactError, B: 'Artifact B'});
expect(gathererA.stopInstrumentation).not.toHaveBeenCalled();
expect(gathererB.stopInstrumentation).toHaveBeenCalled();
Expand All @@ -158,7 +159,7 @@ describe('Timespan Runner', () => {

const timespan = await startTimespan({page, config});
await timespan.endTimespan();
const artifacts = await mockRunner.run.mock.calls[0][0]();
const artifacts = await mockRunner.gather.mock.calls[0][0]();
expect(artifacts).toMatchObject({A: 'Artifact A'});
expect(artifacts).not.toHaveProperty('B');
expect(gathererB.startInstrumentation).not.toHaveBeenCalled();
Expand All @@ -173,7 +174,7 @@ describe('Timespan Runner', () => {

const timespan = await startTimespan({page, config});
await timespan.endTimespan();
const artifacts = await mockRunner.run.mock.calls[0][0]();
const artifacts = await mockRunner.gather.mock.calls[0][0]();
expect(artifacts).toMatchObject({A: 'Artifact A', B: 'Artifact B'});
expect(gathererB.getArtifact.mock.calls[0][0]).toMatchObject({
dependencies: {
Expand Down