eed3si9n / verify

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

Why use object instead of class? #26

Closed eatkins closed 4 years ago

eatkins commented 4 years ago

I know that this is forked from minitest, which uses object for test suites, but I somewhat prefer classes ala ScalaTest or Specs2. I'm struggling to remember why, but I remember having an issue in swoval because utest used objects for test suites instead of classes. It doesn't matter if you're just using a build tool test runner, but I think classes are more flexible in general.

alexandru commented 4 years ago

What does a class allow you to do that an object doesn't?

I find object to be much better for testing, because an object has its contract pretty clear.

An object cannot be instantiated. This means, for example, that an object cannot have state. Can you say that about a class? When you see a class, can you say anything about its lifecycle during the tests? You couldn't because it depends on the implementation.

If you had an issue with object, it's probably because your tests used shared state, assuming a lifecycle. Otherwise I see no other reason.

And note that with TestSuite[Env] you can have an environment recreated and destroyed for each test, along with a global setup and teardown if you wish. The difference being that the object keeps you honest.

Minitest is used by Monix, which has thousands of tests running for both JS and the JVM, it's one of the better tested libraries in the ecosystem and we never had any problems with object.

eatkins commented 4 years ago
object SomethingTest extends TestSuite[Int] {
  private var system: ActorSystem = _

  override def setupSuite(): Unit = {
    system = ActorSystem.create()
  }

  override def tearDownSuite(): Unit = {
    TestKit.shutdownActorSystem(system)
    system = null
  }
}

would become

class SomethingTest extends TestSuite[Int] {
  private val system: ActorSystem = ActorSystem.create()

  override def tearDownSuite(): Unit = {
    TestKit.shutdownActorSystem(system)
  }
}

and now I can run SomethingTest twice using the same classloader without resorting to mutable variables. I can also concurrently run multiple instance of the tests to see if any flakiness arises with additional concurrency. Like I said in the initial comment, this doesn't really matter if you are just running tests with sbt or another build tool, but if you are doing something more exotic with a custom runner, there are advantages to class compared to object.

swoval, which implements file system monitoring in sbt 1.3.0, uses utest which also uses object. It has roughly 250 tests that are nearly all asynchronous and run on jvm and js. I am not saying that object is a horrible design choice or that it can't be worked around. I just think that class is more flexible. Given that this is a strawman, I figured I'd voice that opinion.

alexandru commented 4 years ago

@eatkins in your sample you're dealing with mutable state anyway, the object is just more honest about it.

Also I think mutable state shared between multiple tests is absolutely terrible. In Minitest I didn't even want to add setupSuite and teardownSuite.

This happened due to Akka and Akka only, because creating that actor system is super expensive. But this does bring back memories of me testing actors. Because the actor system is shared, you can end up polluting its internal queues and so failure in a test can also make other tests fail and then you have to chase down the test that actually failed and that brought down the rest.

This is why I think that:

  1. Tests for actors are integration tests, not unit tests, because they require an expensive actor system that should be treated as an external resource
  2. Testing actors wouldn't be necessary if actors were used as they were meant to be used (e.g. Erlang style immutable state that can be divorced from actor communications)
  3. Usage of Akka shouldn't influence the design of a testing library

Your opinions are always welcome btw and this is no longer my project so it's not even up to me, suggestions welcome 👍

SethTisue commented 4 years ago

fwiw, I switched from JUnit to scala-verify, and one of my test cases that used to run in a fraction of a second started taking two minutes, I believe because of the usual JVM pitfall of code in the constructor not getting JITted. arguably that was my fault for writing code that was arguably a bit trashy? but it's a really easy pit to fall into (in Scala even more so than Java)

eatkins commented 4 years ago

Another obvious problem with using objects for tests is that it makes it possible for a test to inadvertently leak a resources when a different test accesses a static field or method even if the the resource is correctly cleaned up by the containing test:

object FooTest {
  val x = 1
  val y = new ThreadBackedResourceImplementingAutoCloseable
  ...
  override def tearDownSuite(): Unit = {
    y.close()
  }
}
object BarTest {
  val z = FooTest.x
}

Now if I run BarTest in an sbt session without runningFooTest, I inadvertently leaked the resource. This may seem pathological as written, but I think it's actually a pretty easy and common mistake to make, especially for less experienced developers.

