Vary the length of Gen.listOfN #845
Conversation
| property("empty listOfN") = forAll(listOfN(0, arbitrary[Int])) { l => | ||
| l.length == 0 | ||
| } | ||
|
|
There was a problem hiding this comment.
This test of listOfN(0, ...) is redundant to the trivial test of listOfN above that is doing_.length <= n
| val g = Gen.containerOfN[List,TimeUnit](100, arbitrary[TimeUnit]) | ||
| Prop.forAllNoShrink(g) { (l: List[TimeUnit]) => | ||
| l.toSet == TimeUnit.values.toSet | ||
| } |
There was a problem hiding this comment.
I rewrote this, but it will fail occasionally. Could be just a trivial incomplete test using List#contains, since we know the underlying Gen.oneOf works correctly.
| def listOfN[T](n: Int, g: Gen[T]) = | ||
| choose(0, Integer.max(n, 0)) | ||
| .flatMap(n => buildableOfN[List[T], T](n, g)) |
There was a problem hiding this comment.
I am not sure that it is a good idea overall. Having listOfN which behaves differently from containerOfN and buildableOfN would be really confusing. Personally I like to have all of them behaving in a similar way.
Wouldn't it be better to create a separate function (or functions) for creating generators like this one above?
There was a problem hiding this comment.
Something like listOfAtMostN, containerOfAtMostN and buildableOfAtMostN :)
There was a problem hiding this comment.
As I said in #844, we might be able to consider it. We can discuss there.
|
I will just weigh in my opinion as a user - I think changing the behaviour in this way has the potential to break too many tests. Also the current behaviour is consistent with the name |
|
I agree. I raised the same concern of breaking tests in #844. The chance of generating lists less than N with |
Fixes #844.