From a8d23ed5fd54d5b9dcc03091184445386f5d40f2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9bastien?= Date: Wed, 18 May 2022 17:33:10 +0200 Subject: [PATCH 1/2] auto detect unnecessary stubbing --- README.md | 81 ++++++++++++------- dsl/common/src/main/kotlin/io/mockk/API.kt | 13 +++ .../src/main/kotlin/io/mockk/GatewayAPI.kt | 4 + .../common/src/main/kotlin/io/mockk/MockK.kt | 7 ++ .../CommonVerificationAcknowledger.kt | 24 +++++- .../io/mockk/impl/stub/ConstructorStub.kt | 4 +- .../kotlin/io/mockk/impl/stub/MockKStub.kt | 9 +++ .../main/kotlin/io/mockk/impl/stub/Stub.kt | 2 + mockk/jvm/api/mockk-jvm.api | 3 + .../kotlin/io/mockk/junit5/MockKExtension.kt | 23 +++++- .../mockk/it/VerificationAcknowledgeTest.kt | 63 +++++++++++++++ 11 files changed, 196 insertions(+), 37 deletions(-) diff --git a/README.md b/README.md index ba6a38d1c..e128a735f 100644 --- a/README.md +++ b/README.md @@ -262,6 +262,12 @@ This will internally call `confirmVerified` on all mocks after each test, to mak Please note that this behavior may not work as expected when running tests in your IDE, as it is Gradle who takes care of handling the exception being thrown when these `confirmVerified` calls fail. +#### Automatic unnecessary stubbing check + +You can make sure that all stubbed methods are useful - used at least once - by also annotating your test class with `@MockKExtension.CheckUnnecessaryStub`. + +This will internally call `checkUnnecessaryStub` on all mocks after each test, to make sure there are no unnecessary stubbings. + ### Spy @@ -718,6 +724,18 @@ verify { confirmVerified(car) // makes sure all calls were covered with verification ``` +### Unnecessary stubbing + +Because clean & maintainable test code requires zero unnecessary code, you can ensure that there is no unnecessary stubs. + +``` +checkUnnecessaryStub(mock1, mock2) +``` + +It will throw an exception if there are some declared calls on the mocks that are not used by the tested code. +This can happen if you have declared some really unnecessary stubs or if the tested code doesn't call an expected one. + + ### Recording exclusions To exclude unimportant calls from being recorded, you can use `excludeRecords`: @@ -1174,37 +1192,38 @@ Here are a few tables to help you master the DSL. ### Top level functions -|Function|Description| -|--------|-----------| -|`mockk(...)`|builds a regular mock| -|`spyk()`|builds a spy using the default constructor| -|`spyk(obj)`|builds a spy by copying from `obj`| -|`slot`|creates a capturing slot| -|`every`|starts a stubbing block| -|`coEvery`|starts a stubbing block for coroutines| -|`verify`|starts a verification block| -|`coVerify`|starts a verification block for coroutines| -|`verifyAll`|starts a verification block that should include all calls| -|`coVerifyAll`|starts a verification block that should include all calls for coroutines| -|`verifyOrder`|starts a verification block that checks the order| -|`coVerifyOrder`|starts a verification block that checks the order for coroutines| -|`verifySequence`|starts a verification block that checks whether all calls were made in a specified sequence| -|`coVerifySequence`|starts a verification block that checks whether all calls were made in a specified sequence for coroutines| -|`excludeRecords`|exclude some calls from being recorded| -|`confirmVerified`|confirms that all recorded calls were verified| -|`clearMocks`|clears specified mocks| -|`registerInstanceFactory`|allows you to redefine the way of instantiation for certain object| -|`mockkClass`|builds a regular mock by passing the class as parameter| -|`mockkObject`|turns an object into an object mock, or clears it if was already transformed| -|`unmockkObject`|turns an object mock back into a regular object| -|`mockkStatic`|makes a static mock out of a class, or clears it if it was already transformed| -|`unmockkStatic`|turns a static mock back into a regular class| -|`clearStaticMockk`|clears a static mock| -|`mockkConstructor`|makes a constructor mock out of a class, or clears it if it was already transformed| -|`unmockkConstructor`|turns a constructor mock back into a regular class| -|`clearConstructorMockk`|clears the constructor mock| -|`unmockkAll`|unmocks object, static and constructor mocks| -|`clearAllMocks`|clears regular, object, static and constructor mocks| +|Function| Description | +|--------|------------------------------------------------------------------------------------------------------------| +|`mockk(...)`| builds a regular mock | +|`spyk()`| builds a spy using the default constructor | +|`spyk(obj)`| builds a spy by copying from `obj` | +|`slot`| creates a capturing slot | +|`every`| starts a stubbing block | +|`coEvery`| starts a stubbing block for coroutines | +|`verify`| starts a verification block | +|`coVerify`| starts a verification block for coroutines | +|`verifyAll`| starts a verification block that should include all calls | +|`coVerifyAll`| starts a verification block that should include all calls for coroutines | +|`verifyOrder`| starts a verification block that checks the order | +|`coVerifyOrder`| starts a verification block that checks the order for coroutines | +|`verifySequence`| starts a verification block that checks whether all calls were made in a specified sequence | +|`coVerifySequence`| starts a verification block that checks whether all calls were made in a specified sequence for coroutines | +|`excludeRecords`| exclude some calls from being recorded | +|`confirmVerified`| confirms that all recorded calls were verified | +|`checkUnnecessaryStub`| confirms that all recorded calls are used at least once | +|`clearMocks`| clears specified mocks | +|`registerInstanceFactory`| allows you to redefine the way of instantiation for certain object | +|`mockkClass`| builds a regular mock by passing the class as parameter | +|`mockkObject`| turns an object into an object mock, or clears it if was already transformed | +|`unmockkObject`| turns an object mock back into a regular object | +|`mockkStatic`| makes a static mock out of a class, or clears it if it was already transformed | +|`unmockkStatic`| turns a static mock back into a regular class | +|`clearStaticMockk`| clears a static mock | +|`mockkConstructor`| makes a constructor mock out of a class, or clears it if it was already transformed | +|`unmockkConstructor`| turns a constructor mock back into a regular class | +|`clearConstructorMockk`| clears the constructor mock | +|`unmockkAll`| unmocks object, static and constructor mocks | +|`clearAllMocks`| clears regular, object, static and constructor mocks | ### Matchers diff --git a/dsl/common/src/main/kotlin/io/mockk/API.kt b/dsl/common/src/main/kotlin/io/mockk/API.kt index eb8a5d924..2c095eaff 100644 --- a/dsl/common/src/main/kotlin/io/mockk/API.kt +++ b/dsl/common/src/main/kotlin/io/mockk/API.kt @@ -278,6 +278,19 @@ object MockKDsl { } } + /** + * Checks if all recorded calls are necessary. + */ + fun internalCheckUnnecessaryStub(vararg mocks: Any) { + if (mocks.isEmpty()) { + MockKGateway.implementation().verificationAcknowledger.checkUnnecessaryStub() + } + + for (mock in mocks) { + MockKGateway.implementation().verificationAcknowledger.checkUnnecessaryStub(mock) + } + } + /** * Resets information associated with mock */ diff --git a/dsl/common/src/main/kotlin/io/mockk/GatewayAPI.kt b/dsl/common/src/main/kotlin/io/mockk/GatewayAPI.kt index cdce94ada..c7c070eae 100644 --- a/dsl/common/src/main/kotlin/io/mockk/GatewayAPI.kt +++ b/dsl/common/src/main/kotlin/io/mockk/GatewayAPI.kt @@ -253,6 +253,10 @@ interface MockKGateway { fun acknowledgeVerified(mock: Any) fun acknowledgeVerified() + + fun checkUnnecessaryStub(mock: Any) + + fun checkUnnecessaryStub() } interface MockTypeChecker { diff --git a/mockk/common/src/main/kotlin/io/mockk/MockK.kt b/mockk/common/src/main/kotlin/io/mockk/MockK.kt index 0cd950350..03925b817 100644 --- a/mockk/common/src/main/kotlin/io/mockk/MockK.kt +++ b/mockk/common/src/main/kotlin/io/mockk/MockK.kt @@ -319,6 +319,13 @@ fun confirmVerified(vararg mocks: Any) = MockK.useImpl { MockKDsl.internalConfirmVerified(*mocks) } +/** + * Checks if all recorded calls are necessary. + */ +fun checkUnnecessaryStub(vararg mocks: Any) = MockK.useImpl { + MockKDsl.internalCheckUnnecessaryStub(*mocks) +} + /** * Resets information associated with specified mocks. * To clear all mocks use clearAllMocks. diff --git a/mockk/common/src/main/kotlin/io/mockk/impl/recording/CommonVerificationAcknowledger.kt b/mockk/common/src/main/kotlin/io/mockk/impl/recording/CommonVerificationAcknowledger.kt index 238b6c30f..f265ab8a4 100644 --- a/mockk/common/src/main/kotlin/io/mockk/impl/recording/CommonVerificationAcknowledger.kt +++ b/mockk/common/src/main/kotlin/io/mockk/impl/recording/CommonVerificationAcknowledger.kt @@ -26,6 +26,15 @@ class CommonVerificationAcknowledger( acknowledgeVerificationHelper(stub) } + override fun checkUnnecessaryStub() { + stubRepo.allStubs.forEach { checkUnnecessaryStubHelper(it) } + } + + override fun checkUnnecessaryStub(mock: Any) { + val stub = stubRepo.stubFor(mock) + checkUnnecessaryStubHelper(stub) + } + private fun acknowledgeVerificationHelper(stub: Stub) { val allCalls = stub.allRecordedCalls().map { InternalPlatform.ref(it) }.toHashSet() val verifiedCalls = stub.verifiedCalls().map { InternalPlatform.ref(it) }.toHashSet() @@ -57,4 +66,17 @@ class CommonVerificationAcknowledger( VerificationHelpers.stackTraces(notVerified) } -} \ No newline at end of file + private fun checkUnnecessaryStubHelper(stub: Stub) { + val unnecessaryMatcher = stub.matcherUsages().filterValues { it == 0 }.keys + + if (unnecessaryMatcher.isEmpty()) return + + val report = + "Unnecessary stubbings detected.\nFollowing stubbings are not used, either because there are unnecessary or because tested code doesn't call them :\n\n" + + unnecessaryMatcher + .mapIndexed { idx, matcher -> "${idx + 1}) $matcher" } + .joinToString("\n") + throw AssertionError(report) + } + +} diff --git a/mockk/common/src/main/kotlin/io/mockk/impl/stub/ConstructorStub.kt b/mockk/common/src/main/kotlin/io/mockk/impl/stub/ConstructorStub.kt index 0ac15f9dd..d17fb9ed3 100644 --- a/mockk/common/src/main/kotlin/io/mockk/impl/stub/ConstructorStub.kt +++ b/mockk/common/src/main/kotlin/io/mockk/impl/stub/ConstructorStub.kt @@ -77,6 +77,8 @@ class ConstructorStub( it.substitute(revertRepresentation) } + override fun matcherUsages() = stub.matcherUsages() + override fun clear(options: MockKGateway.ClearOptions) = stub.clear(options) @@ -97,4 +99,4 @@ class ConstructorStub( override fun toStr() = stub.toStr() override fun stdObjectAnswer(invocation: Invocation) = stub.stdObjectAnswer(invocation.substitute(represent)) override fun dispose() = stub.dispose() -} \ No newline at end of file +} diff --git a/mockk/common/src/main/kotlin/io/mockk/impl/stub/MockKStub.kt b/mockk/common/src/main/kotlin/io/mockk/impl/stub/MockKStub.kt index e25022e1a..c008dce56 100644 --- a/mockk/common/src/main/kotlin/io/mockk/impl/stub/MockKStub.kt +++ b/mockk/common/src/main/kotlin/io/mockk/impl/stub/MockKStub.kt @@ -18,6 +18,7 @@ open class MockKStub( val log = gatewayAccess.safeToString(Logger()) private val answers = InternalPlatform.synchronizedMutableList() + private val answerUsages = InternalPlatform.synchronizedMutableMap() private val childs = InternalPlatform.synchronizedMutableMap() private val recordedCalls = InternalPlatform.synchronizedMutableList() private val recordedCallsByMethod = @@ -32,6 +33,7 @@ open class MockKStub( override fun addAnswer(matcher: InvocationMatcher, answer: Answer<*>) { val invocationAnswer = InvocationAnswer(matcher, answer) answers.add(invocationAnswer) + answerUsages[invocationAnswer] = 0 } override fun answer(invocation: Invocation): Any? { @@ -39,6 +41,7 @@ open class MockKStub( answers .reversed() .firstOrNull { it.matcher.match(invocation) } + ?.also { answerUsages[it] = answerUsages[it]!! + 1 } } ?: return defaultAnswer(invocation) return with(invocationAndMatcher) { @@ -187,6 +190,11 @@ open class MockKStub( } } + override fun matcherUsages(): Map = + InternalPlatform.synchronized(answers) { + answerUsages.mapKeys { it.key.matcher } + } + override fun toStr() = "${type.simpleName}($name)" override fun childMockK(matcher: InvocationMatcher, childType: KClass<*>): Any { @@ -269,6 +277,7 @@ open class MockKStub( override fun clear(options: MockKGateway.ClearOptions) { if (options.answers) { this.answers.clear() + this.answerUsages.clear() } if (options.recordedCalls) { this.recordedCalls.clear() diff --git a/mockk/common/src/main/kotlin/io/mockk/impl/stub/Stub.kt b/mockk/common/src/main/kotlin/io/mockk/impl/stub/Stub.kt index f0fc29574..46572434d 100644 --- a/mockk/common/src/main/kotlin/io/mockk/impl/stub/Stub.kt +++ b/mockk/common/src/main/kotlin/io/mockk/impl/stub/Stub.kt @@ -28,6 +28,8 @@ interface Stub : Disposable { fun verifiedCalls(): List + fun matcherUsages(): Map + fun clear(options: MockKGateway.ClearOptions) fun handleInvocation( diff --git a/mockk/jvm/api/mockk-jvm.api b/mockk/jvm/api/mockk-jvm.api index 71814f39d..9ed1d55aa 100644 --- a/mockk/jvm/api/mockk-jvm.api +++ b/mockk/jvm/api/mockk-jvm.api @@ -1133,6 +1133,9 @@ public final class io/mockk/junit5/MockKExtension$Companion { public abstract interface annotation class io/mockk/junit5/MockKExtension$ConfirmVerification : java/lang/annotation/Annotation { } +public abstract interface annotation class io/mockk/junit5/MockKExtension$CheckUnnecessaryStub : java/lang/annotation/Annotation { +} + public abstract interface annotation class io/mockk/junit5/MockKExtension$KeepMocks : java/lang/annotation/Annotation { } diff --git a/mockk/jvm/src/main/kotlin/io/mockk/junit5/MockKExtension.kt b/mockk/jvm/src/main/kotlin/io/mockk/junit5/MockKExtension.kt index 175e8334f..41597dcd0 100755 --- a/mockk/jvm/src/main/kotlin/io/mockk/junit5/MockKExtension.kt +++ b/mockk/jvm/src/main/kotlin/io/mockk/junit5/MockKExtension.kt @@ -1,13 +1,10 @@ package io.mockk.junit5 -import io.mockk.MockKAnnotations -import io.mockk.confirmVerified +import io.mockk.* import io.mockk.impl.annotations.AdditionalInterface import io.mockk.impl.annotations.MockK import io.mockk.impl.annotations.RelaxedMockK import io.mockk.impl.annotations.SpyK -import io.mockk.mockkClass -import io.mockk.unmockkAll import org.junit.jupiter.api.extension.* import java.lang.annotation.Inherited import java.lang.reflect.AnnotatedElement @@ -93,6 +90,10 @@ class MockKExtension : TestInstancePostProcessor, ParameterResolver, AfterAllCal if (context.confirmVerification) { confirmVerified() } + + if (context.checkUnnecessaryStub) { + checkUnnecessaryStub() + } } private val ExtensionContext.keepMocks: Boolean @@ -111,6 +112,14 @@ class MockKExtension : TestInstancePostProcessor, ParameterResolver, AfterAllCal get() = map { it.getAnnotation(ConfirmVerification::class.java) != null} .orElse(false) + private val ExtensionContext.checkUnnecessaryStub: Boolean + get() = testClass.checkUnnecessaryStub || + getConfigurationParameter(CHECK_UNNECESSARY_STUB_PROPERTY).map { it.toBoolean() }.orElse(false) + + private val Optional.checkUnnecessaryStub + get() = map { it.getAnnotation(CheckUnnecessaryStub::class.java) != null} + .orElse(false) + /*** * Prevent calling [unmockkAll] after each test execution */ @@ -124,9 +133,15 @@ class MockKExtension : TestInstancePostProcessor, ParameterResolver, AfterAllCal @Target(AnnotationTarget.CLASS) annotation class ConfirmVerification + @Inherited + @Retention(AnnotationRetention.RUNTIME) + @Target(AnnotationTarget.CLASS) + annotation class CheckUnnecessaryStub + companion object { const val KEEP_MOCKS_PROPERTY = "mockk.junit.extension.keepmocks" const val CONFIRM_VERIFICATION_PROPERTY = "mockk.junit.extension.confirmverification" + const val CHECK_UNNECESSARY_STUB_PROPERTY = "mockk.junit.extension.checkUnnecessaryStub" } } diff --git a/mockk/jvm/src/test/kotlin/io/mockk/it/VerificationAcknowledgeTest.kt b/mockk/jvm/src/test/kotlin/io/mockk/it/VerificationAcknowledgeTest.kt index 88202f7d3..c67636ab5 100644 --- a/mockk/jvm/src/test/kotlin/io/mockk/it/VerificationAcknowledgeTest.kt +++ b/mockk/jvm/src/test/kotlin/io/mockk/it/VerificationAcknowledgeTest.kt @@ -1,6 +1,8 @@ package io.mockk.it import io.mockk.* +import org.hamcrest.CoreMatchers.equalTo +import org.hamcrest.MatcherAssert.assertThat import kotlin.test.Test import kotlin.test.assertEquals import kotlin.test.assertFails @@ -38,6 +40,67 @@ class VerificationAcknowledgeTest { assertEquals(3, mock.op(1)) } + @Test + fun `checkUnnecessaryStub with single call to each stub`() { + doCalls1() + checkUnnecessaryStub(mock) + } + + @Test + fun `checkUnnecessaryStub with multiple calls to some stub`() { + doCalls2() + checkUnnecessaryStub(mock) + } + + @Test + fun `checkUnnecessaryStub with some used and some unused stubs`() { + every { mock.op(98) } returns 1 + every { mock.op(99) } returns 2 + doCalls1() + assertFailsWith { + checkUnnecessaryStub(mock) + } + .also { + assertThat( + it.message, equalTo( + "Unnecessary stubbings detected.\n" + + "Following stubbings are not used, either because there are unnecessary or because tested code doesn't call them :\n" + + "\n" + + "1) ${mock}.op(eq(98)))\n" + + "2) ${mock}.op(eq(99)))" + ) + ) + } + } + + @Test + fun `checkUnnecessaryStubAll with single call to each stub`() { + doCalls1() + val mock2 = mockk() + every { mock2.op(98) } returns 1 + mock2.op(98) + checkUnnecessaryStub() + } + + @Test + fun `checkUnnecessaryStubAll with some used and some unused stubs`() { + doCalls1() + val mock2 = mockk { every { op(42) } returns 1 } + assertFailsWith { + checkUnnecessaryStub() + } + .also { + assertThat( + it.message, equalTo( + "Unnecessary stubbings detected.\n" + + "Following stubbings are not used, either because there are unnecessary or because tested code doesn't call them :\n" + + "\n" + + "1) ${mock2}.op(eq(42)))" + ) + ) + } + } + @Test fun checkVerify() { doCalls1() From 3de3ad7f72a93f4245779703c30302e921c43c99 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9bastien?= Date: Fri, 20 May 2022 08:14:14 +0200 Subject: [PATCH 2/2] fixes issue with InvocationAnswer.hashCode() --- .../main/kotlin/io/mockk/impl/stub/MockKStub.kt | 14 +++++--------- .../io/mockk/it/VerificationAcknowledgeTest.kt | 2 ++ 2 files changed, 7 insertions(+), 9 deletions(-) diff --git a/mockk/common/src/main/kotlin/io/mockk/impl/stub/MockKStub.kt b/mockk/common/src/main/kotlin/io/mockk/impl/stub/MockKStub.kt index c008dce56..cfea60af0 100644 --- a/mockk/common/src/main/kotlin/io/mockk/impl/stub/MockKStub.kt +++ b/mockk/common/src/main/kotlin/io/mockk/impl/stub/MockKStub.kt @@ -18,7 +18,6 @@ open class MockKStub( val log = gatewayAccess.safeToString(Logger()) private val answers = InternalPlatform.synchronizedMutableList() - private val answerUsages = InternalPlatform.synchronizedMutableMap() private val childs = InternalPlatform.synchronizedMutableMap() private val recordedCalls = InternalPlatform.synchronizedMutableList() private val recordedCallsByMethod = @@ -31,17 +30,15 @@ open class MockKStub( var disposeRoutine: () -> Unit = {} override fun addAnswer(matcher: InvocationMatcher, answer: Answer<*>) { - val invocationAnswer = InvocationAnswer(matcher, answer) + val invocationAnswer = InvocationAnswer(matcher, answer, 0) answers.add(invocationAnswer) - answerUsages[invocationAnswer] = 0 } override fun answer(invocation: Invocation): Any? { val invocationAndMatcher = InternalPlatform.synchronized(answers) { answers - .reversed() - .firstOrNull { it.matcher.match(invocation) } - ?.also { answerUsages[it] = answerUsages[it]!! + 1 } + .findLast { it.matcher.match(invocation) } + ?.also { it.usageCount++ } } ?: return defaultAnswer(invocation) return with(invocationAndMatcher) { @@ -192,7 +189,7 @@ open class MockKStub( override fun matcherUsages(): Map = InternalPlatform.synchronized(answers) { - answerUsages.mapKeys { it.key.matcher } + answers.associate { it.matcher to it.usageCount } } override fun toStr() = "${type.simpleName}($name)" @@ -277,7 +274,6 @@ open class MockKStub( override fun clear(options: MockKGateway.ClearOptions) { if (options.answers) { this.answers.clear() - this.answerUsages.clear() } if (options.recordedCalls) { this.recordedCalls.clear() @@ -298,7 +294,7 @@ open class MockKStub( val childOfRegex = Regex("child(\\^(\\d+))? of (.+)") } - private data class InvocationAnswer(val matcher: InvocationMatcher, val answer: Answer<*>) + private data class InvocationAnswer(val matcher: InvocationMatcher, val answer: Answer<*>, var usageCount: Int) protected fun Invocation.allEqMatcher() = InvocationMatcher( diff --git a/mockk/jvm/src/test/kotlin/io/mockk/it/VerificationAcknowledgeTest.kt b/mockk/jvm/src/test/kotlin/io/mockk/it/VerificationAcknowledgeTest.kt index c67636ab5..4a5fee58d 100644 --- a/mockk/jvm/src/test/kotlin/io/mockk/it/VerificationAcknowledgeTest.kt +++ b/mockk/jvm/src/test/kotlin/io/mockk/it/VerificationAcknowledgeTest.kt @@ -75,6 +75,7 @@ class VerificationAcknowledgeTest { @Test fun `checkUnnecessaryStubAll with single call to each stub`() { + clearAllMocks() // ensure that previous tests haven't created unnecessary stubs doCalls1() val mock2 = mockk() every { mock2.op(98) } returns 1 @@ -84,6 +85,7 @@ class VerificationAcknowledgeTest { @Test fun `checkUnnecessaryStubAll with some used and some unused stubs`() { + clearAllMocks() // ensure that previous tests haven't created unnecessary stubs doCalls1() val mock2 = mockk { every { op(42) } returns 1 } assertFailsWith {