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 #2626 : Introduce MockSettings.mockMaker #2701

Merged
merged 1 commit into from Sep 7, 2022

Conversation

JojOatXGME
Copy link
Contributor

@JojOatXGME JojOatXGME commented Jul 4, 2022

I implemented a prototype to fix #2626 as suggested in #2589 (comment). The basics doesn't seem that difficult, but I could have missed some edge cases. I mostly skipped documentation and tests for now. Furthermore, I ignored that ByteBuddyMockMaker and InlineByteBuddyMockMaker are not exposed by the API, so we still have to adjust the API to allow setting the mock maker without accessing these classes.

Personally, I don't need this functionality right now. However, if you think it would be a worthwhile addition, I can still try to help with that topic.

This pull request allows users of Mockito to choose different MockMakers per mock. This feature could ease the migration from one MockMaker to another in a big project.

SomeClass subclassMock = Mockito.mock(SomeClass.class, Mockito.withSettings()
    .mockMaker(AwesomeMockMaker.class));

PS: The Gradle task spotlessApply failed on my system with “java.lang.NoClassDefFoundError: Could not initialize class com.google.googlejavaformat.java.java14.Java14InputAstVisitor”. Probably because it couldn't find BindingPatternTree. Not sure if it is a local problem or something with the project configuration.

Checklist

  • Read the contributing guide
  • PR should be motivated, i.e. what does it fix, why, and if relevant how
  • If possible / relevant include an example in the description, that could help all readers
    including project members to get a better picture of the change
  • Avoid other runtime dependencies
  • Meaningful commit history ; intention is important please rebase your commit history so that each
    commit is meaningful and help the people that will explore a change in 2 years
  • The pull request follows coding style
  • Mention Fixes #<issue number> in the description if relevant
  • At least one commit should mention Fixes #<issue number> if relevant

@JojOatXGME
Copy link
Contributor Author

JojOatXGME commented Jul 4, 2022

I was able to run spotlessApply from the terminal (PowerShell on Windows). I previously tried to run the task from IntelliJ which for some reason was using a development build of JDK 14 (with Project Panama). 😅

@codecov-commenter
Copy link

codecov-commenter commented Jul 4, 2022

Codecov Report

Merging #2701 (181d961) into main (0753d48) will increase coverage by 0.13%.
The diff coverage is 92.50%.

@@             Coverage Diff              @@
##               main    #2701      +/-   ##
============================================
+ Coverage     86.04%   86.18%   +0.13%     
- Complexity     2815     2828      +13     
============================================
  Files           320      320              
  Lines          8529     8583      +54     
  Branches       1053     1060       +7     
============================================
+ Hits           7339     7397      +58     
+ Misses          913      905       -8     
- Partials        277      281       +4     
Impacted Files Coverage Δ
src/main/java/org/mockito/Mock.java 100.00% <ø> (ø)
src/main/java/org/mockito/Mockito.java 96.10% <ø> (ø)
...to/internal/configuration/SpyAnnotationEngine.java 98.41% <ø> (ø)
...guration/injection/SpyOnInjectedFieldsHandler.java 84.21% <ø> (ø)
src/main/java/org/mockito/plugins/MockMaker.java 87.50% <ø> (ø)
...ernal/configuration/plugins/PluginInitializer.java 66.66% <66.66%> (-0.78%) ⬇️
.../main/java/org/mockito/internal/util/MockUtil.java 91.66% <87.17%> (-6.49%) ⬇️
...rc/main/java/org/mockito/internal/MockitoCore.java 94.26% <100.00%> (+0.07%) ⬆️
...nternal/configuration/MockAnnotationProcessor.java 97.29% <100.00%> (+0.15%) ⬆️
...l/configuration/plugins/DefaultMockitoPlugins.java 91.66% <100.00%> (+1.34%) ⬆️
... and 13 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Contributor

@TimvdLippe TimvdLippe left a comment

Choose a reason for hiding this comment

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

Nice work! Definitely getting there, but left some initial review comments. Looking forward to this feature 😄

Copy link
Contributor Author

@JojOatXGME JojOatXGME left a comment

Choose a reason for hiding this comment

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

Here are my changes. I hope I am able to respond faster the next time. We also have to think about the API. Should the user be able to provide arbitrary mock makers (as right now in the PR), or should we make it more specific to the upcoming migration.

