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

Remove dependency on Specs2 #563

Merged
merged 1 commit into from Nov 17, 2013
Merged

Conversation

retronym
Copy link
Member

Scalaz is an implementation dependendecy for Specs2. This makes it tricky to
use Specs2 to test Scalaz itself.

Historically, we have either:

  • used a older relesae of specs2 (hoping it was binary compatible
    with the new Scala release)
  • failing that, published an untested scalaz-core and waited
    for Specs2.

This seems a dangerous state of affairs, and one that is most simple resolved
by using a different test framework.

This commit does exactly that, using Scalacheck with a small facade to retain
a measure of source compatibility with our existing tests.

  • Adds SpecLite, a replacement for Specs
    • This contains a facade over Scalacheck to minimize
      the amount of changes we need to make to test sources.
    • Converts test cases to use this

@retronym
Copy link
Member Author

Review by @larsrh. /cc @etorreborre.

I think this could use a bit more work to achieve better consistency in the tests (in vs ! etc), but I have to leave that to the Scalaz team.

The main risk in this change is that I have turned some tests into no-ops. I'd suggest you review by inserting failures into some tests and making sure they are reported, and seeing how well they are reported. We will have lost a little bit of pretty reporting, but the added checks (must_===) still show the expected/actual.

@retronym
Copy link
Member Author

Just repushed this to import forAll rather than fully qualify it.

@xuwei-k
Copy link
Member

xuwei-k commented Oct 14, 2013


