eed3si9n / verify

Scala Verify is a minimal testing framework for Scala. Trust, but verify.
Apache License 2.0
92 stars 21 forks source link

Drop macros #34

Open dwijnand opened 4 years ago

dwijnand commented 4 years ago

While I understand the value some appreciate from "power assertions" and similar, I think that this library should keep things simple and not introduce its own macros.

I agree that sourcecode's macros should roll into Scala proper, and once that's the case this library can make use of that. But nice-to-haves like power assertions should probably live elsewhere, on some "shiny things" shelf.

JUnit seems to have done well without needing macros.

Here's an example of the kind of pitfalls of using macros: https://github.com/lampepfl/dotty/issues/7128

sjrd commented 4 years ago

I agree. JUnit's API is actually very friendly and reports very good error messages. Power asserts are overrated.

eed3si9n commented 4 years ago

I'd say having a well-functioning power assertion macro in the core library has equal or more value to having scala-verify itself, and same for SourceCode. After all, people are able to test things today using whatever test frameworks, and they could also adopt JUnit 4 or 5 if that fits their needs. The point of scala-verify is to implement something that's cross-platform, zero-dependency, and minimal.

My interpretation of minimal here is not the-smallest-implementation-there-could-be (that would be Predef.assert), but aiming to remove unnecessary API or things user needs to memorize. I'm not saying that it's the best thing in this life (Paul said that), but power assertion IMO has decent enough UX for very little to memorize, which is the essence of minimalism.

Also the fact that we are surfacing problems with Dotty (or my attempt at Dotty macros) is a feature, not a bug. Dotty is still in beta, and by having a library that tries to use it we can catch its bugs and fix either scala-verify or Dotty itself.

dwijnand commented 4 years ago

I welcome experiments that excercise Dotty's macros, but that's not a good reason to use them here.

Power assertions can always be added later, if we find we can't live without them.

alexandru commented 4 years ago

One reason I never bothered to introduce fancy macro-powered assertions in Minitest is that whenever I felt the need for a better error message, I would use the optional hint parameter, which I believe is what people also do in JUnit.

Turning from:

assert(x < y)

To:

assert(x < y, s"$x < $y")

Doesn't seem to be that much of a big deal. Similarly for other types of messages.

Having a "power asserts" micro-library in the ecosystem would be great though and this nanotest-strawman lib could even provide quick integration with it, as a sub-project.

I like to say that the biggest barrier to entry for Scala beginners are the macros and the macro-enabled libraries, due to all of the edge-cases and the weird errors, that beginners are destined to fall into.

lrytz commented 4 years ago

if we find we can't live without them

Doesn't seem to be that much of a big deal

JUnit seems to have done well without needing macros

I can follow these points, yes we can live without them. But I don't agree that this means it's better not to have them in this library. I agree with @eed3si9n, it's a nice, easy to use, time-saving feature.

dwijnand commented 4 years ago

I didn't like utest's macros for reasons I expect to find using this/expecty's macros: it's fiddly on unimportant details such as whether you inline or val things. In utest's case I inlined things and the output was of no use. Here I expect to find much the same.

Take the example in https://github.com/eed3si9n/expecty#code-examples. If you write

assert(person.say(word1, word2) == "pong pong")

then you get

assert(person.say(word1, word2) == "pong pong")
       |      |   |      |      |
       |      |   ping   pong   false
       |      ping pong
       Person(Fred,42)

But if you chose to allocate the result of calling person.say to a val, then you get nothing useful. Or if you don't allocate the person but define it inline (assert(Person("Fred", 42).say ...) then you get even more output.

Now imagine Person is a 10 field case class, and I have a List of 3 of them and do:

assert(List(fred, bob, john) == foo.getOwners)

Now I'm getting 3x10 fields for the 3 Persons, then another 3x10 fields for the LHF List itself, then another 3x10 fields for the RHS List. How does that look? How does it truncate? How customisable is it?

This whole power assertion thing looks great on paper, but I think it doesn't work well in practice.

Power assertion look to me like a fad, much like the 10 different styles of doing "unit testing" the ScalaTest ships with.

I suggest withholding adding such fads to such an important library I hoped this library would become.

lrytz commented 4 years ago

The most useful feature is printing the left and right values if an assertion fails, ideally with some nice diffing. I agree that seeing all the sub-expressions is not crucial and has various pitfalls. To underline that, in the example it's not obvious to spot the "ping pong" string (which is the left value of the assertion) between all the other output.

alexandru commented 4 years ago

I can follow these points, yes we can live without them. But I don't agree that this means it's better not to have them in this library. I agree with @eed3si9n, it's a nice, easy to use, time-saving feature.

