Mockito, testing, nulls and exceptions

[morgen-peschke]

@eyalroth

Speaking of testing, I wonder how this affects mocking frameworks such as Mockito, which heavily rely on null s for both implementation and semantics.

I’m very interested in this particular interaction, as Mockito is the most common source of NPEs I have to deal with

[eyalroth]

@morgen-peschke

I’m very interested in this particular interaction, as Mockito is the most common source of NPEs I have to deal with

And usually with Mockito it means that a mock was invoked with the wrong arguments. It’s a more permissive way of testing; the restrictive way is to throw a particular exception when a mock is invoked with the wrong arguments instead of returning null. In any case, it usually doesn’t mean that the tested code is expected handle nulls.

[tarsa]

@eyalroth

Adding to that, I rarely encounter NullPointerException s, and when I do it’s discovered very early in testing and is easily fixed (just Option(nullableExpression) ).

I use nulls explicitly in tests to designate parameters that shouldn’t be used by tested logic. In general, NPEs in tests are exceptions like any other exception - they allow us to find problems quickly, before things go to production. OTOH exceptions in production code are evil as they can break user experience. Treating exceptions in production and exceptions in tests differently is the only sane way to use them effectively, I think.

@eyalroth

Speaking of testing, I wonder how this affects mocking frameworks such as Mockito, which heavily rely on null s for both implementation and semantics.

Mockito relies heavily on reflection so there should be no problem for Mockito to supply Nones instead of nulls (if that’s what you want) . Here’s my quick stab at it:

package mocking

import org.mockito.{Answers, Mockito}
import org.mockito.invocation.InvocationOnMock
import org.mockito.stubbing.Answer

import scala.util.Try

object NonesForNulls {
  trait Behavior {
    def greet(): Unit
  }
  case class Struct(fieldOpt: Option[Integer] = None,
                    none: None.type = None,
                    field: Behavior)

  def main(args: Array[String]): Unit = {
    println("With default settings:")
    val mock1 = Mockito.mock(classOf[Struct])
    println(mock1.fieldOpt)
    println(mock1.none)
    println(mock1.field)
    println("...added Option[_] and None awareness:")
    val mock2 = Mockito.mock(
      classOf[Struct],
      optionAwareAnswer(Answers.RETURNS_DEFAULTS)
    )
    println(mock2.fieldOpt)
    println(mock2.none)
    println(mock2.field)

    println("With smart nulls:")
    val mock3 = Mockito.mock(classOf[Struct], Answers.RETURNS_SMART_NULLS)
    println(mock3.fieldOpt)
    println(mock3.none)
    println(mock3.field)
    Try(mock3.field.greet()).failed.foreach(_.printStackTrace(System.out))
    println("...added Option[_] and None awareness:")
    val mock4 = Mockito.mock(
      classOf[Struct],
      optionAwareAnswer(Answers.RETURNS_SMART_NULLS)
    )
    println(mock4.fieldOpt)
    println(mock4.none)
    println(mock4.field)
    Try(mock4.field.greet()).failed.foreach(_.printStackTrace(System.out))
  }

  // this method is probably wrong
  // I haven't tested Options in nested mocks
  def optionAwareAnswer(delegate: Answer[AnyRef]): Answer[AnyRef] = {
    invocation: InvocationOnMock =>
      val returnType = invocation.getMethod.getReturnType
      if (returnType.isAssignableFrom(None.getClass)) {
        None
      } else {
        delegate.answer(invocation)
      }
  }
}

There’s mockito-scala library so maybe that was already done.

[eyalroth]

@tarsa

I use nulls explicitly in tests to designate parameters that shouldn’t be used by tested logic. In general, NPEs in tests are exceptions like any other exception - they allow us to find problems quickly, before things go to production. OTOH exceptions in production code are evil as they can break user experience. Treating exceptions in production and exceptions in tests differently is the only sane way to use them effectively, I think.

I completely disagree with the last statement, and particularly because we’re talking about runtime exceptions, which are never intended to be thrown and imply a bug. Bugs in tests are as bad as bugs in production.

