Bug 1941337: Sign the Delta column and group thousands in metric values - #1071
Bug 1941337: Sign the Delta column and group thousands in metric values#1071sumairq wants to merge 9 commits into
Conversation
✅ Deploy Preview for mozilla-perfcompare ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
| > | ||
| {`${formatNumber(pct)} %`} | ||
| {`${signPrefix(pct)}${formatNumber(pct)} %`} | ||
| {normality === 'one' && ( |
There was a problem hiding this comment.
Why not do this instead?: {`${signPrefix(formatNumber(pct))} %`} We want to add the sign to the number we show to users; not the number we started with, right? I'd recommend updating the signPrefix helper in src/utils/format to return the sign and formatted number vs only the sign. What do you think?
There was a problem hiding this comment.
Also, could you please add test coverage for your signPrefix helper and consider more edge cases?
expect(signPrefix(formatNumber(0.0004))).toBe('0'); // not '+0'
expect(signPrefix(formatNumber(-0.0004))).toBe('0'); // not '-0'
expect(signPrefix(formatNumber(1.08))).toBe('+1.08');
There was a problem hiding this comment.
Great suggestions 👍 , agreed on both, and I've updated the code accordingly.
Signing the displayed number: reworked the helper to take the displayed value and return the full signed string, so the call sites are now withSign(formatNumber(pct)). I renamed it from signPrefix to withSign since it no longer returns just a prefix. It takes either a formatted string or a raw number and derives the sign from whichever it gets, so the sign always matches what's shown (no +0/-0 issue).
Test coverage: added the tests in a new src/__tests__/format.test.ts, format.ts had no test file yet, made a desicion to put them in a new file instead of an old one.
One note: reading the sign from the formatted string relies on the hardcoded en-US locale, consistent with the bug's decision, just flagging it as intentional.
8768dcf to
eac60e8
Compare
Summary
Improves numeric formatting in the comparison results, addressing the two asks in
Bug 1941337:
with a leading
+(e.g.+53.31 %instead of53.31 %), so direction isimmediately readable alongside the existing
-on negatives.grouping separator (e.g.
2,113.69), completing the work started in Format numbers in revision and subtests revision rows #828 whichcovered the Base/New columns.
Locale stays hardcoded to
en-US, per the decision recorded on the bug.Changes
Sign (
+) on directional percentages via a smallwithSignhelper that takesthe displayed value and prefixes a
+when it is positive:"Difference of means" / "Difference of medians" lines.
(−1…1), p-values, and confidence.
Thousands grouping on measurement values via
formatNumberFixedTwo(grouping.toFixed(2)appearance):RunValues: the overflow individual run values and the standard-deviation linenow group consistently with the first-100 values and Base/New columns from Format numbers in revision and subtests revision rows #828.
Notes / decisions
No misleading zero signs. Because
withSigndecides the sign from the exacttext shown, a tiny value that rounds to zero never displays
+0or-0, it justshows
0.Decimal places are preserved where they mattered. The metrics cells keep
exactly 2 decimals (mirroring
.toFixed(2));RunValuesreuses the existingformatNumber, so its values follow the same ≤3-decimal display as the rest ofthe column (real run values are ≤2 decimals, so this is not visible in practice;
full precision is still preserved by the "Copy results" button).
🤖 Generated with Claude Code