-
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(gather-runner): don't save trace on pass with pageLoadError #9198
Changes from all commits
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 |
---|---|---|
|
@@ -776,6 +776,85 @@ describe('GatherRunner', function() { | |
}); | ||
}); | ||
|
||
it('saves trace and devtoolsLog with error prefix when there was a runtime error', async () => { | ||
const requestedUrl = 'https://example.com'; | ||
const driver = Object.assign({}, fakeDriver, { | ||
// resolved URL here does not match any request in the network records, causing a runtime error. | ||
gotoURL: async _ => requestedUrl, | ||
online: true, | ||
}); | ||
|
||
const config = new Config({ | ||
passes: [{ | ||
passName: 'firstPass', | ||
recordTrace: true, | ||
gatherers: [{instance: new TestGatherer()}], | ||
}], | ||
}); | ||
const options = {driver, requestedUrl, settings: config.settings}; | ||
const artifacts = await GatherRunner.run(config.passes, options); | ||
|
||
expect(artifacts.TestGatherer.code).toEqual('NO_DOCUMENT_REQUEST'); | ||
|
||
// The only loadData available should be prefixed with `pageLoadError-`. | ||
expect(Object.keys(artifacts.traces)).toEqual(['pageLoadError-firstPass']); | ||
expect(Object.keys(artifacts.devtoolsLogs)).toEqual(['pageLoadError-firstPass']); | ||
}); | ||
|
||
it('does not run additional passes after a runtime error', async () => { | ||
const t1 = new (class Test1 extends TestGatherer {})(); | ||
const t2 = new (class Test2 extends TestGatherer {})(); | ||
const t3 = new (class Test3 extends TestGatherer {})(); | ||
const config = new Config({ | ||
passes: [{ | ||
passName: 'firstPass', | ||
recordTrace: true, | ||
gatherers: [{instance: t1}], | ||
}, { | ||
passName: 'secondPass', | ||
recordTrace: true, | ||
gatherers: [{instance: t2}], | ||
}, { | ||
passName: 'thirdPass', | ||
recordTrace: true, | ||
gatherers: [{instance: t3}], | ||
}], | ||
}); | ||
|
||
const requestedUrl = 'https://www.reddit.com/r/nba'; | ||
let firstLoad = true; | ||
const driver = Object.assign({}, fakeDriver, { | ||
// Loads the page successfully in the first pass, fails with NO_FCP in the second. | ||
async gotoURL(url) { | ||
if (url.includes('blank')) return null; | ||
if (firstLoad) { | ||
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. how do you feel about IMO, it would be a bit easier to read our mock setups if we constructed them consistently with the creators. Right now we kinda have to figure out each situation on a case-by-case basis jest.fn()
.mockResolvedValueOnce(requestedUrl)
.mockRejectedValueOnce(new LHError(LHError.errors.NO_FCP)); could go whole hog if we wanted const gotoBlankFn = jest.fn().mockResolvedValue(null);
const gotoPageURLFn = jest.fn()
.mockResolvedValueOnce(requestedUrl)
.mockRejectedValueOnce(new LHError(LHError.errors.NO_FCP));
const gotoURL = url => url.includes('blank') ? gotoBlankFn() : gotoPageUrlFn();
const driver = Object.assign({}, fakeDriver, {gotoURL}); 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 like it sometimes and have no problem with it, but in this case no matter what I try the readability comes out slightly worse (the |
||
firstLoad = false; | ||
return requestedUrl; | ||
} else { | ||
throw new LHError(LHError.errors.NO_FCP); | ||
} | ||
}, | ||
online: true, | ||
}); | ||
const options = {driver, requestedUrl, settings: config.settings}; | ||
const artifacts = await GatherRunner.run(config.passes, options); | ||
|
||
// t1.pass() and t2.pass() called; t3.pass(), after the error, was not. | ||
expect(t1.called).toBe(true); | ||
expect(t2.called).toBe(true); | ||
expect(t3.called).toBe(false); | ||
|
||
// But only t1 has a valid artifact, t2 has an error artifact, and t3 never ran. | ||
expect(artifacts.Test1).toBe('MyArtifact'); | ||
expect(artifacts.Test2).toBeInstanceOf(LHError); | ||
expect(artifacts.Test2.code).toEqual('NO_FCP'); | ||
expect(artifacts.Test3).toBeUndefined(); | ||
|
||
// firstPass has a saved trace and devtoolsLog, secondPass has an error trace and log. | ||
expect(Object.keys(artifacts.traces)).toEqual(['firstPass', 'pageLoadError-secondPass']); | ||
expect(Object.keys(artifacts.devtoolsLogs)).toEqual(['firstPass', 'pageLoadError-secondPass']); | ||
}); | ||
|
||
describe('#getNetworkError', () => { | ||
it('passes when the page is loaded', () => { | ||
const url = 'http://the-page.com'; | ||
|
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.
seems like this audit is useless at this point now, candidate for deletion?
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.
I'd say yes (are there cases when non 200 status codes shouldn't be a runtimeError?)