Conversation
The index page recomputes the Total row in JavaScript with toFixed(), which has neither of the guarantees display_covered() provides, so the footer could disagree with the heading right above it -- including showing 100% when coverage was not complete, and 0% when some code did run. A synthetic input event is dispatched on load, so this happens on every page view, not just filtered ones. Port display_covered() to JavaScript and use it for the footer. Ties are rounded to even, as Python's round() does, which means working from the exact value of the double: scaling in floating point can nudge a value that is merely close to a tie onto one. Verified against Python over 166,604 (numer, denom, precision) combinations, all matching. Closes coveragepy#2256
The JS test suite was de-jQuery'd in coveragepy#2272, and index.html no longer loads jQuery. The display_covered cases added here still used $.each, so once this is merged onto main the whole suite would fail to load with "ReferenceError: $ is not defined". Use Array.prototype.forEach instead, matching the rest of the file.
|
Sorry about the noise here -- my tooling posted a filename instead of the file's contents. This was meant to be the note below; the PR description covers the same ground. The footer's Total is recomputed in the browser, so it has to agree with the percentage the server rendered in the heading directly above it.
A synthetic The fix ports One divergence from the issue is not addressed: with |
What was this meant to be? |
| // an exact tie rounds to the even neighbor, where Number.toFixed() always | ||
| // rounds away from zero. The double is split into an exact integer fraction | ||
| // first, because scaling in floating point can nudge a value that is merely | ||
| // close to a tie onto one, and so change the result. |
There was a problem hiding this comment.
I don't understand all this code, but it seems really complicated for something that is conceptually simple. Are you sure we need all this? I'd rather have a very occasional mismatch than have to maintain this code.
There was a problem hiding this comment.
Good question -- you were right to push back, and I've rewritten it. The same function is now:
function round_half_even(value, places) {
let mantissa = Math.abs(value);
let exponent = 0;
while (mantissa !== Math.floor(mantissa)) {
mantissa *= 2;
exponent += 1;
}
const numerator = BigInt(mantissa) * (10n ** BigInt(places));
const denominator = 1n << BigInt(exponent);
let rounded = numerator / denominator;
const twice_remainder = 2n * (numerator % denominator);
if (twice_remainder > denominator || (twice_remainder === denominator && (rounded % 2n) === 1n)) {
rounded += 1n;
}
return rounded;
}No DataView and no bit masks: doubling a double is exact, so the loop recovers the value as m/2**e and the rounding is done on that fraction.
On whether any of it is needed: the clamp half certainly is, and that part is short. For the rounding half, here is what each step buys, measured over 51,540 (numerator, denominator, precision) combinations against Python's display_covered():
toFixed()plus the clamps: 215 disagreements. These are ordinary numbers -- 1/8 is 12.5%, whichtoFixed()shows as 13 and Python shows as 12. Exact ties are not exotic.- Half-even, but with the scaling done in floating point first: 13 disagreements. That shortcut looks right and isn't:
0.15 * 10is exactly 1.5 even though the double nearest 0.15 is a shade below it, so scaling invents ties that Python never sees. Scaling the integers instead collapses the same last bits and lands on the same 13. - Rounding the exact value of the double, as above: 0.
If you would still rather not carry it, there is a smaller version: pass the numerator and denominator instead of a percentage, scale with a single integer division, and round with Math.floor plus a remainder === 0.5 test. About ten lines, no BigInt -- and it lands in that middle bucket. The 13 are all cases like 45/10000, which is exactly 0.45%: Python renders it as 0.5 and that version renders it as 0.4. Say the word and I'll switch to it.
round_half_even() used to decode the IEEE-754 bits with a DataView to get the double's exact value as a fraction before scaling it. Doubling a double is exact, so the same m/2**e pair can be recovered with a loop and no bit twiddling, which is a lot easier to read. Verified against Python's coverage.results.display_covered() over 51,540 (numer, denom, precision) combinations: all match, as before.
main gained a new Unreleased entry (issue 1563) in the same place, so CHANGES.rst conflicted. Both entries are kept, the upstream one first. No code changes were needed: coverage/htmlfiles/coverage_html.js and tests/js/tests.js merged cleanly, and the diff against main is still the same three files.
|
Branch synced with The |
|
I only found #2257 today (siaklin99-gif, opened 2026-08-11) — I opened this PR without having seen it, which is on me. Since you asked for a comparison on #2259, here are the measured numbers so you can pick one and I will get out of the way. All three PRs (#2257, #2259, this one) fix the reported symptom in #2256 and all three are green. They differ on one design decision:
The residual gap in the two "clamp" implementations is tie rounding.
Two examples: For #2257 all 345 can only surface while a filter is hiding rows, since that is the only path where it recomputes at all. #2259 recomputes unconditionally, so there they can also appear on a plain page view. Given your "I'd rather have a very occasional mismatch than have to maintain this code", #2257 looks like the better trade to me and I would suggest merging that one. I am happy to close this PR, or — if you would rather have the filtered view exact too — to rebase just the tie-rounding piece onto #2257 as a follow-up once it lands. Say which and I will do it. |
This branch has not been deployed
2 participants
Closes #2256.
The index page recomputes the Total row of the footer in JavaScript, using
toFixed(). That has neither of the two guaranteesdisplay_covered()provides, so the footer could disagree with the heading rendered by the server directly above it:toFixed()has no 0/100 clamp, so 249/250 displayed as 100% even though a statement never ran, and 4/1000 displayed as 0% even though four statements did.toFixed()rounds ties away from zero, where Python''sround()rounds them to even, so 173/200 was 87% in the footer and 86% in the heading.A synthetic
inputevent is dispatched on load, so this is not limited to filtered views: the server-rendered footer is overwritten on every page view.This ports
display_covered()to JavaScript (coverage.display_covered) and uses it for the footer, so the two can never disagree. The zero-denominator case still reports 100%, matchingNumbers._percent().On the rounding. Matching Python means matching round-half-to-even, which has to work from the exact value of the double. The obvious implementation scales by 10**places and compares the fraction to 0.5, but that is wrong:
0.15 * 10rounds to exactly1.5in floating point even though the stored value is just below 0.15, inventing a tie that Python does not see. That variant disagreed with Python on 401 of 166,604 cases.round_half_even()here decomposes the double into an exact integer fraction and rounds with BigInt instead, which agrees on all 166,604.Verification.
coverage.display_coveredwas compared against Python''sdisplay_coveredover every (numer, denom, precision) combination for denom up to 150 plus 14 larger values, at precisions 0-3: all 166,604 matched, including the zero-denominator case. QUnit tests were added totests/js/tests.jscovering the cases from the issue plus the same four casestest_results.pyuses fordisplay_covered, and were run headlessly.Note the tests in
tests/js/are browser-only and are not run in CI, so they were executed with a small local harness rather than by the test suite.One divergence reported in the issue is not addressed here: with
--skip-covered, fully covered files add to the heading total but produce no row, so the JS sum of surviving rows is lower than the heading. That is a separate accounting problem in how the totals are accumulated, not a rounding error, and it seemed better kept out of this fix. Happy to open a follow-up if you want it.