Runtime exceptions are certainly not like any other exception. I believe this confusion stems from the shallow adoption of the Java exception model in Scala. NPE – like any other runtime exception – happens when a programmer has made a mistake. The question is whether this kind of a mistake is common enough, and whether it’s hard to fix once discovered. I believe that’s not the case with NPE.

@tarsa

Mockito relies heavily on reflection so there should be no problem for Mockito to supply Nones instead of nulls (if that’s what you want).

There’s much more reliance on null in Mockito. A lot of that is implementation detail, but it is crucial nonetheless. For instance, argument matchers in fact return nulls:

argThat(...) == null
when(foo.bar(argThat(...))) // the mock (foo) is invoked with a `null` argument

[jducoeur]

@eyalroth

Speaking of testing, I wonder how this affects mocking frameworks such as Mockito, which heavily rely on null s for both implementation and semantics.

Personally, I think Mockito has no business being in a Scala codebase – it is too type-weak and null-happy. Scalamock is a little better, but still leads to too many nulls. Java code tends to be cautious about null, because they are common there, but Scala code assumes there aren’t any nulls floating around, so it tends to just crash.

I have a vague ambition (and I’d encourage somebody else to pick up the ball and run with it before I get to it) to fork Scalamock to change one aspect: when you hit an undefined method, immediately throw an Exception, instead of silently injecting a null that will cause confusing errors later. While that wouldn’t be perfect, I suspect it would work better in practice…

[eyalroth]

@jducoeur

Personally, I think Mockito has no business being in a Scala codebase – it is too type-weak and null-happy.

Personally, I prefer Mockito over any other mocking framework – Scala or Java. It has its caveats, but I strongly believe in its core testing methodology (stub mocks → invoke tested unit → assert output), its API is relatively simple, and it has a lot of (useful) features.

@jducoeur

I have a vague ambition (and I’d encourage somebody else to pick up the ball and run with it before I get to it) to fork Scalamock to change one aspect: when you hit an undefined method, immediately throw an Exception, instead of silently injecting a null that will cause confusing errors later. While that wouldn’t be perfect, I suspect it would work better in practice…

In Mockito, you have several options to achieve that; explicitly using verifyNoMoreInteractions on every test case; defining a Strictness test rule; or even returning “smart nulls” instead of plain nulls. See this SO question for details.

[sighoya]

Java code tends to be cautious about null, because they are common there

In Java, NPE’s are mostly a non issue these days because of the @NotNull Annotations on all the entry points of a library. It has, however, the drawback to induce runtime reflection to do the work which is bloated and slow.

[tarsa]

Scala doesn’t have checked exceptions so all throwables are treated the same. NullPointerException is basically the same as IOException.

Using null for instances is like using scala.Predef.??? for method implementations. Within test code, do you ban ??? as much as you ban explicit nulls?

@eyalroth

The question is whether this kind of a mistake is common enough, and whether it’s hard to fix once discovered. I believe that’s not the case with NPE.

I’m talking about test code. Spurious failures in tests don’t affect production code. Since tests are made for frequent running, NPEs will surface quickly and you’ll have to fix them before pushing your changes to master branch, unless you don’t care about tests {but then why would you care about any kind of bugs in tests?}.

[jducoeur]

(Thanks to whoever ripped this out into a more appropriate thread.)

@tarsa

I’m talking about test code. Spurious failures in tests don’t affect production code. Since tests are made for frequent running, NPEs will surface quickly

The problem is, they don’t surface reliably – I’ve found Mockito to be pretty brittle in practice, with test code working fine for years and then crashing due to an apparently unrelated change that is now exercising a nulled-out pathway. And the relationship between the source of the error and where it actually manifests often isn’t especially obvious, because the null can flow silently through the code before something dereferences it. Debugging Mockito problems is a frequent source of pain.

So my rule at work is to avoid it: use proper separation of interface and implementation, and write more serious mocks manually, that are more carefully thought out. It’s a little more work upfront, but I’ve found it to be much more maintainable.

(I suspect that there is a good middle ground to be found, with semi-automated mocks that are more typeful and which allow easy overriding when necessary, but that’s a research project yet to come, probably in Scala 3.)

[morgen-peschke]

@tarsa

Using null for instances is like using scala.Predef.??? for method implementations. Within test code, do you ban ??? as much as you ban explicit null s?

Yes. Yes, we do.

[sighoya]

And the relationship between the source of the error and where it actually manifests often isn’t especially obvious, because the null can flow silently through the code before something dereferences it. Debugging Mockito problems is a frequent source of pain.

I think this has nothing to do with NPE’s at all, it more relates to debugging runtime generated code during RR which is heavily used in any common Java project.
The main problem is that annotations aren’t checked at compile time, they request data for it and when they aren’t available RR crashes at runtime with exceptions that no one understands and no one can backtrack because the stack don’t exist anymore.

But Scala macros aren’t enough to solve the problem, macros are generally local only, a global observer like a VM or a compiler is needed to gather all the necessary information needed for code generation and compatibility.

[scottcarey]

Runtime reflection is not needed. Compile time tools can assert warnings or errors and that is as far as it goes for java nullability annotations in most cases. Some frameworks inject extra null checks into the bytecode at compile time (e.g. Intellij). I am unaware of ones that do runtime reflection on annotations.

[eyalroth]

@tarsa

Scala doesn’t have checked exceptions so all throwables are treated the same. NullPointerException is basically the same as IOException.

I cannot stress how incorrect this statement is. The java exception hierarchy is not just about checked vs non-checked exceptions. That mechanism is just another layer on top of that hierarchy.

There is a more fundamental difference between the throwables; Error is for non-recoverable situations (reduced to NonFatal in Scala); RuntimeException is for programming mistakes that couldn’t be discovered during compilation; lastly, all the other Exceptions are indication of exceptional-yet-possible situations that the program needs to deal with.

This misconception – that Java’s exceptions are all about checked vs non-checked – is common, even in the Java world. Much like any other concept borrowed by Scala from Java – for instance, global equality and hashcode per object – this concept is an integral part of Scala too.

@tarsa

Using null for instances is like using scala.Predef.??? for method implementations. Within test code, do you ban ??? as much as you ban explicit nulls?

No one is encouraging the usage of nulls for instances in any code. If you’re referring to my last mention of argument-matchers in Mockito, then that is just an implementation detail that a user shouldn’t care or be aware of. I agree that it’s not ideal – and I even suggested an alternative solution on their repo – but it’s not that bad either and I can understand why this is not a top priority task for them.

@tarsa

@eyalroth

The question is whether this kind of a mistake is common enough, and whether it’s hard to fix once discovered. I believe that’s not the case with NPE.

I’m talking about test code. Spurious failures in tests don’t affect production code. Since tests are made for frequent running, NPEs will surface quickly and you’ll have to fix them before pushing your changes to master branch, unless you don’t care about tests {but then why would you care about any kind of bugs in tests?}.

I’m not sure what is your argument. For me, NPEs are uncommon, both in production and in test code. When they do occur – either in production or test code – they will surface early in tests (because this is where I first run my code), and will be easily fixed. I don’t see how the SIP will help me improve my code or my development process.

[eyalroth]

@jducoeur

The problem is, they don’t surface reliably – I’ve found Mockito to be pretty brittle in practice, with test code working fine for years and then crashing due to an apparently unrelated change that is now exercising a nulled-out pathway. And the relationship between the source of the error and where it actually manifests often isn’t especially obvious, because the null can flow silently through the code before something dereferences it. Debugging Mockito problems is a frequent source of pain.

I don’t believe Mockito has much to do with this problem; the tool is fairly simplistic and direct. This sort of problem will happen in any code base with not enough separation between code units, and not enough fine-grained tests for those units. If all you have are large tests which test the functionality of entire units of logic in just a few cases, then you’ll end up with this problem regardless of which testing tool you’re using.

And again, in Mockito you have the option to immediately fail a test once a mock was invoked in a way that was not stubbed.

@jducoeur

So my rule at work is to avoid it: use proper separation of interface and implementation, and write more serious mocks manually, that are more carefully thought out. It’s a little more work upfront, but I’ve found it to be much more maintainable.

What you are referring to are “fakes” in the testing lingo. They are entirely different than mocks and stubs (which are quite similar). They have their uses, but I personally don’t use them as much. Regardless, “fakes” are outside the scope of any mocking framework out there – be it Mockito or ScalaMock – and advocating for the sole usage of them goes against any of these frameworks.