Skip to content
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

Fixes #2648 : Add support for customising strictness via @Mock annotation and MockSettings #2650

Merged
merged 5 commits into from May 26, 2022

Conversation

sivaprasadreddy
Copy link
Contributor

@sivaprasadreddy sivaprasadreddy commented May 23, 2022

This PR fixes the issue #2648.

This is to add support for customising strictness level for mock using @mock annotation or MockSettings.

Example 1:

class MyTest {
     @Mock(strictness = Strictness.LENIENT)
     Foo foo;
     ....
}

Example 2:

class MyTest {
     Foo mock = Mockito.mock(Foo.class, withSettings().strictness(Strictness.WARN));
     ....
}

@codecov-commenter
Copy link

codecov-commenter commented May 26, 2022

Codecov Report

Merging #2650 (ff8a4ee) into main (d7a8ae0) will decrease coverage by 0.03%.
The diff coverage is 75.00%.

Current head ff8a4ee differs from pull request most recent head d090780. Consider uploading reports for the commit d090780 to get more accurate results

@@             Coverage Diff              @@
##               main    #2650      +/-   ##
============================================
- Coverage     86.24%   86.21%   -0.04%     
- Complexity     2761     2762       +1     
============================================
  Files           314      314              
  Lines          8282     8290       +8     
  Branches       1031     1031              
