Skip to content

Use mockito-scala-scalatest to enable strict mocks on all tests. #255

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

Merged
merged 1 commit into from
Feb 4, 2021

Conversation

ultrasecreth
Copy link
Contributor

Inspired by the comment on #172 this PR uses the ScalaTest integration of mockito-scala so all mocks are strict by default.

An easy way to verify this would be to re-write the first test on LoggerSpec as (notice the extra direct call to underlying.info

    "call the underlying logger's error method if the error level is enabled" in {
        val f = fixture(_.isErrorEnabled, isEnabled = true)
        import f._
        logger.error(msg)
        underlying.info("called by accident")
        verify(underlying).error(msg)
    }

Executing such test would fail as

Unexpected invocations found

The following invocations are unexpected (click to navigate to relevant line of code):
1. logger.info("called by accident"); -> at com.typesafe.scalalogging.LoggerSpec.$anonfun$new$2(LoggerSpec.scala:26)
Please make sure you aren't missing any stubbing or that your code actually does what you want
org.mockito.exceptions.misusing.UnexpectedInvocationException: Unexpected invocations found

This change has already paid by itself as it shown that the tests on LoggerWithTaggedArgsSpec L#68 and L#77 were not testing anything as the wrong log level was being enabled

when(p(underlying)).thenReturn(isEnabled)
when(canLogCorrelationId.logMessage(anyString(), any[CorrelationId])).thenReturn(logMsg)
if (stubCanLog) when(canLogCorrelationId.logMessage(any[String], any[CorrelationId])).thenReturn(logMsg)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Strict mode also means that if you stub something and is not used you'll get a failure as it indicates your code is not doing what the test expected it to do, hence the flag to only stub this for the tests that do a positive verification

Comment on lines +30 to +32
logger.error("This should not throw: {}, {} - {}", arg1, arg2, arg3)
}
verify(underlying).error(any[String], *, *, *)
Copy link
Contributor Author

@ultrasecreth ultrasecreth Feb 3, 2021

Choose a reason for hiding this comment

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

Had to add the verification due to strict-mode (if a mock method is called, and it wasn't either stubbed nor verified the test will fail)
I had to add the 3rd param as the macro always calls the varargs version of the Slf4j class, but on the direct call to verify, the compiler would pick the overloaded non-vararg version and the verification would fail as they are effectively different methods. By using 3 arguments the problem goes away as the only option is the varargs method

}
}

"Calling debug with tagged args" should {

"not throw ClassCastException when varargs are passed" in {
val f = fixture(_.isTraceEnabled)
val f = fixture(_.isDebugEnabled)
Copy link
Contributor Author

@ultrasecreth ultrasecreth Feb 3, 2021

Choose a reason for hiding this comment

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

This test and the next were actually not testing anything, as the incorrect log level was being enabled, thanks strict mode!! :)

@SethTisue
Copy link
Collaborator

I don't know anything about Mockito... is there anyone watching this repo who can review this?

(If we can't find anybody, I guess I'll merge regardless after a waiting period...)

@SethTisue SethTisue merged commit cedbc55 into lightbend-labs:master Feb 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants