A failed suite ran, so count it as one (#447) - #450
Conversation
The per-major tally added up only while everything passed. commandprompt#448 counts a suite as having run when it exits 0 and as skipped when it exits 2, and a failure lands in neither. The first five-major matrix after it merged reported PG18 suites that ran: 119 of 122 (skipped: 2) temporal=FAIL PG19 suites that ran: 121 of 122 (skipped: 0) temporal=FAIL 121 + 0 is not 122. The failing suite vanished from the count whose entire purpose is to say what ran, which is a milder version of the confusion commandprompt#447 was about. Four majors hid it. A tally only contradicts itself once something fails, and 15, 16 and 17 were green.
jdatcmd
left a comment
There was a problem hiding this comment.
Correct, and the bug is a good example of the thing it is fixing
PG19 suites that ran: 121 of 122 (skipped: 0)
121 + 0 is not 122, and the number that exists to say what ran was the one that could not
account for a failure. Four majors hid it, because a tally only disagrees with itself once
something fails. That is the same shape as everything else this week: the instrument was
fine right up to the moment it mattered.
Verified every path now increments exactly one counter, including the one nobody writes on
purpose: a missing .rc file gives an empty _rc, which falls to the else and is now
counted as ran. So ran + skipped == total holds for pass, skip, fail, and absent.
What I would add, and it is the whole lesson
Nothing asserts the invariant. The arithmetic is correct today because all four paths
happen to be covered. A fifth outcome, or a continue added above one of the increments,
puts it back to silently not adding up, and we would find out the same way: only when
something failed, on one major, months later.
One line, next to where the total is printed:
if [ $((suites_ran + suites_skipped)) -ne "${#SUITES[@]}" ]; then
echo " BUG: the tally does not add up: $suites_ran ran + $suites_skipped skipped != ${#SUITES[@]}"
overall=1
fiThat is the difference between a count that is right and a count that cannot be wrong
without saying so. Given this PR exists because a count was wrong without saying so, I
think it belongs here rather than in a follow-up.
Not blocking. The fix is right either way and I will merge it as it stands if you would
rather keep the change minimal.
One thing worth noting for the record
This is the second self-correction you have filed today, after #434. Both were on your own
work, both found by looking at output you could have accepted, and both produced a better
result than the original. That is the arrangement working, and it is worth saying so rather
than only reporting defects.
Follow-up to #448, which I got wrong in a small way that only shows up when something fails.
#448 counts a suite as having run when it exits 0 and as skipped when it exits 2. A
failure exits 1 and lands in neither, so the per-major tally silently drops it. The first
five-major matrix after it merged said:
121 + 0 is not 122. The failing suite disappeared from the count whose whole purpose is to say
what ran, which is a milder version of the confusion #447 was about.
Four majors hid it. PG15, 16 and 17 were green, and a tally only contradicts itself once
something actually fails. I did not see it in the arms I ran before opening #448, because none
of them failed, and neither did the review.
Validated by forcing a failure, both arms
btree_gisthidden on pg18a, which since #448 makestemporalfail as a missing dependency.Same major, same box, same forced failure. Only the patch differs.
suites that ran: 119 of 122 (skipped: 2)suites that ran: 120 of 122 (skipped: 2)Both arms report
FAIL temporal. The verdict never changed; only whether the count admittedthe suite existed.
A failed suite ran. It ran, and it failed. Those are the two facts the line should carry, and
now it does.
Note on how this was validated
The fix cannot be demonstrated by a green run, because a green run has nothing in the failure
branch. So the check forces one:
btree_gistis hidden on pg18a, which since #448 makestemporalfail as a missing dependency, and the same major runs both with and without thepatch. If both arms had printed the same tally the fix would be doing nothing.