============================================
+ Hits           7143     7147       +4     
- Misses          872      874       +2     
- Partials        267      269       +2     
Impacted Files Coverage Δ
src/main/java/org/mockito/Mockito.java 96.10% <ø> (ø)
...java/org/mockito/internal/exceptions/Reporter.java 90.47% <0.00%> (-0.29%) ⬇️
...rg/mockito/internal/creation/MockSettingsImpl.java 86.13% <60.00%> (-1.50%) ⬇️
...nternal/configuration/MockAnnotationProcessor.java 97.05% <100.00%> (+0.08%) ⬆️
...o/internal/creation/settings/CreationSettings.java 100.00% <100.00%> (ø)
...ito/internal/stubbing/InvocationContainerImpl.java 94.20% <100.00%> (ø)
.../mockito/internal/stubbing/StrictnessSelector.java 80.00% <0.00%> (-20.00%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d7a8ae0...d090780. Read the comment docs.

Copy link
Contributor

@TimvdLippe TimvdLippe left a comment

Nice work! It's already looking good, mostly some nits. After that, this is ready to merge 🎉

* Mock will have custom strictness, see {@link MockSettings#strictness(Strictness)}.
* For examples how to use 'Mock' annotation and parameters see {@link Mock}.
*
* @since 4.5.2
Copy link
Contributor

@TimvdLippe TimvdLippe May 26, 2022

Choose a reason for hiding this comment

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

Nit: @since 4.6.0 since we will include this in the next minor release

Copy link
Contributor Author

@sivaprasadreddy sivaprasadreddy May 26, 2022

Choose a reason for hiding this comment

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

Updated.


/**
* Specifies strictness level for the mock.
* The default strictness level is STRICT_STUBS
Copy link
Contributor

@TimvdLippe TimvdLippe May 26, 2022

Choose a reason for hiding this comment

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

Nit: the default is the default of the runner. If you use the lenient runner, it will be lenient. I suggest to formulate it the following way:

The default strictness level is determined by the rule/runner used. If you are using no rule/runner, the default strictness level is LENIENT.

Copy link
Contributor Author

@sivaprasadreddy sivaprasadreddy May 26, 2022

Choose a reason for hiding this comment

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

Updated.

* Sets strictness level for the mock, e.g. having {@link Strictness#STRICT_STUBS} characteristic.
* For more information about using mocks with custom strictness, see {@link MockSettings#strictness(Strictness)}.
*
* @since 4.5.2
Copy link
Contributor

@TimvdLippe TimvdLippe May 26, 2022

Choose a reason for hiding this comment

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

Nit: 4.6.0

Copy link
Contributor Author

@sivaprasadreddy sivaprasadreddy May 26, 2022

Choose a reason for hiding this comment

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

Updated.

src/main/java/org/mockito/Mock.java Show resolved Hide resolved
* <h3 id="52">52. <a class="meaningful_link" href="#mockito_strictness" name="mockito_strictness">
* New strictness attribute for @Mock annotation and <code>MockSettings.strictness()</code> methods (Since 4.5.2)</a></h3>
*
* By default Mocks use Strict stubbing and now you can customize the strictness level
Copy link
Contributor

@TimvdLippe TimvdLippe May 26, 2022

Choose a reason for hiding this comment

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

Nit: mocks by default are lenient. I would omit that part altogether and reword it to:

You can now customize the strictness level for a single mock, either using `@Mock` annotation strictness attribute or using `MockSettings.strictness()`. This can be useful if you want all of your mocks to be strict, but one of the mocks to be lenient.

Copy link
Contributor Author

@sivaprasadreddy sivaprasadreddy May 26, 2022

Choose a reason for hiding this comment

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

Updated.


// but regular mock throws:
Assertions.assertThatThrownBy(
new ThrowableAssert.ThrowingCallable() {
Copy link
Contributor

@TimvdLippe TimvdLippe May 26, 2022

Choose a reason for hiding this comment

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

Nit: I think we can use a lambda here, can we not?

() -> ProductionCode.simpleMethod(regularMock, "4")

Copy link
Contributor Author

@sivaprasadreddy sivaprasadreddy May 26, 2022

Choose a reason for hiding this comment

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

Updated.

when(regularMock.simpleMethod("3")).thenReturn("3");
when(strictMock.simpleMethod("2")).thenReturn("2");

// then lenient mock does not throw:
Copy link
Contributor

@TimvdLippe TimvdLippe May 26, 2022

Choose a reason for hiding this comment

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

Same here, let's have 3 separate 3 test cases here.

Copy link
Contributor Author

@sivaprasadreddy sivaprasadreddy May 26, 2022

Choose a reason for hiding this comment

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

Separated this into multiple tests.

Copy link
Contributor

@TimvdLippe TimvdLippe left a comment

Thanks for the PR and great implementation 🎉

@TimvdLippe TimvdLippe merged commit 70cf2d2 into mockito:main May 26, 2022
7 checks passed
}

@Test
public void mock_is_strict_with_default_settings() {
Copy link

@chadlwilson chadlwilson May 30, 2022

Choose a reason for hiding this comment

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

Quick Q - was it intentional for this to change mocks defined by mock(Blah.class) i.e with no rule/Junit MockitoSettings to strict by default?

I believe the previous behaviour was lenient by default?

Copy link
Contributor

@TimvdLippe TimvdLippe May 30, 2022

Choose a reason for hiding this comment

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

We have not changed the behavior here. This test is using a Strict rule explicitly (https://github.com/mockito/mockito/pull/2650/files/d0907808e9fce24f6dcddc94d1ea2c3ebfa15676#diff-3040268f6b18ef562dd495ac7fc13465723311889d5f326bfb615cc3aed37415R21) to verify that we handle that case correctly.

In case you are hitting any regressions, please file a new issue. You are most likely running into a different problem.

Choose a reason for hiding this comment

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

OK, thanks. We definitely have regressions (previously lenient mocks now behaving as strict) but I may have misidentified the possible cause. Will need to dig further if I get time.

Choose a reason for hiding this comment

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

Another person has beat me to raising it at #2656 but this seems to be the same issue we are having.

ijuma pushed a commit to apache/kafka that referenced this issue Aug 9, 2022
## Changes
- **mockito: 4.4.0 -> 4.6.1** (https://github.com/mockito/mockito/releases)
Most important updates:
  - Fixes mockito/mockito#2648 : Add support for customising strictness via @mock annotation and MockSettings mockito/mockito#2650

## Why is this change needed?

According to the [Mockito documentation](https://javadoc.io/doc/org.mockito/mockito-core/latest/org/mockito/Mockito.html#when(T)) :
> Although it is possible to verify a stubbed invocation, usually it's just redundant. Let's say you've stubbed foo.bar(). If your code cares what foo.bar() returns then something else breaks(often before even verify() gets executed). If your code doesn't care what get(0) returns then it should not be stubbed. 

While working on the [Replace EasyMock and PowerMock with Mockito for StreamsMetricsImplTest ](https://issues.apache.org/jira/browse/KAFKA-12947) I noticed that described behavior wasn't applied when you create a new `mock` like this.

```java
        final Metrics metrics = mock(Metrics.class);
        when(metrics.metric(metricName)).thenReturn(null);

        ... invoke SUT

        verify(metrics).metric(metricName); // this should be redundant (according to docs)

```

After further investigation I figured out that described behaviour wasn't implemented until`v4.6.1`.

With this change we are now able to mock objects like this:

```java
   Foo explicitStrictMock = mock(Foo.class, withSettings().strictness(Strictness.STRICT_STUBS));
```
- link to docs: [MockSettings.html#strictness](https://javadoc.io/static/org.mockito/mockito-core/4.6.1/org/mockito/quality/Strictness.html#STRICT_STUBS)

It looks like I can accomplish the same thing by using the `@RunWith(MockitoJUnitRunner.StrictStubs.class)
` instead of the `@RunWith(MockitoJUnitRunner.class)` so mockito dependency version update is not mandatory, but it would be nice to stay up-to-date and use the latest version (it's up to MR reviewer to decide if we are going to merge this now, or just close the MR and update mockito version later).

Reviewers: Ismael Juma <ismael@juma.me.uk>
a0x8o added a commit to a0x8o/kafka that referenced this issue Aug 9, 2022
## Changes
- **mockito: 4.4.0 -> 4.6.1** (https://github.com/mockito/mockito/releases)
Most important updates:
  - Fixes mockito/mockito#2648 : Add support for customising strictness via @mock annotation and MockSettings mockito/mockito#2650

## Why is this change needed?

According to the [Mockito documentation](https://javadoc.io/doc/org.mockito/mockito-core/latest/org/mockito/Mockito.html#when(T)) :
> Although it is possible to verify a stubbed invocation, usually it's just redundant. Let's say you've stubbed foo.bar(). If your code cares what foo.bar() returns then something else breaks(often before even verify() gets executed). If your code doesn't care what get(0) returns then it should not be stubbed.

While working on the [Replace EasyMock and PowerMock with Mockito for StreamsMetricsImplTest ](https://issues.apache.org/jira/browse/KAFKA-12947) I noticed that described behavior wasn't applied when you create a new `mock` like this.

```java
        final Metrics metrics = mock(Metrics.class);
        when(metrics.metric(metricName)).thenReturn(null);

        ... invoke SUT

        verify(metrics).metric(metricName); // this should be redundant (according to docs)

```

After further investigation I figured out that described behaviour wasn't implemented until`v4.6.1`.

With this change we are now able to mock objects like this:

```java
   Foo explicitStrictMock = mock(Foo.class, withSettings().strictness(Strictness.STRICT_STUBS));
```
- link to docs: [MockSettings.html#strictness](https://javadoc.io/static/org.mockito/mockito-core/4.6.1/org/mockito/quality/Strictness.html#STRICT_STUBS)

It looks like I can accomplish the same thing by using the `@RunWith(MockitoJUnitRunner.StrictStubs.class)
` instead of the `@RunWith(MockitoJUnitRunner.class)` so mockito dependency version update is not mandatory, but it would be nice to stay up-to-date and use the latest version (it's up to MR reviewer to decide if we are going to merge this now, or just close the MR and update mockito version later).

Reviewers: Ismael Juma <ismael@juma.me.uk>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants