Refined failed in the community build

[non]

Extracting the error from CI we get:

[refined] [info] ! NonNegShift.shift Byte: Gave up after only 0 passed tests. 501 tests were discarded.
[refined] [info] ! NonNegShift.shift Short: Gave up after only 0 passed tests. 501 tests were discarded.
[refined] [info] ! NonNegShift.shift Int: Gave up after only 0 passed tests. 501 tests were discarded.
[refined] [info] ! NonNegShift.shift Long: Gave up after only 0 passed tests. 501 tests were discarded.
...
[refined] [info] ! NegShift.shift Byte: Gave up after only 0 passed tests. 501 tests were discarded.
[refined] [info] ! NegShift.shift Short: Gave up after only 0 passed tests. 501 tests were discarded.
[refined] [info] ! NegShift.shift Int: Gave up after only 0 passed tests. 501 tests were discarded.
[refined] [info] ! NegShift.shift Long: Gave up after only 0 passed tests. 501 tests were discarded.

It sounds like @ashawley wasn't able to reproduce these. I'm going to read these specific tests tests to see if I can figure out what's going on. Given that the tests were discarded (not failed) it's possible a change to our generation logic may have caused problems.

[non]

Alright, I understand the problem. The failing property looks like this:

    def createProperty[A: Arbitrary: Min: NonNegShift](implicit num: Numeric[A]): Prop = {
      import num.{abs, gteq, lt, plus, zero}

      forAll { a: A =>
        gteq(a, zero) ==> (NonNegShift[A].shift(a) == a)
      } &&
      forAll { a: A =>
        lt(a, zero) ==> (NonNegShift[A].shift(a) == plus(a, abs(Min[A].min)))
      }
    }

The issue is that the two forAll statements are generating the same a values. Since the conditions are mutually exclusive, we cannot find any examples that pass both preconditions.

I think it's likely that changes around deterministic testing are causing this problem. Either that, or we are not updating seeds appropriately. I'll dig into this a bit and report back.

cc @ashawley

[non]

I figured it out. It's a bug with the interaction between viewSeed, &&, and mutually-incompatible properties that's manifesting because we're now using viewSeed everywhere.

[ashawley]

Ok, glad you figured it out. I will quit bisecting then, which I didn't even get a chance to start. ^_^

I didn't think we had any changes to seeds or deterministic testing in this release, but I wasn't aware that it could be a side effect of using viewSeed everywhere.

Because the failing test in refined was about numbers, the only other thing I thought of were changes #380 and #451, but those deal with floating point.

[non]

I'm about to submit a PR to fix this with a test case. Definitely take a look and see what you think.

[non]

#531

[ashawley]

Yes, I can confirm that your change in #531 fixed it. I'm still trying to follow the reasoning behind your fix and the refined test, so thanks for providing the long write-up in the commit message.

This entire thing reminds me of something I wrote about about && and || in #425 (comment)

Indeed, it seems the current implementation in ScalaCheck will combine two properties with && in to one, and then the generators are run for each of the separate properties simultaneously to produce a pair of values where both properties are true. I understand the confusion from properties becoming combined and then tested, rather than tested atomically and then, and only then, their results combined. This could be especially confusing if you assign your properties to variables, as you have done, which are later be combined in an actual property. [...]

It's a long discussion that is difficult to follow. A lot of the back and forth was trying to find idiomatic examples, minimize and generally clarify the issue(s). There's a good chance that your change in #531 addresses those problems somehow.

[non]

Alright, I'm going to merge #531 then. Thanks for confirming the fix.

[non]

I'm closing this -- I'll try to head over to #425 to chime in there, although it's unlikely I'll be able to do that today.

[ashawley]

Yeah, fine to merge. We need to keep the community build process going.

[ashawley]

Any idea if viewSeed could have a different implementation to avoid this defect?