Reduce maxSize and minSuccessful in Scalacheck params #952

ceedubs · 2016-03-27T13:11:46Z

This is an experiment to see if builds time out less if we don't run as
many samples with Scalacheck.

codecov-io · 2016-03-27T15:57:49Z

Current coverage is `90.82%`

Merging #952 into master will increase coverage by +1.89% as of eec72f9

@@            master    #952   diff @@
======================================
  Files          179     184     +5
  Stmts         2133    2181    +48
  Branches        42      43     +1
  Methods          0       0       
======================================
+ Hit           1897    1981    +84
  Partial          0       0       
+ Missed         236     200    -36

Review entire Coverage Diff as of eec72f9

Powered by Codecov. Updated on successful CI builds.

ceedubs · 2016-03-27T16:41:08Z

This build passed the first time (after I fixed my compile error :|). I'm going to restart it and see if it succeeds again.

ceedubs · 2016-03-27T17:50:27Z

This build succeeded twice in a row. We might want to do something like this. We can tweak the parameters as others see fit. I'm not too concerned about how much this will reduce test cases because:

The current status of most builds failing is the worse of two evils I think.
These tests are run quite often (every PR, every merge to master), so over time we should still get quite a few test cases in.
As far as setting maxSize to 5 (from the previous 100): Due to the abstract nature of Cats, I think that generally if things work with 5 elements they will work with 100. Stack overflows are the most likely exception, but 100 still probably isn't enough to trigger them unless we are doing something quadratic in nature.

adelbertc · 2016-03-27T19:29:37Z

LGTM 👍

mikejcurry · 2016-03-27T20:05:33Z

tests/src/test/scala/cats/tests/CatsSuite.scala

@@ -20,8 +20,11 @@ trait TestSettings extends Configuration with Matchers {

  lazy val checkConfiguration: PropertyCheckConfiguration =
    PropertyCheckConfiguration(
-      minSuccessful = if (Platform.isJvm) PosInt(100) else PosInt(10),
-      maxDiscardedFactor = if (Platform.isJvm) PosZDouble(5.0) else PosZDouble(50.0))
+      minSuccessful = if (Platform.isJvm) PosInt(5) else PosInt(5),


Minor, but the if expression is doing nothing here.

@mikejcurry thanks for catching my hastiness :). Fixed.

This is an experiment to see if builds time out less if we don't run as many samples with Scalacheck.

mikejcurry · 2016-03-27T22:05:12Z

That's 3 passed builds in a row on this ticket I think. :-) - FWIW, I say 👍 - lot's of PRs with failed builds at the moment and this could resolve a lot of them.

It might also make sense to have an issue recorded to revisit and see if there is somewhere in between 5 and 100 that works - I guess the more tests run in a single run the better, so somewhere in between, there might be a sweet spot :-)

ceedubs · 2016-03-28T11:05:34Z

Some changes on the scala.js side have improved the failing build situation a lot. I think there is still benefit in something like this but we might not want quite as drastic of a reduction as I had previously made.

travisbrown · 2016-04-11T12:33:02Z

To be honest my vote would be to drop Scala.js from the Travis CI builds and check it locally before releases. That's annoying, but it would cut two-thirds (or more) of the time off of PR builds and eliminate the flakiness.

I'd be a little worried about ScalaCheck hitting the same cases most of the time for some types with minSuccessful this small.

ceedubs · 2016-04-18T12:24:34Z

@travisbrown I can see your argument for dropping scala.js from the Travis builds. It makes me a bit nervous, though. I don't want us to get into a situation where a PR is merged and then months later when we go to do a release we realize that approach isn't going to work at all in the scala.js world. Would there be a way to avoid that?

For now I've used Platform.isJVM to reduce the sample size/count in the JS world while keeping it fairly high for the JVM world. If this PR build passes several times in a row then it might be a good-enough-for-now solution. I'd really like to see an end to the sporadic build failures, and since they have improved a lot after some recent scala.js changes, I'm hopeful that this could be the extra inch that's needed.

mikejcurry · 2016-04-18T20:23:51Z

laws/src/main/scala/cats/laws/discipline/Eq.scala

