Fix 'fromIndex' internal helper #23
Conversation
src/Control/Monad/Gen.purs
Outdated
| where | ||
| go j (Cons a _) | j <= 0 = a | ||
| go j (Cons _ as) = go (j - 1) as | ||
| go _ Nil = un Last (foldMap1 Last xs) |
There was a problem hiding this comment.
If you added a case for Cons a Nil you could avoid another traversal, and instead this would just serve as proof of non-emptyness, but never actually be invoked.
|
Finally added that case 🤦 |
|
Perhaps this should be merged before #26 because its |
| @@ -31,6 +31,13 @@ main = do | |||
| log "`genNonEmpty` should not reduce the remainder size below zero" | |||
| one :: NonEmpty Array Int ← Gen.resize (const 0) $ GenC.genNonEmpty (Gen.sized pure) | |||
| liftEffect $ assertEqual { actual: one, expected: 0 :| [] } | |||
|
|
|||
| log "Ensure that `elements` will produce all possible values (tests will hang if this fails)" | |||
| _ ← Gen.suchThat (Gen.elements ("A" :| ["B", "C", "D"])) (_ == "A") | |||
There was a problem hiding this comment.
Couldn't we do something that will fail (rather than hang CI for who knows how long) like this?
test = do
let testElems = "A" :| ["B", "C", "D"]
elems <- randomSample' 100 (Gen.elements testElems)
let generatedElems = foldl (\mymap e -> Map.insertWith (_ + 1) e 0 mymap) Map.empty elems
keys generatedElems `shouldEqual` (toArray testElems)
-- some other frequency test here, too.
There was a problem hiding this comment.
Only if we don't mind there occasionally being random failures. There's no limited number of samples you can generate that won't result in a false negative from time to time.
I suppose that's still technically true with the current test, just the number of things it will generate is bound by the CI timeout or node crashing rather than there being an explicit limit.
There was a problem hiding this comment.
Only if we don't mind there occasionally being random failures. There's no limited number of samples you can generate that won't result in a false negative from time to time.
True, but at least CI fails quicker. Also, if we increase the number of elements generated, wouldn't that decrease the possibility of a random failure occurring?
There was a problem hiding this comment.
Yeah, the larger the number the less likely it will fail.
There was a problem hiding this comment.
So.... 1,000, right? 😆
There was a problem hiding this comment.
I'll merge this PR and then submit a PR that uses the above approach.
Resolves #22
The test isn't great, since it'll just never complete if it fails, but that's better than nothing for now. Testing the distribution properly would be good at some point, seems like we did have more tests at one point: #16.
fromIndexcould be expensive if it used with an index greater than the size of theFoldable1since it usesLastafter already having converted to a linked list and running through it... but I'm pretty sure it won't hit that case the way it's used anyway.