Add tests for the ordinary index #362
Conversation
jeltsch
requested review from
dcoutts,
jorisdral,
mheinzel and
recursion-ninja
as code owners
jeltsch
force-pushed
the
jeltsch/ordinary-index/tests
branch
from
9f6cee1
to
42a575a
Compare
| fromIndexOrdinary :: IndexOrdinary -> Vector SerialisedKey | ||
| fromIndexOrdinary (IndexOrdinary lastKeys) = lastKeys |
There was a problem hiding this comment.
Since the module is supposed to be imported qualified, mentioning the name again is redundant (IndexOrdinary.fromIndexOrdinary). Maybe focussing on the result, e.g. toLastKeys or lastKeysPerPage?
|
|
||
| {-# OPTIONS_GHC -Wno-orphans #-} | ||
|
|
||
| {- HLINT ignore "Avoid restricted alias" -} |
There was a problem hiding this comment.
I think these hints are all custom ones we specifically added to our hlint config, and I think it makes sense to use qualified imports consistently. I see that we already have a few similar HLINT ignore exceptions in src, but I don't think it's a good idea. If we're not consistent about it, we might as well get rid of the hlint rules, but I'd rather follow them.
| key < selectionHead .&&. | ||
| all (< key) prefix .&&. | ||
| all (== selectionHead) (tail selection) .&&. | ||
| all (> selectionHead) suffix |
There was a problem hiding this comment.
If this test fails, it's hard to find out why. QC.=== is nicer, but unfortunately there's nothing similar for < etc. (Might be worth adding locally?). Then some QC.conjoin to combine these properties and maybe a QC.counterexample to make it clear which part failed (selectionHead/prefix/selection/suffix).
We can defer adding these niceties until someone has to debug a failure, but it's nice to do it right away. At least a counterexample (show (prefix, selection, suffix)) around everything, so we see what search returned.
| = not (null lesserLastKeys) ==> all (< selectionHead) prefix .&&. | ||
| all (== selectionHead) (tail selection) .&&. | ||
| all (> selectionHead) suffix |
There was a problem hiding this comment.
Ah, at first I didn't see how that ensure that the key is unmentioned and beyond range, but we construct new index to have that. Makes sense.
But why do we still check the same properties? Couldn't we just say that prefix === lesserLastKeys and the rest is empty?
| input :: ShortByteString | ||
| input = potentialSerialisedIndex | ||
| testedTypeAndVersionBlock | ||
| (lastKeysBlocks lastKeys ++ [statedSizeBlock, partialKeyBlock]) |
There was a problem hiding this comment.
Yes, nice. Maybe one more test with a completely random key block would make sense, where we don't care about Left/Right, just that the result is QC.total. But this here should already cover the case of a key length going out of range. 👍
| instance Arbitrary NumEntries where | ||
|
|
||
| arbitrary = NumEntries <$> (getNonNegative <$> arbitrary) | ||
| `suchThat` | ||
| (toInteger >>> (< 2 ^ (64 :: Int))) | ||
|
|
||
| -- This shrinker relies on 'Int' values not becoming greater by shrinking. | ||
| shrink = shrinkMap (NumEntries . getNonNegative) | ||
| (\ (NumEntries entryCount) -> NonNegative entryCount) | ||
|
|
||
| instance Arbitrary SerialisedKey where | ||
|
|
||
| arbitrary = SerialisedKey' <$> | ||
| arbitrary `suchThat` (Primitive.length >>> (< 0x10000)) | ||
|
|
||
| -- This shrinker relies on lists not becoming longer by shrinking. | ||
| shrink = shrinkMap SerialisedKey' (\ (SerialisedKey' bytes) -> bytes) |
There was a problem hiding this comment.
We already have these instances in src-extras/../Generators.hs. The distribution for the second one might be a bit different, but I don't think it's a problem.
| SearchableIndex (indexFromLastKeys lastKeys') | | ||
| lastKeys' <- shrink (toList lastKeysVec), | ||
| not (null lastKeys'), | ||
| and (zipWith (<=) lastKeys' (List.tail lastKeys')) |
| instance Arbitrary Append where | ||
|
|
||
| arbitrary = oneof $ | ||
| [ | ||
| AppendSinglePage <$> arbitrary <*> arbitrary, | ||
| AppendMultiPage <$> arbitrary <*> (getSmall <$> arbitrary) | ||
| ] | ||
|
|
||
| shrink (AppendSinglePage firstKey lastKey) | ||
| = [AppendSinglePage firstKey' lastKey | firstKey' <- shrink firstKey] ++ | ||
| [AppendSinglePage firstKey lastKey' | lastKey' <- shrink lastKey] | ||
| shrink (AppendMultiPage key overflowPageCount) | ||
| = [AppendSinglePage key key] | ||
| ++ | ||
| [ | ||
| AppendMultiPage key' overflowPageCount | | ||
| key' <- shrink key | ||
| ] | ||
| ++ | ||
| [ | ||
| AppendMultiPage key overflowPageCount' | | ||
| overflowPageCount' <- shrinkMap getSmall Small $ | ||
| overflowPageCount | ||
| ] |
There was a problem hiding this comment.
Another way of doing this (which also the compact index tests use) would be to use the existing generators for LogicalPageSummaries, which has a function toAppends :: _ -> [Append].
|
Also, would be nice to see a rebase making the tests pass in CI. They pass for me locally when I rebase, but would be nice to see it here as well. |
The new implementation doesn’t lead to QuickCheck discarding too many test cases. The downside of it is that the indexes this property uses for search are only half the size on average.
jeltsch
force-pushed
the
jeltsch/ordinary-index/tests
branch
from
42a575a
to
af6c8f4
Compare
Sure thing, and, since #363 is finally merged, I performed the rebasing. Let’s see if the builds succeed now. |
They do. 🙂 |
This adds tests for the ordinary index.
Currently, the property
prop_searchForUnmentionedKeyBeyondRangeWorkstends to discard too many test cases. I will address this and then drop the draft status of this pull request.