@@ -12,8 +13,10 @@ object eq {
   * and comparing the application of the two functions.
   */
  implicit def function1Eq[A, B](implicit A: Arbitrary[A], B: Eq[B]): Eq[A => B] = new Eq[A => B] {
+    val sampleCnt: Int = if (Platform.isJvm) 50 else 5


Would it be better to keep the JVM side at 100? Is there example builds where this has failed for lack of output when building the JVM side?

This is a fair question. When I wrote this my thought process was that these tests run (presumably with a different RNG seed) on every PR and every merge to master, so 50 samples should be plenty, especially given the general nature of cats. I thought this might just speed things up a bit. I don't have any strong opinions on this and am happy to change it if people would prefer.

@mikejcurry I'm going to go ahead and merge this PR to help make the build reliable. Feel free to open up a followup issue if you have any concerns about this lower number.

@ceedubs - I've no strong feelings. I just wanted to raise the question, and the build speed is a good argument as the cats end to end build has certainly gotten longer. I just wanted to raise the question. :-)

non · 2016-04-18T20:40:42Z

I think we should do this. Making sure the JS tests run is important (and we should be running the tests manually e.g. before a release) but for Travis I think this is a better default.

non · 2016-04-18T20:40:45Z

👍

mikejcurry reviewed Mar 27, 2016
View reviewed changes

Reduce maxSize and minSuccessful in Scalacheck params

8608018

This is an experiment to see if builds time out less if we don't run as many samples with Scalacheck.

ceedubs force-pushed the scalacheck-params branch from cc3e95e to 8608018 Compare March 27, 2016 20:51

ceedubs mentioned this pull request Apr 11, 2016

Add flatMapF for StateT #973

Merged

Use more/bigger scalacheck samples on JVM than JS

2e7b4ff

ceedubs closed this Apr 18, 2016

ceedubs reopened this Apr 18, 2016

ceedubs added the in progress label Apr 18, 2016

mikejcurry reviewed Apr 18, 2016
View reviewed changes

ceedubs merged commit ed194a7 into typelevel:master Apr 18, 2016

ceedubs deleted the scalacheck-params branch April 18, 2016 21:04

ceedubs removed the in progress label Apr 18, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce maxSize and minSuccessful in Scalacheck params #952

Reduce maxSize and minSuccessful in Scalacheck params #952

ceedubs commented Mar 27, 2016

codecov-io commented Mar 27, 2016

ceedubs commented Mar 27, 2016

ceedubs commented Mar 27, 2016

adelbertc commented Mar 27, 2016

mikejcurry Mar 27, 2016

ceedubs Mar 27, 2016

mikejcurry commented Mar 27, 2016

ceedubs commented Mar 28, 2016

travisbrown commented Apr 11, 2016

ceedubs commented Apr 18, 2016

mikejcurry Apr 18, 2016

ceedubs Apr 18, 2016

ceedubs Apr 18, 2016

mikejcurry Apr 18, 2016

non commented Apr 18, 2016

non commented Apr 18, 2016

Reduce maxSize and minSuccessful in Scalacheck params #952

Reduce maxSize and minSuccessful in Scalacheck params #952

Conversation

ceedubs commented Mar 27, 2016

codecov-io commented Mar 27, 2016

Current coverage is 90.82%

ceedubs commented Mar 27, 2016

ceedubs commented Mar 27, 2016

adelbertc commented Mar 27, 2016

mikejcurry Mar 27, 2016

Choose a reason for hiding this comment

ceedubs Mar 27, 2016

Choose a reason for hiding this comment

mikejcurry commented Mar 27, 2016

ceedubs commented Mar 28, 2016

travisbrown commented Apr 11, 2016

ceedubs commented Apr 18, 2016

mikejcurry Apr 18, 2016

Choose a reason for hiding this comment

ceedubs Apr 18, 2016

Choose a reason for hiding this comment

ceedubs Apr 18, 2016

Choose a reason for hiding this comment

mikejcurry Apr 18, 2016

Choose a reason for hiding this comment

non commented Apr 18, 2016

non commented Apr 18, 2016

Current coverage is `90.82%`