def checkAll(name: String, props: Properties) {
for ((name2, prop) <- props.properties) yield {
property(s"$name:$name2") = prop
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should not use string interpolation

property(name + ":" + name2) = prop

@retronym
Copy link
Member Author

Rebased to migrate the newly added FutureTest, and converted string interpolation to plain old concatenation.

case ex: Throwable =>
if (!man.runtimeClass.isInstance(ex))
fail("wrong exception thrown, expected " + man.runtimeClass + " got: " + ex)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://travis-ci.org/scalaz/scalaz/jobs/12556914#L706

[error] /home/travis/build/scalaz/scalaz/tests/src/test/scala/scalaz/SpecLite.scala:94: value runtimeClass is not a member of ClassManifest[T]
[error]         fail("no exception thrown, expected " + man.runtimeClass)
[error]                                                     ^
[error] /home/travis/build/scalaz/scalaz/tests/src/test/scala/scalaz/SpecLite.scala:97: value runtimeClass is not a member of ClassManifest[T]
[error]           if (!man.runtimeClass.isInstance(ex))
[error]                    ^
[error] /home/travis/build/scalaz/scalaz/tests/src/test/scala/scalaz/SpecLite.scala:98: value runtimeClass is not a member of ClassManifest[T]
[error]             fail("wrong exception thrown, expected " + man.runtimeClass + " got: " + ex)
[error]

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed by using the (now deprecated) .erasure.

@xuwei-k
Copy link
Member

xuwei-k commented Oct 15, 2013

LGTM

@markhibberd
Copy link
Contributor

This seems somewhat rash to me. Has there been more discussion elsewhere?

I can see the problem that is trying to be addressed, but it seems that this will just shift it slightly where downstream libraries of specs2 & scalaz will continue to have to face the same (or at least related) issues in that there needs to be this cross-product of scala, scalaz & specs2 versions, that are increasingly difficult to keep in sync when you want to ship a library across multiple versions of scala.

I feel like the old, status-quo of namespacing scalaz for specs2 is better than the current approach (even with this change), but I think it would be nice if there was some discussion of how to handle this slightly more holistically, so that libraries that want to depend on a platform of scalaz/shapeless/specs2 by default have an agreed way to handle this beyond Eric cranking out a never ending splattering of specs2 versions tweaked for every combination of scala & scalaz. I find it difficult to swallow that this problem is just too hard, and libraries start spawning their own test frameworks all over the place to avoid it.

Does anyone have any more ideas on this? @etorreborre?

@etorreborre
Copy link
Contributor

An idea might be a fail-proof repackaging plugin, but I'm not sure how hard it is to do.

On 15/10/2013, at 9:06 PM, markhibberd notifications@github.com wrote:

This seems somewhat rash to me. Has there been more discussion elsewhere?

I can see the problem that is trying to be addressed, but it seems that this will just shift it slightly where downstream libraries of specs2 & scalaz will continue to have to face the same (or at least related) issues in that there needs to be this cross-product of scala, scalaz & specs2 versions, that are increasingly difficult to keep in sync when you want to ship a library across multiple versions of scala.

I feel like the old, status-quo of namespacing scalaz for specs2 is better than the current approach (even with this change), but I think it would be nice if there was some discussion of how to handle this slightly more holistically, so that libraries that want to depend on a platform of scalaz/shapeless/specs2 by default have an agreed way to handle this beyond Eric cranking out a never ending splattering of specs2 versions tweaked for every combination of scala & scalaz. I find it difficult to swallow that this problem is just too hard, and libraries start spawning their own test frameworks all over the place to avoid it.

Does anyone have any more ideas on this? @etorreborre?


Reply to this email directly or view it on GitHub.

@retronym
Copy link
Member Author

I've been discussing this with Lars and Eric by mail.

The primary problem we are trying to solve is avoiding the cycle. If we publish a new compiler that breaks the ABI (e.g changing the way we pickle types), Lars and Eric have to go through as coordination to: 1. publish an untested version of scalaz-core and scala-concurrent; 2. use this to build specs; 3. test scalaz post factum.

This problem is constrained to projects that specs depends on.

@markhibberd
Copy link
Contributor

@retronym Ok, I misunderstood that part of the problem. But in the situation (or all the time actually) wouldn't depending on specs2 as a git ref remove this loop anyway? It seems like a pretty good mutual test run.

I have done this on one project just because specs2 didn't have a published version, and given how long it takes to compile scalaz from scratch anyway the overhead seems acceptable (at least for me) given the alternatives. Although I do admit to significant bias at this point, because I generally end up running against a custom build of scalaz to get fixes when I need them anyway.

@larsrh
Copy link
Contributor

larsrh commented Oct 15, 2013

@retronym already summarized the mail thread, but let me elaborate a bit more:

scalaz is an implementation dependency of specs2, hence a specs2 without an explicit scalaz dependency is probably out of reach. As already indicated, not only is repacking harder to maintain, but also quite the kludge. Repacking is not an option (anymore). (Another argument against repackaging is the unnecessary blowup of the classpath size.)

Without repackaging, we end up with a cyclic dependency which pretty much encourages horrible build practices by making it very hard to reproduce a build. It also requires @etorreborre and me to move forward in an interleaved fashion, which slows us both down. It prevents specs2 and scalaz from evolving independently. That problem cannot be solved by depending on a git ref. On the contrary, it makes the build even messier.

Now, depending on specs2 gives us great benefits for the tests. I'm just not sure that the benefit outweighs the problems which are introduced by the build cycle. scalaz is a widely used library, and often by people who are happy to experiment with milestone releases of both Scala and libraries. With the removal of the specs2 dependency, we could accelerate the speed of which scalaz can track new Scala developments.

@retronym
Copy link
Member Author

@etorreborre A repackaging plugin would solve a different problem. It would allow Specs2 users to use a different, incompatible version of Scalaz from the one you use as an implementation dependency.

@nuttycom
Copy link
Contributor

I think that this is a pretty important problem to solve, honestly - there have been a number of times in my projects where I've had to depend upon a fork of scalaz while changes were not yet ready to be merged back upstream, but still wanted to be able to use specs2 without having to deal with classpath hackery. Having specs2 depend upon scalaz without repackaging, from my perspective, means that the two are essentially a single project from the perspective of the end user, at least when you're choosing to use specs2 (and I do.)

@larsrh @etorreborre what is the reason that you no longer consider repackaging an option?

That being said, I think breaking the cycle makes perfect sense.

@larsrh
Copy link
Contributor

larsrh commented Oct 16, 2013

@larsrh @etorreborre what is the reason that you no longer consider repackaging an option?

Maintenance overhead, basically. It's difficult to get right and produces a load of essentially-duplicate artifacts.

Having specs2 depend upon scalaz without repackaging, from my perspective, means that the two are essentially a single project from the perspective of the end user, at least when you're choosing to use specs2 (and I do.)

I'm well aware of that. I think the long-term solution to this problem is for specs2 to adapt a binary-compatibility policy. We have already adopted that for scalaz, which allows projects like specs2 to pull in scalaz 7.0.x without users having to worry about the concrete value of x. Then, when a new major version of scalaz gets released, specs2 could move on with it, while still maintaining an older version. Such a scheme would also help for projects like scalaz-specs2, for which at the moment, there is a combinatorial explosion of possible dependencies..

Obviously, that will be a burden for @etorreborre, but it's less of a burden than repackaging. In the end, it's Eric's choice.

@larsrh
Copy link
Contributor

larsrh commented Oct 16, 2013

Obviously, that will be a burden for @etorreborre, but it's less of a burden than repackaging. In the end, it's Eric's choice.

I should add: Of course, I'd be happy to help out in making binary compatibility happen for specs2.

@etorreborre
Copy link
Contributor

@nuttycom @larsrh yes this is a maintenance problem for me. I can try a binary compatibility scheme after 2.3 where I am going towards modularisation.

However, Lars I don't see how this solves Kris problem. If he wants to use a version of Scalaz that has non-compatible changes, things might break. On the other hand I'm not using much in Scalaz (yet):

  • Monoid, Semigroup
  • Monad, Applicative
  • Traverse, Foldable, Reducer
  • Tree, TreeLoc
  • NonEmptyList
  • Show, Equal
  • Memo
  • Promise

I might be wrong but I expect those things to be pretty stable in the future unless there are things like traits reorganisation (I don't know exactly what defines binary compatibility). Anyway Lars, thanks for proposing your help I will indeed need it (after 2.3).

@retronym
Copy link
Member Author

While modularization and binary compatibility for Specs itself is worthy and important, I don't think it is necessary to solve @nuttycom's problem. You are free to use Spec2 with your own fork of Scalaz, provided that you keep that fork binary compatible with the version of Scalaz that Specs ships with. (In practice, with the subset of Scalaz that Specs uses).

@adriaanm has (experimentally) patched the Maxine bytecode verifier to be able to detect if there are potential linkage errors on a classpath. You could use that against your fork and Specs2 to keep things in sync. This would give you more freedom than the strict checks imposed by MiMa.

I'd still like to get a Scala-aware version of JarJar working. Sometimes you have no better choice, especially when you are down near the bottom of the dependency pile. To make it work in Scala, we will also need to parse the pickled signatures and apply the package translation.

Here's a (slightly out of date) project that parses pickled signatures to display them as .dot graphs: https://github.com/retronym/scala-pickled-visualizer.

If anyone is interested in trying this out, I'm happy to answer any questions.

@retronym
Copy link
Member Author

I don't know exactly what defines binary compatibility

Scalaz N+1 is binary compatible with Scalaz N if any program (like specs) that was compiled against N will run against N+1 without LinkageErrors.

Changes to behaviour (even changing all the method bodies to ???) are a separate sort of compatibility. ("Semantic compatiblity").

The MiMa docs have a bit more background: https://github.com/typesafehub/migration-manager/wiki

Scalaz is an implementation dependendecy for Specs2. This makes
it tricky to use Specs2 to test Scalaz itself.

Historically, we have either:
  - used a older relesae of specs2 (hoping it was binary compatible
    with the new Scala release)
  - failing that, published an untested scalaz-core and waited
    for Specs2.

This seems a dangerous state of affairs, and one that is most
simple resolved by using a different test framework.

This commit does exactly that, using Scalacheck with a small
facade to retain a measure of source compatibility with our
existing tests.

 - Adds SpecLite, a replacement for Specs
   - This contains a facade over Scalacheck to minimize
     the amount of changes we need to make to test sources.
 - Converts test cases to use this
@retronym
Copy link
Member Author

Just rebased on top of my other patch for Scala 2.11 compatibility.

@larsrh
Copy link
Contributor

larsrh commented Oct 19, 2013

@retronym Jason, thanks a lot for your efforts. I added your branch to the main repository, so that we can stabilize and test (as you indicated your comment) everything.

(I'll leave this pull request open, should further discussion become necessary.)

@retronym
Copy link
Member Author

Any thoughts on merging this one? I'd like to have this in place Scala 2.11.0-M7, which is just around the corner.

@larsrh
Copy link
Contributor

larsrh commented Nov 15, 2013

I'm +1 for merging.

@markhibberd
Copy link
Contributor

@larsrh @retronym @etorreborre Just a heads up that there are potentially further issues now because of scalacheck changes. Scalaz and Specs both depend on scalacheck (and different versions now 1.10 vs 1.11) which means scalaz-scalacheck-binding no longer works among other things. Don't think it changes this PR but it is at least somewhat related to the compatibility + testing story.

For reference most of my downstream code fails with things like this now:

[info]    ! equals laws
[error]  NoSuchMethodError: : org.scalacheck.Gen.map2(Lorg/scalacheck/Gen;Lscala/Function2;)Lorg/scalacheck/Gen;  (Arbitrary.scala:64)
[error] org.scalacheck.Arbitrary$$anon$3.arbitrary$lzycompute(Arbitrary.scala:64)
[error] org.scalacheck.Arbitrary$$anon$3.arbitrary(Arbitrary.scala:64)
[error] org.scalacheck.Arbitrary$.arbitrary(Arbitrary.scala:68)
[error] org.scalacheck.Arbitrary$$anonfun$arbEither$1.apply(Arbitrary.scala:262)
[error] org.scalacheck.Arbitrary$$anonfun$arbEither$1.apply(Arbitrary.scala:262)

I plan to investigate further and see if I can come up with anything, but I don't see any obvious ways to address this besides everyone running MiMa and agreeing to some loose set of versions that should work together because right now it is just chaos. I know it has been tried and failed before but I am probably willing to put up some hardware, infrastructure and time to try to get a base "make scala useful" platform for type level things + specs2 + scalacheck. Thoughts?

@larsrh
Copy link
Contributor

larsrh commented Nov 16, 2013

@markhibberd We'll move to scalacheck 1.11 after this PR is merged.

@larsrh
Copy link
Contributor

larsrh commented Nov 17, 2013

Merging in progress.

larsrh added a commit that referenced this pull request Nov 17, 2013
Remove dependency on Specs2
@larsrh larsrh merged commit 55240f7 into scalaz:scalaz-seven Nov 17, 2013
@larsrh
Copy link
Contributor

larsrh commented Nov 17, 2013

Once again, big thanks to @retronym for his continuing efforts!

@xuwei-k
Copy link
Member

xuwei-k commented Nov 21, 2013

backport 7.0.x branch?

@retronym
Copy link
Member Author

Pretty Please :)

@larsrh
Copy link
Contributor

larsrh commented Nov 21, 2013

I won't have time to do it, so if anybody is willing to send a pull request, that'd be good :-)

@larsrh larsrh mentioned this pull request Nov 21, 2013
@retronym
Copy link
Member Author

I've done about 80% of the merge here but also might not have time to push this over the line for a few days. Help welcome!

https://github.com/retronym/scalaz/compare/scalaz:series%2F7.0.x...topic%2Fspec2-lite-backport?expand=1

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

Successfully merging this pull request may close these issues.

None yet

6 participants