-
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(dom-size): display metric values as integers #14479
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
This won't actually change the value because the numbers are already integers. The problem is that the Lighthouse report displays the integers with unnecessary granularity. To fix this, we need to change the details items from {
statistic: str_(UIStrings.statisticDOMElements),
value: stats.totalBodyElements,
} to {
statistic: str_(UIStrings.statisticDOMElements),
value: {
type: 'numeric',
granularity: 1,
value: stats.totalBodyElements,
},
} |
@adamraine |
Please confirm your changes locally via |
@connorjclark @adamraine In addition, I checked there is no |
Some of our tests are failing, to fix this you should
lighthouse/core/test/audits/dobetterweb/dom-size-test.js Lines 24 to 32 in 60c2fa2
lighthouse/cli/test/smokehouse/test-definitions/dobetterweb.js Lines 403 to 417 in 60c2fa2
|
@adamraine I made changes in the commits below. A test case of https://github.com/GoogleChrome/lighthouse/actions/runs/3371728139/jobs/5594423467 |
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.
A test case of difference at total-blocking-time audit.numericValue fails but it seems there is no relation between that failure and this PR.
You are correct, this is a common flaky failure #14271. This PR looks good, thanks for the contribution!
Thanks for your quick review! I hope to contribute more 👍 |
Summary
Fixes #14422
Related Issues/PRs
I think this issue was caused by the changes in this PR