dwijnand commented 4 years ago

I guess I would support adding support for classes, given the pitfalls expressed above. But I would still like objects to be supported and even be the default in the docs.

eatkins commented 4 years ago

But I would still like objects to be supported and even be the default in the docs.

Forgive my ignorance, but I genuinely don't understand the preference for object. I think I understand @alexandru's argument above, but I find it unpersuasive. From my perspective, tests inherently have a dynamic lifecycle that is managed by the test framework so class is the appropriate abstraction. When I see the word object, I see global state and the potential for resource leaks which are precisely what we want to discourage.

Really I am asking what real world problem is solved by using object that cannot be solved at the framework level using class? There are problems with object that can be fixed by switching to class but I'm failing to see what problems with class can't be addressed by the test framework. Even with objects the framework still has to manage lifecycle, see e.g. tearDownSuite.

Having classes and objects as valid test implementations seems problematic both because it introduces two ways to do the same thing which is bound to cause downstream arguments and because the name becomes ambiguous. Consider the following:

object FooTest extends Test {
  ...
}
class FooTest extends Test {
  ...
}

In sbt, how would I run one but not the other? Would I have to run FooTest$ to run the object version?

eatkins commented 4 years ago

Also I think mutable state shared between multiple tests is absolutely terrible. In Minitest I didn't even want to add setupSuite and teardownSuite.

It isn't about shared mutable state, it's about resources. In an ideal world every single test case would have its own resources. Unfortunately, that can kill performance. In many cases, whether we're talking about akka actor systems, test databases, caches, webservers, etc., the majority of the test runtime is spent creating resources. To make it concrete, suppose I want to test read only database queries. One way to do this is to fill a test database with mock data before all of the test case and drop the data after they complete. At my last job, filling a test postgres server with data could easily take 500ms to 1s while each query against that data might take 1-50ms. As a user, I don't want to be told that my test suite with 10 cases has to take 10 seconds instead of 1 second because I'm doing it wrong by sharing data between cases. When we wanted to test updates, we had to reset the data between test cases which made the tests dramatically slower.

While some might argue that these are integration tests rather than unit tests, I personally do not care about the semantic difference. The beauty of this technique is that, when using dependency injection, you don't have to mock your code, you only have to mock your data. Scalatest adapted to this style and we were able to get unit test like feedback while exercising code paths that very closely mimicked production code paths.

SethTisue commented 4 years ago

it's about resources

agree.

tests inherently have a dynamic lifecycle that is managed by the test framework so class is the appropriate abstraction

agree

dwijnand commented 4 years ago

When I see the word object, I see global state and the potential for resource leaks

I see no state, or at least no mutable state and/or side-effecting state initialisation. But I agree the framework should continue to support the lifecycle hooks.

eatkins commented 4 years ago

To clarify, what I meant is that I ask myself, "what could go wrong with this abstraction?" I think object is a valuable concept in scala and overall prefer it to java style static fields and methods but it does have some pitfalls.

dwijnand commented 4 years ago

Yeah, I've gained a newfound respect for Java's lack of object.

it does have some pitfalls.

Agreed. "convenient but with pitfalls", very Scala-esque.

In sbt, how would I run one but not the other? Would I have to run FooTest$ to run the object version?

Or maybe FooTest. for the object and FooTest# for the class (ala Semanticdb: https://scalameta.org/docs/semanticdb/specification.html#symbol)

eed3si9n commented 4 years ago

Is either class or object clear about exactly when the test suits are created and when they are destroyed after I type testOnly x.y.z into sbt shell? Aren't both somewhat unclear / up to the runner implementation?

Maybe picking class would be more to do with familiarity since that's the style used by JUnit and ScalaTest? Either case, I agree with Ethan that it would be good to pick either class or object but not both.

alexandru commented 4 years ago

@eatkins

That example with the resource leak can very well happen with classes as well:

class FooTest {
  val x = 1
  val y = new InputStream(...)
  ...
  override def tearDownSuite(): Unit = {
    y.close()
  }
}

class BarTest {
  val z = new FooTest().x
}

This is still a leak and the garbage collector will not help.

I do wonder if we can support both classes and objects.

alexandru commented 4 years ago

@eatkins

Having classes and objects as valid test implementations seems problematic both because it introduces two ways to do the same thing which is bound to cause downstream arguments and because the name becomes ambiguous.

Introducing two ways to do the same thing isn't necessarily bad, we aren't dogmatic about it like in Python.

Also if something hurts, like in the case of defining both a class and an object with the same name, then you stop doing it.

alexandru commented 4 years ago

@SethTisue

fwiw, I switched from JUnit to scala-verify, and one of my test cases that used to run in a fraction of a second started taking two minutes, I believe because of the usual JVM pitfall of code in the constructor not getting JITted. arguably that was my fault for writing code that was arguably a bit trashy? but it's a really easy pit to fall into (in Scala even more so than Java)

Performance issues might be a valid reason for having classes.

eatkins commented 4 years ago

@alexandru sure, you can create the same type of leak using class but it is far less likely. In particular, implicit classes must be defined in an object, not a class, so what my example might actually look like in practice is: InetAddressTest.scala:

object InetAddressTest extends Test {
  implicit class InetStringOps(private val s: String) extends AnyVal {
    def toInetAddress: java.net.InetAddress = new java.net.InetAddress(s)
  }
  val y = new TheThingThatWillLeakIfNotClosed
  ...
}

Bar.scala:

import InetAddressTest._
object BarTest extends Test {
  val blah = "127.0.0.1".toInetAddress
  ...
}

This example is much more difficult to write if tests must be defined in a class because you wouldn't even be able to define the implicit class inside of the test implementation. A counter to this is: "well you should put the implicit class definition in a different object than the test object," but this seems exactly like the kind of thing that companion objects were designed for. And besides, to me this isn't so much about right and wrong but what kind of mistakes are users likely to make and how can we help them avoid them?

As an sbt maintainer, I am very sensitive to resource leaks. It is easy to write application code that leaks resources but never causes any problems because it is expected that the resource lives for the duration of the program. In long running sbt sessions, those same resource leaks cause various problems which will generally manifest as either sbt running out of memory or performance degrading so badly that sbt must be restarted (which is frustrating given how long sbt can take to start up). For me, the use of object makes it more likely that our downstream users run into problems. Of course there is no way to prevent users from writing resource leaks in general but I will continue to push for the patterns that reduce their likelihood.

eatkins commented 4 years ago

Introducing two ways to do the same thing isn't necessarily bad, we aren't dogmatic about it like in Python.

I agree that it isn't necessarily bad but that doesn't mean it shouldn't be avoided if possible.

eatkins commented 4 years ago

Is either class or object clear about exactly when the test suits are created and when they are destroyed after I type testOnly x.y.z into sbt shell? Aren't both somewhat unclear / up to the runner implementation?

Yes, but at least we can assume that if class is used then each test implementation will be initialized in the same place in the runner code. With objects, it is pretty much impossible to say when any particular test implementation is initialized since it depends on whether and how dependent test implementations access each other's object fields and/or methods.

alexandru commented 4 years ago

@eatkins

How often have you seen test fields being reused in other tests, thus creating leaks?

Anecdotally speaking, I can't think of any one such instance. I've never seen it in any of the projects I participated it. Of course we might not be the target demographic and maybe this is a consequence of using actual FP lately and describing resources via Resource.

And I'm aware that people find creative ways to shoot themselves in the foot, but I don't find this as being a compelling argument against objects, mostly because you're arguing against simple functions. Personally I like simple functions and I'm pretty much anti-classes at this point 80% of the time 🙂

Most leaks are file handle leaks. And in the case for tests, no matter the type of the leak, mutable resources being reused means that a test can pollute other tests and that's always bad. To me this is super obvious and even though I'm not in the target demographic ...

I don't see a good reason to make resource handling safer with a construct that in fact fails to do that and fails hard. Putting things in a class instead of an object is simply not a good way to manage resources.

But personally I can be persuaded by anecdotes, so do you have any such stories?

Or in other worlds, why in the world would somebody define extension methods in the object of a test? Or reusable resources for that matter that need to be imported in other tests? These sound like stories meant for the daily wtf.

  • Doctor, it hurts when I do this
  • Then don't do that

😛

alexandru commented 4 years ago

@eatkins

Also anecdotally speaking — working with object makes people think that they shouldn't initialize all kinds of garbage in the class's constructor, to be shared between tests — because sharing between tests, with few exceptions, is an anti-pattern.

