Fix exhaustive test check #51
Conversation
a64f006 to
97ef6fd
Compare
The old test was not doing anything; it would exit the loop instantly at i==0 rather than at the end of the range.
CFLAGS is unused, so the tests have likely been running without these flags set.
97ef6fd to
294b6c5
Compare
|
@gmaxwell @naumenkogs Care to review? |
| CHECK(serialized == serialized_rebuild); | ||
| // Count it | ||
| if (elements_0.size() <= capacity) ++counts[elements_0.size()]; | ||
| if (impl == 0 && elements_0.size() <= capacity) ++counts[elements_0.size()]; |
There was a problem hiding this comment.
So this line changes nothing, it just saves us from rewriting something again with the same value?
There was a problem hiding this comment.
Without it, counts[...] will be tripled if there are 3 implementations.
| // Verify that the number of decodable sketches with given elements is expected. | ||
| for (uint64_t i = 0; i <= capacity && i >> bits; ++i) { | ||
| CHECK(counts[i] == Combination((uint64_t{1} << bits) - 1, i)); | ||
| uint64_t mask = bits == 64 ? UINT64_MAX : (uint64_t{1} << bits) - 1; |
There was a problem hiding this comment.
And this is the real change: replacing i >> bits with (uint64_t{1} << bits) - 1?
There was a problem hiding this comment.
There were 3 issues:
- The comparison between
countsandCombination()wasn't running at all ((i >> bits) is always 0 for i==0, so the loop would instantly exit. - The
countsvalues were wrong (because it was counting a sum across all implementation), so if they would be compared to something, the test would incorrectly fail. (i >> bits)is undefined when bits==64.
The first would be fixed by changing i >> bits to (i >> bits) == 0, but that leaves the >> 64 issue, so I introduce this mask and use a different way of writing the same, that keeps working for bits==64.
The second is fixed by only incrementing counts[...] for impl==0.
There was a problem hiding this comment.
Updated the PR description with this explanation.
|
ACK 294b6c5 I wish we had a way to test that tests were run. |
|
There is a way: introduce intentional bugs, and see if the tests catch it. |
There were 3 issues:
counts[...]andCombination()wasn't running at all ((i >> bits)is always 0 for i==0, so the loop would instantly exit).countsvalues were wrong (because it was counting a sum across all implementation), so if they would be compared to something, the test would incorrectly fail.(i >> bits)is undefined when bits==64.(1) would be fixed by changing
i >> bitsto(i >> bits) == 0, but that leaves issue (3), so introduce amaskand use a different way of writing the same, that keeps working for bits==64. (2) is fixed by only incrementing counts[...] for impl==0.This was discovered through #50.