It's a time-saving feature, but consider that it's the kind of feature that needs to be maintained and maintenance isn't necessarily cheap.

Macros are sensitive to changes in the compiler's internals, their API being unstable still, so whenever a new Scala version happens, you end up doing fixes for supporting the new version, or waiting on compiler plugins to be published first.

Not sure if nanotest-strawman still wants to support Scala 2.10 or 2.11 for example, but I've had many problems with 2.10 and even 2.11 in getting macros to work. I crashed that compiler so many times with no idea of how or why, desperately trying out changes in order to make it not crash. Working with macros in Scala, at least with 2.10, but also in newer versions, involves a lot of tribal knowledge (e.g. this tree is now dirty, don't do that twice, this expression can trigger this or that issue so tree needs to be rewritten, etc).

And we might say that we should no longer care about Scala 2.10 or 2.11, however if maintaining a 2.10 build is hard, then adopting Scala 2.14 or other future versions will be hard as well.

So in my opinion, a feature might be useful, but maintainer time matters more, especially if you want this library to be always available on new Scala versions and platforms, which I assume is the purpose of the project.

lrytz commented 4 years ago

Remotely related, we can consider integrating https://github.com/xdotai/diff

dwijnand commented 4 years ago

I'm not sure if scala.quoted macros will be stable in Scala 3, but https://github.com/scala/nanotest-strawman/pull/44 is a fresh data point on the "Macros are sensitive to changes in the compiler's internals" case.

julienrf commented 4 years ago

If the goal of this lib is to have something that we can easily publish for each Scala version then we should probably move the macros to some optional module because they will for sure require more upgrade work.

dwijnand commented 4 years ago

https://github.com/eed3si9n/expecty is that other module (in my eyes).

dwijnand commented 4 years ago

Tried to see if the new Dotty and Verify releases fixed https://github.com/lampepfl/dotty/issues/7128#issuecomment-571505284, but sadly not. Verify is still unusable as a testing library on Dotty due to its use of macros.

SethTisue commented 4 years ago

Verify is still unusable as a testing library on Dotty

meta-issue: why isn't CI telling us that?

eed3si9n commented 4 years ago

I don't know why CI isn't catching some use cases but the diff like

-    import qctx.tasty._
+    import qctx.tasty.{ _, given }

in https://github.com/scala/nanotest-strawman/pull/44 has nothing to do with macros. It's to do with implicit/givens related language changes that are happening in Dotty. This is somewhat expected if you write a library on top of a language that's not feature frozen. https://contributors.scala-lang.org/t/preparing-for-feature-freeze/3780 said 0.20 will be the last, but there's another PR to change the syntax again.

dwijnand commented 4 years ago

Of course it does:

  1. the reason that I couldn't use (and continue to not be able to use) scala-verify is because of macros: https://github.com/lampepfl/dotty/issues/7128
  2. the reason that scala-verify was unusable with Dotty 0.20 (#41) and upgrading from Dotty 0.19 to 0.21 took a while (#44) is becomes of macros
dwijnand commented 4 years ago

Also the fact that we are surfacing problems with Dotty (or my attempt at Dotty macros) is a feature, not a bug. Dotty is still in beta, and by having a library that tries to use it we can catch its bugs and fix either scala-verify or Dotty itself.

You mentioned this earlier. Why don't you make expecty that experiment, and let scala-verify be readily available to start testing across Scala/Dotty?

SethTisue commented 4 years ago

are verify and/or expecty in the Dotty community build? that could be a mechanism for making sure either or both are up to date with the latest Dotty changes ahead of time, rather than after a new Dotty is released

dwijnand commented 4 years ago

Power assertion look to me like a fad

Another recent example of power assertion being a gimmick:

[info] - cancel on-going task with string id *** FAILED ***
[info]   assertion failed
[info]   
[info]   assert(svr.waitForString(1.minute) { s =>
[info]              |             | |           |
[info]              false         | 1 minute    testpkg.EventsTest$$$Lambda$330/0x00000001003c0840@248d118
[info]                            scala.concurrent.duration.package$DurationInt@1
eed3si9n commented 4 years ago

gimmick is a bit harsh. Let's compare the two:

Predef.assert

[info] - cancel on-going task with string id *** FAILED ***
[info]   assertion failed

Verify's assert

[info] - cancel on-going task with string id *** FAILED ***
[info]   assertion failed
[info]   
[info]   assert(svr.waitForString(1.minute) { s =>
[info]              |             | |           |
[info]              false         | 1 minute    testpkg.EventsTest$$$Lambda$330/0x00000001003c0840@248d118
[info]                            scala.concurrent.duration.package$DurationInt@1

Without providing custom error message, I can tell that the failed assertion was about 1.minute timeout.