dwijnand commented 4 years ago

why in the world would somebody define extension methods in the object of a test

Cos extension methods read nicer than functions.

eatkins commented 4 years ago

@alexandru I have made my points as clearly as I intend to. I have been writing scala for about 8 years and have seen resource leaks in all forms. I am 100% sure that I have seen some form of the examples I have provided show up in the real world but I'm sorry that I can't recall every specific bug that I have seen over the years. I have no illusions that my experience is comprehensive but I have provided numerous examples in this thread and you keep coming back with the no true scotsman argument. It is exhausting and I'm not particularly inclined to keep debating the validity of my concerns.

Most leaks are file handle leaks. And in the case for tests, no matter the type of the leak, mutable resources being reused means that a test can pollute other tests and that's always bad. To me this is super obvious and even though I'm not in the target demographic ...

This is an opinion presented as fact.

mostly because you're arguing against simple functions.

When have I ever argued against simple functions?

Also anecdotally speaking — working with object makes people think that they shouldn't initialize all kinds of garbage in the class's constructor, to be shared between tests — because sharing between tests, with few exceptions, is an anti-pattern.

This is an opinion that you are projecting onto the world at large. I think a lot of, if not most, people don't think much at all about what they should or shouldn't do in an object or class constructor.

Or in other worlds, why in the world would somebody define extension methods in the object of a test?

Because people don't design things up front. They add an extension method for one test and then later they write another test and think, "hey, I added an extension method in Foo, let me just use it in Bar!" You may think that's dumb, but that's how code often gets written.

I don't see a good reason to make resource handling safer with a construct that in fact fails to do that and fails hard. Putting things in a class instead of an object is simply not a good way to manage resources.

I guess java never should have introduced the AutoCloseable interface then?

All that I have heard from your comments in this thread is that you have a strong opinion about how scala code should be written and the library is designed to facilitate coding in your preferred style. That is fine for Minitest but this is a library that aspires to be a de-facto standard which means people with all kind of styles will use it.

dwijnand commented 4 years ago

Btw, with the new lambda encoding it's much easier than before to shoot yourself in the foot with objects, see this REPL bug which I've been working on recently: https://github.com/scala/bug/issues/9076.

It's because object initialisation is bound to class initialisation, which blocks all calls from other threads, so you can deadlock yourself if your object initialisation accesses the object from another thread. Here's a nice and terse example:

// This works fine:
val x = Future(42)
Await.result(x, 1.second)

// This throws a TimeoutException:
Await.result(Future(42), 1.second)
joan38 commented 4 years ago

In this case should we change:

object Main extends App { ... }

by:

class Main extends App { ... }

?

To me a test is just another entrypoint like a main. I always found ScalaTest weird to use classes instead of objects and was actually happy to see verify using an object.

alexandru commented 4 years ago

On 2/13/2020 7:04 PM, Dale Wijnand wrote:

It's because object initialisation is bound to class initialisation, which blocks all calls from other threads, so you can deadlock yourself if your object initialisation accesses the object from another thread.

I'm not exactly sure what the Scala compiler does, but in general if this escapes the current thread during the execution of the constructor, bad things can happen. But this is relevant for classes too, being a JMM gotcha related to classes.

I know that the code you posted misbehaves in the REPL, it's an old problem visible when using Await.result on a Future(...). I don't see the relevance though, unless I'm missing something.

