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(smoke): print multiple differences #14141

Merged
merged 7 commits into from
Jun 22, 2022

Conversation

connorjclark
Copy link
Collaborator

@connorjclark connorjclark commented Jun 17, 2022

Fixes #14128

Brendan mentioned here that presenting all the differences in a smoke test may be noisy. This PR doesn't take any of that into account (I did first try to limit the amount of differences as determined here, but that seems to reduce to the same behavior we have today), first I'd like to see exactly what the smoke results might look like when too noisy. @brendankenny can you think of a good way to test this?

@connorjclark connorjclark requested a review from a team as a code owner June 17, 2022 22:05
@connorjclark connorjclark requested review from adamraine and removed request for a team June 17, 2022 22:05
@connorjclark connorjclark changed the base branch from master to report-assert-tests June 17, 2022 22:06
found: ${JSON.stringify(diff.actual)}
found: ${JSON.stringify(diff.actual)}\n`;
localConsole.log(msg);
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

praying for String.dedent

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

first I'd like to see exactly what the smoke results might look like when too noisy

two examples I can think of:

both are quite noisy.

That said, I tried this PR out a bit and it seems like it will still be worth it. The fact that expectations are usually limited in size and they're what drive the comparisons means that often even excessive errors aren't that bad (LinkElements and ConsoleMessages also happen to both be arrays, rename LinkElements to URL (which is an object) instead and the diff list is considerably smaller).

lighthouse-cli/test/smokehouse/report-assert.js Outdated Show resolved Hide resolved
lighthouse-cli/test/smokehouse/report-assert.js Outdated Show resolved Hide resolved
@@ -427,19 +425,20 @@ function reportAssertion(localConsole, assertion) {
log.greenify(assertion.actual));
}
} else {
if (assertion.diff) {
const diff = assertion.diff;
const fullActual = String(JSON.stringify(assertion.actual, null, 2))
Copy link
Member

Choose a reason for hiding this comment

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

little did you know this was a load-bearing String() (try renaming LinkElements to LinkElementz or whatever :)

Though I certainly didn't remember why it was there, so we can change it to something more obvious that does the same thing

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Made it more explicit and added tests.

@connorjclark connorjclark changed the title tests(smoke): add tests for report-assert.js tests(smoke): print multiple differences Jun 21, 2022
Base automatically changed from report-assert-tests to master June 22, 2022 00:16
@connorjclark connorjclark merged commit eb313f6 into master Jun 22, 2022
@connorjclark connorjclark deleted the report-assert-tests-multiple branch June 22, 2022 17:49
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.

Smoke runner should display more than one failed assertion at a time
4 participants