test(e2e): assert on rendered values, not bare numbers, in the floql tests - #63
Merged
Conversation
…tests
`ts floql` renders each point as `<epoch_ms>: <value>`, and these tests write
with wall-clock timestamps, so a bare-number substring assertion is matching
against a 13-digit number as well as the value.
try testing.expect(result.contains("10"));
try testing.expect(!result.contains("30"));
Both directions are unsound. Sampling epoch-ms across 48h: the timestamp
contains "30" about 4.2% of the time, which makes the negative assertion fail a
run that is behaving correctly; it contains "10" about 9.6% of the time, which
makes the positive assertion pass even when the value is wrong. The worst case
was `contains("2")` in the modulo test — a single digit appears in a 13-digit
timestamp roughly three quarters of the time, so that assertion was mostly
measuring nothing.
Values always render with four decimals and timestamps never contain a '.', so
matching the rendered form cannot collide. `floql glob tag filter =~` already
did this with "20.0000"; the rest now match.
Verified the new assertions actually bite: mutating an expected value makes the
test fail. The whole `e2e/ts` suite passes, as does test-unit.
Whether this is the cause of the intermittent failure in #61 is not proven —
16 local runs of the old assertion did not reproduce it, though consecutive
local runs share nearly all timestamp digits so that is weak evidence, and 4.2%
predicts roughly zero failures in 16. The assertion is unsound regardless.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses #61 — with an honest caveat about what is and isn't proven.
The defect
ts floqlrenders each point as<epoch_ms>: <value>:These tests write with wall-clock timestamps, so a bare-number substring assertion matches against a 13-digit number as well as the value:
Sampling epoch-ms across 48 hours:
"30""10"The worst was
contains("2")in the modulo test — one digit appears in a 13-digit timestamp roughly three quarters of the time, so that assertion was mostly measuring nothing.The fix
Values always render with four decimals and timestamps never contain a
., so the rendered form cannot collide. Nine tests updated.floql glob tag filter =~already did exactly this with"20.0000"— the idiom was already in the file, just not applied consistently.Verification
I checked the new assertions actually bite rather than assuming: mutating an expected value (
2.0000→9.0000) makes the test fail. Fulle2e/tssuite andtest-unitpass.What is not proven
I have not shown this caused the #61 flake. I ran the old assertion 16 times locally and it passed 16/16. That is weak evidence in both directions — consecutive local runs share nearly all timestamp digits, and 4.2% predicts roughly zero failures in 16 anyway — but I would rather say so than present a tidy story.
What is certain is that the assertions were unsound in both directions and are now sound. I'd suggest leaving #61 open until a few CI cycles pass without recurrence, rather than closing it on this PR.