I admit that I haven't looked carefully at what causes that particular problem in the REPL, but if we are talking about the poor man's way of avoiding cyclic dependencies (also see https://github.com/lihaoyi/acyclic), then this isn't relevant because we are arguing against usage of plain functions.

alexandru commented 4 years ago

@eatkins

I do value your opinions and for whatever it is worth, will take them under consideration.

This is an opinion that you are projecting onto the world at large.

Note my use of the word "anecdotally". I'm very much aware that we're talking of opinions here - in general software development isn't engineering and computer science isn't an actual science, as I like pointing out the field isn't evolved via the scientific method and in general, if we are not talking about math, then we are talking about opinions and anecdotes. That goes without saying. And compared with others however, I am very careful in underlying my anecdotes and admitting my opinions.

I don't like your tone though, even if I'm aware this is the norm in many communities (Reddit and elsewhere) and this discussion is no longer productive, so we'll agree to disagree.

Cheers,

dwijnand commented 4 years ago

I'm not exactly sure what the Scala compiler does, but in general if this escapes the current thread during the execution of the constructor, bad things can happen. But this is relevant for classes too, being a JMM gotcha related to classes.

I guess, if you will, it's about "degrees" of "badness", maybe? If I'm guessing correctly, this escaping the current thread during construction means that the class won't have fully constructed, meaning its fields won't have been all initialised, which means nulls. For class loading (and, as I said, Scala's singleton object initialising) it's (arguably) worst: it means deadlocks. (Or, counter-arguably, better because it means hard fail, but at least fast fail 😄).

I know that the code you posted misbehaves in the REPL, it's an old problem visible when using Await.result on a Future(...). I don't see the relevance though, unless I'm missing something.

Do you mean old problem as in old problem in the REPL, or in general? The problem I'm thinking about became more of a problem ever since the Java 8 / Scala 2.12 lambda encoding. It's relevant here because it triggers in the REPL because the REPL (currently, we're working on trying to change it) uses object wrappers as in its implementation (there's no real evaluator - the E in REPL is a lie :o).

if we are talking about the poor man's way of avoiding cyclic dependencies (also see https://github.com/lihaoyi/acyclic), then this isn't relevant because we are arguing against usage of plain functions.

In my eyes I see it as arguing for what encoding is better (and, later, if both encodings should be supported). As I stated in the beginning, I prefer object. So, at minimum, I'd like objects supported. I'm happy to give up the docs promoting objects, as I'm becoming more and more convinced that it's a footgun for beginners. If such beginners then want to choose the more strict FP style of Scala, then they'll have to learn about the pitfalls of side-effecting and so forth, but then rejoice in being able to use simple functions in an object. But perhaps when they're still beginners they shouldn't be tested on this while setting up their... tests. 😄

alexandru commented 4 years ago

@dwijnand I'm coming around to the opinion that class should be supported as well. The good thing about supporting both is that a beginner can't go wrong.

Not sure how the implementation would work.

One thing I really dislike about class here is that instantiation relies on calling a no-parameters constructor via reflection. I'm guessing it is possible to do in Scala.js and Scala Native as well, along with some test to see if the object or the class should be executed. Or at least I hope it's possible, given we don't have the same reflective capabilities as on the JVM.

eatkins commented 4 years ago

I don't like your tone though, even if I'm aware this is the norm in many communities (Reddit and elsewhere) and this discussion is no longer productive, so we'll agree to disagree.

@alexandru I regret that the tone of this discussion has indeed devolved. I try to keep things civil and respectful and am personally dismayed by much of the discussion that exists around scala on the internet. That being said, I'd challenge you to reflect on how you have engaged with me in this thread. From your first response to my initial comment, you wrote:

If you had an issue with object, it's probably because your tests used shared state, assuming a lifecycle. Otherwise I see no other reason.

This bothered me because it reads to me like you're accusing me of incompetence whether or not that was your intent. Similar to your claim that I "was arguing against simple functions," it felt like you were putting words in my mouth forcing me to defend a claim that I never made. Moreover, you made a number of strong judgments on various stylistic choices, e.g. "why in the world would somebody define extension methods in the object of a test?" that I find completely reasonable. I made a choice to respond the way that I did that was a direct reaction to the emotional content of your previous comments. It admittedly wasn't my best look but it didn't happen in a vacuum.

At any rate, I no longer wish to participate in this discussion so, as you said, we'll have to agree to disagree.

I am closing this issue because I no longer want to receive notifications.

eed3si9n commented 4 years ago

I am saddened by this communication breakdown since I have lots of respect for both @eatkins and @alexandru, and I feel like both have tried to make sincere cases. Maybe we can cool off for a few days, and someone else can open a new issue with pros/cons summary.

alexandru commented 4 years ago

@eatkins

I apologize if my communication style put you off.

I have strong opinions that I should probably tone down. Rest assured that it wasn't my intention to accuse anyone going against my opinions of incompetence, nor do I have such thoughts. At times I'm also half joking and forget to add the smileys, as if my interlocutor could see the body language.

Online communications are hard ¯_(ツ)_/¯

Your input is very valuable and I think the folks here will seriously think of supporting or switching to class.

eatkins commented 4 years ago

@alexandru water under the bridge. I apologize for overreacting.