Temporarily insource Scalacheck #5244
Conversation
| @@ -550,6 +549,7 @@ lazy val junit = project.in(file("test") / "junit") | |||
| fork in Test := true, | |||
| libraryDependencies ++= Seq(junitDep, junitIntefaceDep, jolDep), | |||
| testOptions += Tests.Argument(TestFrameworks.JUnit, "-a", "-v"), | |||
| testFrameworks -= new TestFramework("org.scalacheck.ScalaCheckFramework"), | |||
|
Locally, I'm seeing a problen in vs 2.12.0-M4 has yet another idea: |
|
Are we compiling scalacheck with the right compiler? |
|
It is compiled with STARR in the SBT build. Not 100% sure about the Ant build. |
|
Test failure here: |
|
I think this mix of compilers is the problem. It seems to me the proposal to go with a private published scalacheck from the bootstrap build for one commit is the easier solution. |
So could the test failures be explained by the fact that scalacheck is compiled with STARR, while the tests are compiled with quick? The failures could be a result of different super call semantics, right? |
|
This PR is not based on the trait-statics PR. I don't think there is a material difference between STARR and quick in super calls. But anything is possible... |
|
Right, but we have changed other things about the encoding, like #5085 |
|
I believe that the scalacheck tests might been neutered since the change to remove unnecessary forwarders. They seem to rely on mixin in a |
|
#5207 is probably related, too. |
By "support" you mean display a warning? It's a good step, but I wouldn't call it "support". We do provide actual support in a compiler plugin, though: https://github.com/scala-js/scala-2.12-junit-mixin-plugin |
retronym
force-pushed
the
topic/insource-scalacheck
branch
from
be7ba72
to
c2252d2
Compare
|
RIght, I think the I just rebased this on top the latest 2.12.x, which includes the switch to SBT based PR validation. That change does not skip locker, so at least the NaN problem should be gone. |
|
@sjrd We may yet need to add the forwarders back in as we're facing a non-neglible performance hit without them. 😦 |
|
The reported failure in the We could avoid the problem by using To troubleshoot further, I'm using |
|
The root problem (that is being masked by the incorrect shrinking) is that this throws an unsupported operation error in 2.11 and 2.12 I'm not sure why that wasn't being caught by the Scalacheck tests before. |
|
Wow this just gets weirder and weirder. Maybe we've never been testing what we thought we were testing? 😳 Sounds like I should've been more keen on insourcing when you suggested it. |
|
One confounding factor is that I failed to insource exactly the same version of Scalacheck as we'd been using before. I'm going to rebase to include a commit to upgrade first and see what that tells me. The actual fix for the |
This is a temporary measure until we release Scala
2.12.0. It means we are able to release milestones,
and RCs of Scala without needing a public release of
Scalacheck. While we've never had to wait very long
for these in the past (Thanks, Rickard!) we'd like
to spare the maintainer some work betwen now and 2.12.0.
After we release Scala 2.12.0, we'll revert to a binary
dependency on the standard Scalacheck.
I have replaced the scala-parser-combinator based
command line option parsing with a quick and dirty
version.
I've had to remove scalacheck as a SBT test framework
in our build. We don't use it directly as such (instead,
it is used indirectly through `partest --scalacheck`),
and it's test discovery (which we expect to return nothing)
fails after re-STARR-ing due to an unsolved problem with
SBT's testLoader including either STARR or sbt-launch.jar
on the classpath used to discover and spawn tests.
For the record, I tried the following to no avail:
```
// Two modifications are needed from the stock SBT configuration in order to exclude STARR
// from the classloader that performs test discovery.
// - We make `isManagedVersion` hold by providing an explicit Scala version, in order to go into the desired
// branch in `createTestLoader`
// - We remove STARR from the classloader of the scala instance
def fixTestLoader = testLoader := {
val s = scalaInstance.value
val scalaInstance1 =
new ScalaInstance(s.version, appConfiguration.value.provider.scalaProvider.loader(), s.libraryJar, s.compilerJar, s.extraJars, Some(s.actualVersion))
assert(scalaInstance1.isManagedVersion)
TestFramework.createTestLoader(Attributed.data(fullClasspath.value), scalaInstance1, IO.createUniqueDirectory(taskTemporaryDirectory.value))
}
```
f
retronym
force-pushed
the
topic/insource-scalacheck
branch
from
c2252d2
to
fa20291
Compare
|
Okay, the last piece of the puzzle was the fact that I had stubbed out the Scalacheck option parser and just used the defaults, which meant our custom option of Details of my fix for this in the new commit comment |
|
@som-snytt DYR whether your addition of |
|
@retronym I haven't clicked that diff yet, but it would have to be "replicate existing". Edit: Yes, the commented-out |
|
I'm running the ant build locally to verify it passes. |
| println(cdoub) | ||
| property("padTos must be equal") = forAllNoShrink(collectionPairsWithLengths) { case (s, coll, len) => | ||
| // assert(s.size == coll.size, (s.size, coll.size)) | ||
| println("=" + coll.size + "/" + coll.getClass) |
There was a problem hiding this comment.
Using ant this gets to the console -- intentional?
There was a problem hiding this comment.
Nope, that's stray debugging. Removed.
This was throwing a UnsupportedOperationError for small operations. The parallel collections test suite sets `-minSuccessfulTests 5` in test/files/scalacheck/parallel-collections/pc.scala, which is far lower thatn the default of 100, and means that we are less likely to falsify properties. This parameter seems to have been added in scala#2476, assuming I'm reading it correctly. Not sure of the motiviation, perhaps just to make the slowest part of the scalacheck test suite run faster? I haven't changed the paramater now, but instead have included a one element collection in generator. I also found that when the test failed, Scalacheck would try to minimize the example, but did so assuming that the elements of the tuple of test data could be independentally shrunk. This breaks the invariant that the two collections contain equal elements, and led to spurious error reports. I have disabled shrinking in all tests tests affected by this.
retronym
force-pushed
the
topic/insource-scalacheck
branch
from
fa20291
to
1ae80e8
Compare
|
LGTM! |
|
I'm seeing a problem using a distribution built with It would be possible to tweak the SBT class loading logic to accommodate this, but before doing that I'd like to be sure that we really do need |
|
See the discussion here for more detail of how this manifests itself in SBT. |
This is a temporary measure until we release Scala
2.12.0. It means we are able to release milestones,
and RCs of Scala without needing a public release of
Scalacheck. While we've never had to wait very long
for these in the past (Thanks, Rickard!) we'd like
to spare the maintainer some work betwen now and 2.12.0.
After we release Scala 2.12.0, we'll revert to a binary
dependency on the standard Scalacheck.
I've had to remove scalacheck as a SBT test framework
in our build. We don't use it directly as such (instead,
it is used indirectly through
partest --scalacheck),and it's test discovery (which we expect to return nothing)
fails after re-STARR-ing due to an unsolved problem with
SBT's testLoader including either STARR or sbt-launch.jar
on the classpath used to discover and spawn tests.
For the record, I tried the following to no avail: