Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Nested non empty list generation fail #581

Closed
zallesov opened this issue Nov 4, 2019 · 16 comments
Closed

Nested non empty list generation fail #581

zallesov opened this issue Nov 4, 2019 · 16 comments
Milestone

Comments

@zallesov
Copy link

zallesov commented Nov 4, 2019

Some changes in 1.14.1 and 1.14.2 make the following test to fail during value generation. With the version 1.14.0, it passes.

This makes it impossible to use generators with nested collections.

package io.moia.dispatching
package execution.impl

import org.scalacheck.Gen
class PlaygroundTest extends UnitTest {

//import org.scalactic.anyvals.PosZDouble
//  implicit override val generatorDrivenConfig =
//    PropertyCheckConfiguration(maxDiscardedFactor = PosZDouble(20.0))

  "Playground test" should {

    case class Item(list: List[Int])
    case class Wrapper(list: List[Item])
    case class NestedList(list: List[List[Int]])

    val itemGen: Gen[Item] = for {
      list <- Gen.nonEmptyListOf(Gen.posNum[Int])
    } yield Item(list)

    val wrapperGen: Gen[Wrapper] = for {
      list <- Gen.nonEmptyListOf(itemGen)
    } yield {
      Wrapper(list)
    }

    val nestedGen: Gen[NestedList] = for {
      list <- Gen.nonEmptyListOf(Gen.nonEmptyListOf(Gen.posNum[Int]))
    } yield {
      NestedList(list)
    }

    "Wrapper" in {
      forAll(wrapperGen) { wrapper =>
        wrapper.list.size should (be > 0)
      }
    }

    "Nested" in {
      forAll(nestedGen) { nested =>
        nested.list.size should (be > 0)
      }
    }
  }
}

Output

[info]  PlaygroundTest:
[info] Playground test
[info] - should Wrapper *** FAILED ***
[info]   Gave up after 6 successful property evaluations. 126 evaluations were discarded.
[info] - should Nested *** FAILED ***
[info]   Gave up after 6 successful property evaluations. 167 evaluations were discarded.

I believe the change that results in the issue was introduced here.
https://github.com/typelevel/scalacheck/blob/1.14.x/src/main/scala/org/scalacheck/Gen.scala#L866

Thank you.

PS. this looks related #568
PSS. seems like it is already being fixed in master.

@ashawley
Copy link
Contributor

ashawley commented Nov 4, 2019

Yeah, I know that a fix for part of #568 was merged to master. I'd need to study this further to see if it handles your case or not.

Incidentally, there are snapshots published when commits are pushed to master. If you're using sbt, you can use them with the the following snippet:

resolvers +=
  "Sonatype OSS Snapshots" at
  "https://oss.sonatype.org/content/repositories/snapshots"

Then this is the sha version that contains the merge of #569:

libraryDependencies += "org.scalacheck" %% "scalacheck" % "1.14.2-e3883f6-SNAPSHOT" % "test"

Feel free to give it a try and let us know.

@zakpatterson
Copy link
Contributor

zakpatterson commented Nov 5, 2019

As noted this looks like a duplicate of #568 to me. In 1.14.2, Gen.posNum[Int] will fail to return a value sometimes, basically due to filtering out zeros. nonEmptyListOf, listOfN, etc all fail if the passed generator fails just once.

This does seem to work though given the sha version by @ashawley:

> Gen.nonEmptyListOf(Gen.nonEmptyListOf(Gen.nonEmptyListOf(Gen.posNum[Int]))).sample 
res1: Option[List[List[List[Int]]]] = Some(List(List(List(64, 9, 27, 72, 38, 39, 94, 49, 50, 50, 49, 53, 94, 75, 25, 28, 11, 30, 91, 13, 7, 12, 64, 4, 63, 24, 49, 51, 85, 5, 11, 96, 58, 55, 14, 95, 28, 89, 50, 72, 94, 56, 73, 51, 43, 59, 57, 46, 65, 98, 21, 1, 57, 60, 51, 63, 37, 36, 51, 52, 85, 32, 77, 41, 15, 55, 60, 8, 28, 71, 98, 57, 91, 95, 75...

@mdedetrich
Copy link

I can also confirm we hit this issue, using the snapshot fixes the regression

@mikla
Copy link

mikla commented Nov 26, 2019

I hit the same issue.
Confirm that 1.14.2-e3883f6-SNAPSHOT version fixes that.

Thanks everybody!

@mdedetrich
Copy link

Is it possible to make a new release with this fix in there considering this was a pretty serious regression?

@ashawley
Copy link
Contributor

ashawley commented Dec 3, 2019

Yes, it does seem worth releasing, and verifying that the fix doesn't have any regressions. I'm going to try and cherry-pick the change to the 1.14 branch. It will probably be a little while until 1.15 is ready.

@mdedetrich
Copy link

I'm going to try and cherry-pick the change to the 1.14 branch. It will probably be a little while until 1.15 is ready.

Does this mean there will be a 1.14b release (or something like this?)

@ashawley
Copy link
Contributor

ashawley commented Dec 3, 2019

Current version is 1.14.2. It will be 1.14.3.

@mdedetrich
Copy link

Ah right, awesome thanks!

@mdedetrich
Copy link

@ashawley Sorry to bug you but is there an ETA for this? Unfortunately we are hitting coursier/coursier#1149 which means the scalacheck snapshot isn't entirely working for us.

I can also try and set up a PR if that would speed up the process.

@djspiewak
Copy link
Member

I’m happy to do the work of making the release if someone can point me to the appropriate branch/commit. I have the appropriate access to do so.

@mdedetrich
Copy link

@djspiewak This is the PR/commit #569 . @ashawley said earlier that the safest option would be to cherry pick the above commit in the 1.14.x branch and release 1.14.3 (see #581 (comment))

@ashawley
Copy link
Contributor

Sorry for the delay. I'm just getting back from a trip, and trying to catch up on things.

Thanks for offering to help with publishing, Daniel. I will likely use it.

I'm going to work on #587 and when that's done I'll work on preparing the 1.14.3 release as Matthew described.

@djspiewak
Copy link
Member

Alrighty, I'll see about helping out a bit later today

@ashawley
Copy link
Contributor

We can continue the discussion of 1.14.3 in #591

@SethTisue
Copy link
Member

closing because it's a duplicate of #568 (and they are both fixed, though the fix isn't released yet)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants