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
New API to clean up all inline mocks after test #1619
Changes from all commits
91c8e9c
808bae8
e44d76e
a6027ac
c3c0d0a
ebb9b85
cc38b6c
d61a8be
1641868
959cba4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -92,4 +92,55 @@ public interface MockitoFramework { | |
*/ | ||
@Incubating | ||
InvocationFactory getInvocationFactory(); | ||
|
||
/** | ||
* Clears up internal state of all inline mocks. | ||
* This method is only meaningful if inline mock maker is in use. | ||
* Otherwise this method is a no-op and need not be used. | ||
* <p> | ||
* This method is useful to tackle subtle memory leaks that are possible due to the nature of inline mocking | ||
* (issue <a href="https://github.com/mockito/mockito/pull/1619">#1619</a>). | ||
* If you are facing those problems, call this method at the end of the test (or in "@After" method). | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't we update our JUnit runner and extension to handle this automatically? I think this is an improvement in any situation, so we can just go ahead and do it eagerly. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This would be a breaking change in thread-based parallel tests that are supported by Mockito (with limitations, as we stated in the FAQ). If you turn this on by default, some Mockito tests will start failing. @ttanxu, discussed this somewhere in this PR (but the thread became long :). We can always make it later if we understand the use patterns better. I suggest we expose the this API first and observe / learn how it goes. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah okay, I missed that discussion. Thanks for the clarification. |
||
* See examples of using "clearInlineMocks" in Mockito test code. | ||
* To find out why inline mock maker keeps track of the mock objects see {@link org.mockito.plugins.InlineMockMaker}. | ||
* <p> | ||
* Mockito's "inline mocking" enables mocking final types, enums and final methods | ||
* (read more in section 39 of {@link Mockito} javadoc). | ||
* This method is only meaningful when {@link org.mockito.plugins.InlineMockMaker} is in use. | ||
* If you're using a different {@link org.mockito.plugins.MockMaker} then this method is a no-op. | ||
* | ||
* <pre class="code"><code class="java"> | ||
* public class ExampleTest { | ||
* | ||
* @After | ||
* public void clearMocks() { | ||
* Mockito.framework().clearInlineMocks(); | ||
* } | ||
* | ||
* @Test | ||
* public void someTest() { | ||
* ... | ||
* } | ||
* } | ||
* </pre> | ||
* | ||
* If you have feedback or a better idea how to solve the problem please reach out. | ||
* | ||
* @since 2.25.0 | ||
* @see #clearInlineMock(Object) | ||
*/ | ||
@Incubating | ||
void clearInlineMocks(); | ||
|
||
/** | ||
* Clears up internal state of specific inline mock. | ||
* This method is a single-mock variant of {@link #clearInlineMocks()}. | ||
* Please read javadoc for {@link #clearInlineMocks()}. | ||
* | ||
* @param mock to clear up | ||
* @since 2.25.0 | ||
* @see #clearInlineMocks() | ||
*/ | ||
@Incubating | ||
void clearInlineMock(Object mock); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,52 @@ | ||
/* | ||
* Copyright (c) 2019 Mockito contributors | ||
* This program is made available under the terms of the MIT License. | ||
*/ | ||
|
||
package org.mockito.plugins; | ||
|
||
import org.mockito.Incubating; | ||
import org.mockito.MockitoFramework; | ||
|
||
/** | ||
* Extension to {@link MockMaker} for mock makers that changes inline method implementations | ||
* and need keep track of created mock objects. | ||
* <p> | ||
* Mockito's default inline mock maker keeps track of created mock objects via weak reference map. | ||
* This poses a risk of memory leaks in certain scenarios | ||
* (issue <a href="https://github.com/mockito/mockito/pull/1619">#1619</a>). | ||
* There is no clean way to tackle those problems at the moment. | ||
* Hence, {@code InlineMockMaker} interface exposes methods to explicitly clear mock references. | ||
* Those methods are called by {@link MockitoFramework#clearInlineMocks()}. | ||
* When the user encounters a leak, he can mitigate the problem with {@link MockitoFramework#clearInlineMocks()}. | ||
* <p> | ||
* {@code InlineMockMaker} is for expert users and framework integrators, when custom inline mock maker is in use. | ||
* If you have a custom {@link MockMaker} that keeps track of mock objects, | ||
* please have your mock maker implement {@code InlineMockMaker} interface. | ||
* This way, it can participate in {@link MockitoFramework#clearInlineMocks()} API. | ||
* | ||
* @since 2.25.0 | ||
*/ | ||
@Incubating | ||
public interface InlineMockMaker extends MockMaker { | ||
|
||
/** | ||
* Clean up internal state for specified {@code mock}. You may assume there won't be any interaction to the specific | ||
* mock after this is called. | ||
* | ||
* @param mock the mock instance whose internal state is to be cleaned. | ||
* @since 2.25.0 | ||
*/ | ||
@Incubating | ||
void clearMock(Object mock); | ||
|
||
/** | ||
* Cleans up internal state for all existing mocks. You may assume there won't be any interaction to mocks created | ||
* previously after this is called. | ||
* | ||
* @since 2.25.0 | ||
*/ | ||
@Incubating | ||
void clearAllMocks(); | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,15 +10,26 @@ | |
import org.mockito.MockSettings; | ||
import org.mockito.StateMaster; | ||
import org.mockito.exceptions.misusing.RedundantListenerException; | ||
import org.mockito.internal.configuration.plugins.Plugins; | ||
import org.mockito.listeners.MockCreationListener; | ||
import org.mockito.listeners.MockitoListener; | ||
import org.mockito.mock.MockCreationSettings; | ||
import org.mockito.plugins.InlineMockMaker; | ||
import org.mockitoutil.TestBase; | ||
|
||
import java.util.List; | ||
import java.util.Set; | ||
|
||
import static org.mockito.Mockito.*; | ||
import static org.junit.Assert.assertFalse; | ||
import static org.junit.Assert.assertTrue; | ||
import static org.junit.Assume.assumeTrue; | ||
import static org.mockito.Mockito.any; | ||
import static org.mockito.Mockito.eq; | ||
import static org.mockito.Mockito.mock; | ||
import static org.mockito.Mockito.mockingDetails; | ||
import static org.mockito.Mockito.verify; | ||
import static org.mockito.Mockito.verifyNoMoreInteractions; | ||
import static org.mockito.Mockito.withSettings; | ||
import static org.mockitoutil.ThrowableAssert.assertThat; | ||
|
||
public class DefaultMockitoFrameworkTest extends TestBase { | ||
|
@@ -112,5 +123,52 @@ public void run() { | |
"For more information, see the javadoc for RedundantListenerException class."); | ||
} | ||
|
||
@Test | ||
public void clearing_all_mocks_is_safe_regardless_of_mock_maker_type() { | ||
List mock = mock(List.class); | ||
|
||
//expect | ||
assertTrue(mockingDetails(mock).isMock()); | ||
framework.clearInlineMocks(); | ||
} | ||
|
||
@Test | ||
public void clears_all_mocks() { | ||
//clearing mocks only works with inline mocking | ||
assumeTrue(Plugins.getMockMaker() instanceof InlineMockMaker); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FWIW, one of the reason why I didn't use assume at first is that it can test user can safely call clearInlineMocks() and clearMock() even when mock maker is not a inline mock maker. That's not very important I suppose, but still would be good to cover that in tests imo. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 I'm fixing it now. |
||
|
||
//given | ||
List list1 = mock(List.class); | ||
assertTrue(mockingDetails(list1).isMock()); | ||
List list2 = mock(List.class); | ||
assertTrue(mockingDetails(list2).isMock()); | ||
|
||
//when | ||
framework.clearInlineMocks(); | ||
|
||
//then | ||
assertFalse(mockingDetails(list1).isMock()); | ||
assertFalse(mockingDetails(list2).isMock()); | ||
} | ||
|
||
@Test | ||
public void clears_mock() { | ||
//clearing mocks only works with inline mocking | ||
assumeTrue(Plugins.getMockMaker() instanceof InlineMockMaker); | ||
|
||
//given | ||
List list1 = mock(List.class); | ||
assertTrue(mockingDetails(list1).isMock()); | ||
List list2 = mock(List.class); | ||
assertTrue(mockingDetails(list2).isMock()); | ||
|
||
//when | ||
framework.clearInlineMock(list1); | ||
|
||
//then | ||
assertFalse(mockingDetails(list1).isMock()); | ||
assertTrue(mockingDetails(list2).isMock()); | ||
} | ||
|
||
private static class MyListener implements MockitoListener {} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
/* | ||
* Copyright (c) 2019 Mockito contributors | ||
* This program is made available under the terms of the MIT License. | ||
*/ | ||
|
||
package org.mockitoinline.bugs; | ||
|
||
import org.junit.Test; | ||
|
||
import static org.mockito.Mockito.framework; | ||
import static org.mockito.Mockito.mock; | ||
|
||
public class CyclicMockMethodArgumentMemoryLeakTest { | ||
private static final int ARRAY_LENGTH = 1 << 20; // 4 MB | ||
|
||
@Test | ||
public void no_memory_leak_when_cyclically_calling_method_with_mocks() { | ||
for (int i = 0; i < 100; ++i) { | ||
final A a = mock(A.class); | ||
a.largeArray = new int[ARRAY_LENGTH]; | ||
final B b = mock(B.class); | ||
|
||
a.accept(b); | ||
b.accept(a); | ||
|
||
framework().clearInlineMocks(); | ||
} | ||
} | ||
|
||
private static class A { | ||
private int[] largeArray; | ||
|
||
void accept(B b) {} | ||
} | ||
|
||
private static class B { | ||
void accept(A a) {} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm skipping this suggestion if that's OK. I prefer skipping those details in the high level javadoc. Also, I prefer the original text grammatically.
Feel free to redo the javadoc if you want, in a separate PR - I'm happy to review/merge it.