Generic

withSettings().mockMaker(SomeMockMaker.class)

In this case, we need to expose the internal mock makers somehow, or provide separate methods in the settings interface.

Specific

withSettings().useSubclassMockMaker()

Keeping the interface more specific to the upcoming migration would make it easier to deprecate and remove the feature in the future if it causes maintenance problems for some reason.

Copy link
Contributor

@TimvdLippe TimvdLippe left a comment

Choose a reason for hiding this comment

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

Nice, yes this is looking a lot better. Very happy about this design choice, this looks natural for loading custom mockmakers as well. I like your solution of MockMakers.java, since these strings were kinda magic at first and now we have a proper dynamic API for them as well.

Some last minor changes, but the overall design LGTM now.

We should also include a nice mention in our main Javadoc. Can you add section 53 to Mockito.java? Something along the lines of:

 * <h3 id="53">53. <a class="meaningful_link" href="#mock_annotation_mockmaker" name="mock_annotation_mockmaker">
 *  New mockmaker attribute for @Mock annotation and <code>MockSettings.mockMaker(String)</code> methods (Since 4.6.0)</a></h3>
 *
 * You can now customize the mockmaker for a single mock, either using `@Mock` annotation mockmaker attribute or
 * using `MockSettings.mockMaker(String)`. This can be useful for migrating to a different mockmakers
 * (which we are planning to do in Mockito 5 with https://github.com/mockito/mockito/issues/2589).
 *
 * <pre class="code"><code class="java">
 *   &#064;Mock(mockMaker = MockMakers.INLINE)
 *   Foo mock;
 *   // using MockSettings.withSettings()
 *   Foo mock = Mockito.mock(Foo.class, withSettings().mockMaker(MockMakers.SUBCLASS));
 * </code></pre>

Also, make sure you add a link to section 53 at the top of the Javadoc section.

Copy link
Contributor Author

@JojOatXGME JojOatXGME left a comment

Choose a reason for hiding this comment

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

I made some adjustments. I want to update the Javadoc later this week. There are also a few remaining TODOs I would like to discuss before merging this pull request.

@JojOatXGME
Copy link
Contributor Author

JojOatXGME commented Sep 3, 2022

@TimvdLippe, I have finished my work. If you have no further suggestions, I would replace my ${NEXT_VERSION} placeholder with 4.8.0 (or should it be 4.7.1?) and rebase/squash everything into one commit. Or what is the process?

FYI: I noticed that the following test is flaky, even on the main branch.

@Test
public void
given_mock_with_returns_deep_stubs__when_called_concurrently__then_does_not_throw_concurrent_modification_exception() {
for (int i = 0; i < 1000; i++) {
Service mock = mock(Service.class, Answers.RETURNS_DEEP_STUBS);
IntStream.range(1, 100).parallel().forEach(index -> mock.doSomething());
}
}

It sometimes throws an ArrayIndexOutOfBoundsException in InvocationContainerImpl.getStubbingsAscending. The exception seems to be caused by a race condition. While the thread which throws the exception is reading InvocationContainerImpl.stubbed, another thread seems to add or remove items from the list.

@TimvdLippe
Copy link
Contributor

@JojOatXGME Great work! The next version will be 4.8.0, so let's use that in the documentation 😄

Regarding the flaky test: if you have time to fix it, please go ahead. But for now, since we retry our tests, I think we can keep it as-is for now. Not ideal, I know...

@JojOatXGME
Copy link
Contributor Author

@TimvdLippe I replaced my version placeholder and rebased and squashed my commits. I completely forgot your suggestion while writing the Javadoc, so if you have any final remarks or prefer another wording, I am happy to make adjustments. Otherwise, I think there is nothing left to be done for this PR.

Copy link
Contributor

@TimvdLippe TimvdLippe left a comment

Choose a reason for hiding this comment

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

Woohoo! Great feature 😄

Running the final checks on CI and then I will merge.

import org.mockito.plugins.MockMaker;
import org.mockitoutil.ClassLoaders;

public class DeferMockMakersClassLoadingTest {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice test!

@TimvdLippe TimvdLippe merged commit 3e910ea into mockito:main Sep 7, 2022
@TimvdLippe
Copy link
Contributor

Released as part of 4.8.0 🎉

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.

Introduce option to disable inline-mock-maker for a specific instance
3 participants