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

tests: check for console errors and warnings in pptr tests #15516

Merged
merged 4 commits into from
Jan 16, 2024

Conversation

connorjclark
Copy link
Collaborator

Better error checking in these tests would have prevented #15515. All the pptr tests the listen for pageerror now also fail on console error/warning. There was also an issue of timing, which I resolved in this PR too w/ usage of promises and a method to "claim" the errors.

@connorjclark connorjclark requested a review from a team as a code owner October 4, 2023 19:16
@connorjclark connorjclark requested review from brendankenny and removed request for a team October 4, 2023 19:16
@@ -46,6 +46,8 @@ export class ViewerUIFeatures extends ReportUIFeatures {
}

this._getI18nModule().then(async (i18nModule) => {
if (!report.i18n?.icuMessagePaths) return;

Copy link
Collaborator Author

@connorjclark connorjclark Oct 4, 2023

Choose a reason for hiding this comment

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

older LHRs were erroring, but in a non-fatal way since this is a loose promise.

I didn't prevent the loading of the i18n module because the test would need to exclude the warning Chrome emits on an unused preload directive, and this being a minor optimization for ancient LHRs I'd rather not further complicate the tests.

viewer/test/viewer-test-pptr.js Outdated Show resolved Hide resolved
pageErrors = [];
// Expected errors.
const errors = await claimErrors();
expect(errors).toHaveLength(3);
Copy link
Member

Choose a reason for hiding this comment

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

Are these new errors expected because of the new timeout above?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, they have always been printed to the console. Now the tests are checking console errors in addition to pageerror.

@connorjclark connorjclark merged commit 047974f into main Jan 16, 2024
29 of 30 checks passed
@connorjclark connorjclark deleted the ppt-errors branch January 16, 2024 23:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants