New API to clean up all inline mocks after test #1619

ttanxu · 2019-02-12T22:48:04Z

In certain specific, rare scenarios (issue #1614) inline mocking causes memory leaks. There is no clean way to mitigate this problem completely. Hence, we introduced a new API to explicitly clear mock state (only make sense in inline mocking!):

public class ExampleTest {

     @After
     public void clearMocks() {
         Mockito.framework().clearInlineMocks();
     }

     @Test
     public void someTest() {
         //...
     }
 }

Due to the introduction of map from weak reference from mock instance to
its invocation handler, Mockito became vulnerable to memory leaks as
there are multiple situations where Mockito could unintentionally hold
strong references to mock instances in the map record. The strong
references could be through spiedInstance for spies, and arguments used
to facilitate method stubbing.

Mockito could never know if the arguments passed in for method stubbing
are also strongly referenced somewhere else or not, so Mockito needs to
save a strong reference to these arguments to avoid premature GC.
Therefore to solve cyclic strong references through arguments Mockito
needs to explicitly know when mocks are out of their life, and clean
up all internal strong references associated with them.

Fixes #1532 and #1533.

TimvdLippe · 2019-02-12T22:57:32Z

I will review this tomorrow. At a first glance, we are going to need to make some changes. Most notably the way we handle interfaces. (A new method on MockMaker is a breaking change). Will give you a detailed review tomorrow.

TimvdLippe · 2019-02-12T23:02:39Z

(That said, the PR is much appreciated. Sorry if my initial reaction seemed not positive, we really do appreciate community PRs for these kind of issues!)

ttanxu · 2019-02-12T23:11:26Z

Oh... Thanks for quick response. I didn't expect any feedback today.

I am not familiar with how to contribute to Mockito (i.e. don't know what can be done, and what can't). It's expected to have some back and forth for a change at this scale. Just please be as detailed as possible, so that I know how to make proper changes.

I just hope the overall approach won't be vetoed, as I basically don't have any other ideas on how to solve it.

codecov-io · 2019-02-12T23:38:35Z

Codecov Report

Merging #1619 into release/2.x will increase coverage by 0.02%.
The diff coverage is 85.71%.

@@                Coverage Diff                @@
##             release/2.x    #1619      +/-   ##
=================================================
+ Coverage          87.38%   87.41%   +0.02%     
- Complexity          2435     2442       +7     
=================================================
  Files                301      301              
  Lines               6278     6285       +7     
  Branches             784      784              
=================================================
+ Hits                5486     5494       +8     
  Misses               596      596              
+ Partials             196      195       -1

Impacted Files	Coverage Δ	Complexity Δ
...l/creation/bytebuddy/InlineByteBuddyMockMaker.java	`68.42% <100%> (+3.58%)`	`14 <2> (+3)`	⬆️
...to/internal/framework/DefaultMockitoFramework.java	`89.47% <80%> (-10.53%)`	`10 <5> (+5)`
...ternal/util/reflection/GenericMetadataSupport.java	`86.86% <0%> (-0.27%)`	`38% <0%> (ø)`
...aultanswers/RetrieveGenericsForDefaultAnswers.java	`95.91% <0%> (-0.24%)`	`20% <0%> (-1%)`
src/main/java/org/mockito/Mockito.java	`96.61% <0%> (ø)`	`40% <0%> (ø)`	⬇️
...rnal/creation/bytebuddy/MockMethodInterceptor.java	`72.72% <0%> (ø)`	`5% <0%> (ø)`	⬇️
...rnal/stubbing/defaultanswers/ReturnsDeepStubs.java	`100% <0%> (+2.17%)`	`16% <0%> (ø)`	⬇️

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 af2d33b...959cba4. Read the comment docs.

mockitoguy · 2019-02-13T06:29:13Z

Thank you for the PR! Since this is changing / adding new public API I will review it, too (many thanks to @TimvdLippe for offering to review this!). I'll commit to provide review by the end of this week.

TimvdLippe

I will let @mockitoguy do the full review, as he has been working on the listeners. So far I have just 1 comment regarding the MockMaker, but that should be easy to fix hopefully.

src/main/java/org/mockito/plugins/MockMaker.java

ttanxu · 2019-02-13T17:28:47Z

Thanks to @mockitoguy .

Just some context here. I tried to avoid new API at first, but DefaultMockitoSession seems (at least from tests) to be used within a thread, and changing its default behavior to listen on events from all threads will break tests for multi-threading. Therefore I came up with the new API solution.

I didn't know how much impact there would be to add a new method in MockMaker interface. That requires everyone providing an implementation when they uprev their Mockito to a version that has the new method, but I have no clue how much work it could be. If it's too noisy maybe we can have a default impl (if only Java 8+ is supported) or a brand new interface called InlineMockMaker to add that method. I personally want to expose a chance to mock makers to clean up as we don't know how they handle the mapping from mock to its handler, which may have other places that could cause memory leaks other than those 2 places we found. We need better documentation on that, but I have to admit I'm not good at writing API docs so I'll defer to any of your suggestions in terms of documentation.

I hope the review won't later become a whole refactor of current implementation.

mockitoguy · 2019-02-18T18:55:10Z

I reviewed the implementation and most of it is really good. It is also a very high quality PR, thank you! Curious, are you facing this problem at G.?

I have a couple of concerns / design choices to discuss:

a) Can we make the fix more seamless? I'm not sure if this is possible but let's discuss. Goals: users don't have to remember to use trackAndCleanUpMocks() when inline mocker is in use. For example (brainstorming):
- we introduce a new interface that InlineByteBuddyMockMaker implements, say: RequiresCleaningMocks
- we expose a new method like framework.getPlugins().getPlugin(MockMacker.class), this way we can find out in runtime what MockMaker is in use.
- in Mockito Session, we check if the mock maker is RequiresCleaningMocks
- in session, we call RequiresCleaningMocks.cleanUpMock()

b) MockMaker.cleanUpMock() is an incompatible change so we cannot ship this PR in its current form. MockMaker is typically provided at runtime from a different Jar (example: dexmaker, powermockito). I have a couple ideas but first let's explore a)

Great tests, deep change in the internals of Mockito, thank you very much for the contribution!

ttanxu · 2019-02-19T02:39:05Z

Thanks for your review @mockitoguy .

Dexmaker is incubating a feature in inline-dexmaker to create spies without creating new instances, so that we can stub final members, and we (Android framework) found these mem leaks when using that. I then found #1533 and reproduced #1614 that I confirmed it also happens with ByteBuddy mock makers so I started the fix with help from @TimvdLippe .

First of all I really didn't see a solution w/o tying mock lives with something, and I think MockitoSession is the most natural choice for the "something". So technically it's already not transparent to properly use inline mock makers because sessions are necessary to make it work while it's optional to use subclass mock makers.

I also first tried to avoid trackAndCleanUpMocks(). The difficulty actually comes from the fact MockitoListeners are ThreadLocal before this change, and it's a requirement to track mocks generated from other threads because FailOnTimeout basically runs tests in different threads than the thread used to run Before and After. Before this change user can have multiple DefaultMockitoSession, as long as they are in different threads (ThreadsRunAllTestsHalfManualTest also verifies that behavior) and not interfering with each other. However if we convert DefaultMockitoSession to have "thread global" influence ThreadRunAllTestsHalfManualTest will fail because cross-threads mock cleanup will invalidate mocks created in tests running in a different thread.

I think I have a way (a bit hacky though) to make it work, but I don't think its complication outweighs the new API (given that it's already not transparent to users). It doesn't encourage clean tests in multi-threading scenario either imho. However I can still put my thought here and let you decide if we should take it:

Don't reject registration of global listeners if it's from a different thread for the same type
Only clean up mocks when the last MockTracker is unregistered

Then it basically treats the life of any DefaultMockitoSession as mock lives. It doesn't allow user to precisely control mock lives but with some care users should still be able to avoid OOM.

ttanxu · 2019-02-19T02:44:06Z

Oh... I already added InlineMockMaker interface so not all mock makers need to implement cleanUpMock(). I'm not sure if the plugin way is still preferred over current change because for most dexmakers they're still just implementing the basic MockMaker interface and nothing should break. While they can choose to implement InlineMockMaker if clean up is necessary.

ttanxu · 2019-02-21T01:02:26Z

Just to be clear that I'm waiting for @mockitoguy 's response on if we would like to go for that approach in exchange of removing trackAndCleanUpMocks() API, just in case we're waiting for each other and it's going nowhere.

TimvdLippe · 2019-02-21T11:51:56Z

As a sidenote: the refactoring with the new InlineMockMaker interface is 👍 for me 😄

mockitoguy · 2019-02-24T02:39:26Z

Hey @ttanxu, so you you're saying that the approach I suggested in a) wouldn't work?

mockitoguy · 2019-02-24T02:44:21Z

Oh... I already added InlineMockMaker interface so not all mock makers need to implement cleanUpMock()

Great! I'll review again this weekend.

mockitoguy · 2019-02-24T02:45:06Z

As a sidenote: the refactoring with the new InlineMockMaker interface is 👍 for me 😄

@TimvdLippe, I'm with you. It's a good idea to have a separate interface and stay compatible.

ttanxu · 2019-02-24T03:18:06Z

Hey @mockitoguy , what you suggested could solve the same issue that is solved by InlineMockMaker in current PR (the MockMaker contract breakage).

trackAndCleanUpMocks() was introduced for other reason, but could use different tech to remove its necessity. I explained the detail in previous comments (mostly because of the multi-thread implementation of FailOnTimeout in JUnit).

mockitoguy · 2019-02-25T15:17:44Z

@ttanxu, thank you for explanation. I'm looking into it.

mockitoguy · 2019-02-27T03:58:20Z

My comment is short but I put a lot of consideration into this. Given all arguments, I agree that a new API is the best way. Before we ship it, I want to bounce one more idea. What if we tie MockMaker with MockitoSession? When the session completes it tears down its MockMaker/cleans up all mocks. We could use ThreadLocal. Thoughts?

ttanxu · 2019-02-27T19:51:23Z

Correct me if I'm wrong.

It seems you're suggesting one MockMaker per thread. The critical part of this approach is the mock map in the inline mock maker needs to be a ThreadLocal, which means mocks won't look have handlers tied to them in threads other than the thread where they were created, and thus won't appear to be a mock and method invocations on it might trigger exceptions. That's not likely a compatible change to existing use cases because if FailOnTimeout is used Befores and Afters are executed in a different thread than the test method.

If we still only have one MockMaker across all threads then the structure is pretty much the same as current change except that instead of calling InlineMockMaker#clearUp() we just tear down the entire MockMaker. I'm not sure if this is better than calling clearUp(). It does sound like a viable solution w/o introduction to InlineMockMaker as we can just swap the instance of MockMaker in PluginRegistry. The con is that's more of a systematic refactor than InlineMockMaker#clearUp() approach and it reduces the cleanness of current code (we can't use final for mockMaker in PluginRegistry anymore). BTW this doesn't help avoid a new API trackAndClearUpMocks() in MockitoSession. I'll leave this decision to you.

mockitoguy · 2019-02-28T03:33:01Z

Great points, thank you for taking the time to explain. Why do we need to keep track of specific mocks? Can we just clear all?

ttanxu · 2019-02-28T06:54:35Z

Oh... You say we don't have to save created mocks in a list in MockTracker? I guess we can just clear all mocks. Especially considering there can't be multiple mock-tracking sessions open at the same time, mocks created before the session is opened may be leaked.

Removing MockTracker can simply this change a lot, especially we don't need global listeners. I'll update the change.

ttanxu · 2019-02-28T08:06:28Z

Actually, I recall that the thought of tracking specific mocks stemmed from the thought that usually mocks have different lives. E.g. mocks held by static variables, created in BeforeClass, in Before and in individual tests may need to have different lives. It would be nice if we can keep mocks created in BeforeClass for later reuse.

Current impl is half baked because there can only be one tracking session, but I want to keep cleanUp(Object) signature (or we can have cleanUp(Object...) instead so that we can clean up everything if Object array is null) for InlineMockMaker just in case we want to support the case above.

With that said I'll change cleanUp(Object) to cleanUp(Object...) and use null array for clean up everything.

ttanxu · 2019-02-28T15:55:11Z

Oh... Besides, if we just want to clear everything, it doesn't seem natural to me to add the API to MockitoSession, maybe we can add an API to Mockito?

mockitoguy · 2019-02-28T17:31:59Z

Thank you for considering! I want us to strive for simplicity. I'll think about the new API for Mockito (or MockitoFramework).

If we have a static method like "Mockito.framework().clearAllMocks()" would it work for you?

E.g. mocks held by static variables, created in BeforeClass, in Before and in individual tests may need to have different lives

Do we know legit use cases for "static" mocks? It seems an anti-pattern.

ttanxu · 2019-02-28T22:37:01Z

Finally cleaned up all remains from previous change. It's a lot simpler now. Please take a look.

I can think of a case for static mocks where constructing a mock may be expensive and someone wants to reuse it across tests. That's not an argument why static mocks have to be used though, they can definitely reconstruct those mocks.

TimvdLippe

This is definitely looking better, nice work! Just some small questions

src/main/java/org/mockito/plugins/InlineMockMaker.java

TimvdLippe · 2019-02-28T23:33:36Z

src/main/java/org/mockito/internal/creation/bytebuddy/InlineByteBuddyMockMaker.java

@@ -271,6 +272,15 @@ public void resetMock(Object mock, MockHandler newHandler, MockCreationSettings
        }
    }

+    @Override
+    public void cleanUpMock(Object mock) {


We are never calling this method with an actual mock. Is there a situation this should be the case? Could you maybe provide an example for it? (If it is not necessary, we might as well make this method clearAllMocks as well)

I said that in comments before I updated the PR.

If we will go with clearAllMocks() we need an additional method if there is strong feature request for keeping some mocks alive later.

That might be over-engineering, but I'm not sure how open you are to introduce new method to incubating class or change existing incubating method signature in this case. It would require mock maker changes later.

Maybe we can change the public API to take in mocks as well? I might just do that quickly.

Updated the PR, but I'm open to discuss this forward. If you two believe cutting off the mock specific clear up feature is better then I'm willing to make another change.

I'm just afraid that there are cases where callers might want to keep some mocks, but still need to release some other of them.

I think it is fair to keep both. Since we are introducing an interface, we are most flexible if we keep them both in. That said, I think it is better to also introduce clearAllMocks on InlineByteBuddyMockMaker as well as rename cleanUpMock to clearMock to keep it consistent.

Updated them for consistency. Thanks.

Due to the introduction of map from weak reference from mock instance to its invocation handler, Mockito became vunerable to memory leaks as there are multiple situations where Mockito could unintentionally hold strong references to mock instances in the map record. The strong references could be through spiedInstance for spies, and arguments used to faciliate method stubbing. Mockito could never know if the arguments passed in for method stubbing are also strongly referenced somewhere else or not, so Mockito needs to save a strong reference to these arguments to avoid premature GC. Therefore to solve cyclic strong references through arguments Mockito needs to explicitly know when mocks are out of their life, and clean up all internal strong references associated with them. This commit also fixes mockito#1532 and fixes mockito#1533.

mockitoguy · 2019-03-02T14:52:16Z

Reviewing...

mockitoguy · 2019-03-02T22:00:57Z

We are ready. Thank you @ttanxu and @TimvdLippe for patience! This is the best pragmatic step to resolve the problem. In the future, perhaps we can avoid having the weak map of references in a first place.

@ttanxu, I made some tweaks to tests, documentation, I've also renamed the method to "clearMocks() -> clearInlineMocks()", for consistency. Take a look and see if you're OK with this. Let us know and I'll merge this. Nice work with this change!

@TimvdLippe, I did some cosmetic changes, please review if you want. Thanks!

TimvdLippe · 2019-03-02T23:44:37Z

I will review this on Monday in the office. Thanks for the polish!

TimvdLippe

Some small documentation changes. Also, I think we should do this eagerly in the JUnit runner and extension. I don't see a situation where this is undesirable to do.

TimvdLippe · 2019-03-04T12:07:14Z

src/main/java/org/mockito/MockitoFramework.java

+     * <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).


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.

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.

Ah okay, I missed that discussion. Thanks for the clarification.

TimvdLippe · 2019-03-04T12:07:49Z

src/main/java/org/mockito/Mockito.java

+ * inline mocking causes memory leaks.
+ * There is no clean way to mitigate this problem completely.
+ * Hence, we introduced a new API to explicitly clear mock state (only make sense in inline mocking!).
+ * See example in {@link MockitoFramework#clearInlineMocks()}.


Suggested change

* See example in {@link MockitoFramework#clearInlineMocks()}.

* See example usage in {@link MockitoFramework#clearInlineMocks()}.

TimvdLippe · 2019-03-04T12:09:21Z

src/main/java/org/mockito/Mockito.java

+ *
+ * In certain specific, rare scenarios (issue <a href="https://github.com/mockito/mockito/pull/1619">#1619</a>)
+ * inline mocking causes memory leaks.
+ * There is no clean way to mitigate this problem completely.


Suggested change

* There is no clean way to mitigate this problem completely.

* It was not possible to resolve this issue without introducing an additional API.

* Any inline MockMaker should therefore implement the additional interface {@link org.mockito.plugins.InlineMockMaker} to allow Mockito to properly clean up mocks.

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.

ttanxu

Generally looks good, just one comment. I suppose I don't have to address @TimvdLippe 's comment this time, since they're all about documentation (which I am not good at).

BTW, there is a conflict in version.properties.

ttanxu · 2019-03-04T17:43:49Z

src/test/java/org/mockito/internal/framework/DefaultMockitoFrameworkTest.java

+    @Test
+    public void clears_all_mocks() {
+        //clearing mocks only works with inline mocking
+        assumeTrue(Plugins.getMockMaker() instanceof InlineMockMaker);


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.

+1 I'm fixing it now.

mockitoguy · 2019-03-05T15:51:59Z

test user can safely call clearInlineMocks() and clearMock() even when mock maker is not a inline mock maker

@ttanxu, great point!

mockitoguy · 2019-03-05T15:52:35Z

I'm making the final tweaks based on the code review and I'm merging.

arturdryomov · 2019-03-05T17:54:25Z

This is awesome, thank you guys! I’ll try this API on our codebase and samples I’ve provided in original issues. Also — kudos for including tests reproducing the behavior I’ve described 😉

s2131 · 2020-07-14T10:37:00Z

Hi, Just to let you know I am using the new API already with v2.28.2 and for me it was breaking the Strict test with following exception:

 org.mockito.exceptions.misusing.NotAMockException: Argument passed to Mockito.mockingDetails() should be a mock, but is an instance of class Cache$MockitoMock$149288076!
   
    at org.mockito.internal.runners.DefaultInternalRunner$1$2.testFinished(DefaultInternalRunner.java:63)
 ...

I had to move it to @AfterClass to mitigate the problem

ttanxu · 2020-07-19T16:58:39Z

@s2131 You are not allowed to interact with mocks anymore after calling that API because all internal states of mocks are cleaned up after the API call. That includes all delayed actions or interactions from other threads.

Unfortunately it's never as easy as just calling that API.

ttanxu force-pushed the release/2.x branch from 59efe50 to 5cf0331 Compare February 12, 2019 22:52

TimvdLippe self-requested a review February 12, 2019 22:56

ttanxu force-pushed the release/2.x branch from 5cf0331 to 9343bec Compare February 12, 2019 23:07

TimvdLippe requested changes Feb 13, 2019

View reviewed changes

src/main/java/org/mockito/plugins/MockMaker.java Outdated Show resolved Hide resolved

ttanxu force-pushed the release/2.x branch from 9343bec to cbc1101 Compare February 13, 2019 17:46

ttanxu force-pushed the release/2.x branch 2 times, most recently from c1ad3a0 to f464f7a Compare February 28, 2019 22:20

ttanxu force-pushed the release/2.x branch from a2fc2d6 to 223c222 Compare February 28, 2019 22:35

TimvdLippe reviewed Feb 28, 2019

View reviewed changes

ttanxu force-pushed the release/2.x branch from 223c222 to 85ea8eb Compare March 1, 2019 01:09

ttanxu force-pushed the release/2.x branch from 85ea8eb to 91c8e9c Compare March 1, 2019 16:53

mockitoguy added 6 commits March 2, 2019 09:58

Tweaked the javadoc and method names

808bae8

Replace conditional with JUnit assume

e44d76e

Tweaked tests

a6027ac

Tweaked javadoc

c3c0d0a

Javadoc and since tags

ebb9b85

Next minor version

cc38b6c

TimvdLippe requested changes Mar 4, 2019

View reviewed changes

ttanxu commented Mar 4, 2019

View reviewed changes

mockitoguy added 2 commits March 5, 2019 07:54

Added missing coverage

d61a8be

Javadoc

1641868

mockitoguy changed the title ~~Fixes #1614: Clean up mocks when session finishes.~~ New API to clean up all inline mocks after test Mar 5, 2019

Merge branch 'release/2.x' into release/2.x

959cba4

mockitoguy merged commit a7ee084 into mockito:release/2.x Mar 5, 2019

ttanxu mentioned this pull request Mar 11, 2019

Implement new InlineMockMaker interface in Mockito 2.25.0 linkedin/dexmaker#137

Closed

TimvdLippe mentioned this pull request Oct 29, 2019

Memory leak in mockito-inline calling method on mock with at least a mock as parameter #1614

Closed

nodece mentioned this pull request Jul 27, 2022

Revert "[fix][ci] Fix tests memory leak due to mockito-inline (#15513)" apache/pulsar#16820

Closed

4 tasks

	* See example in {@link MockitoFramework#clearInlineMocks()}.
	* See example usage in {@link MockitoFramework#clearInlineMocks()}.

	* There is no clean way to mitigate this problem completely.
	* It was not possible to resolve this issue without introducing an additional API.
	* Any inline MockMaker should therefore implement the additional interface {@link org.mockito.plugins.InlineMockMaker} to allow Mockito to properly clean up mocks.

New API to clean up all inline mocks after test #1619

New API to clean up all inline mocks after test #1619

Conversation

ttanxu commented Feb 12, 2019 • edited

TimvdLippe commented Feb 12, 2019

TimvdLippe commented Feb 12, 2019

ttanxu commented Feb 12, 2019

codecov-io commented Feb 12, 2019 • edited

Codecov Report

mockitoguy commented Feb 13, 2019

TimvdLippe left a comment

Choose a reason for hiding this comment

ttanxu commented Feb 13, 2019

mockitoguy commented Feb 18, 2019

ttanxu commented Feb 19, 2019

ttanxu commented Feb 19, 2019

ttanxu commented Feb 21, 2019

TimvdLippe commented Feb 21, 2019

mockitoguy commented Feb 24, 2019

mockitoguy commented Feb 24, 2019

mockitoguy commented Feb 24, 2019

ttanxu commented Feb 24, 2019

mockitoguy commented Feb 25, 2019

mockitoguy commented Feb 27, 2019

ttanxu commented Feb 27, 2019

mockitoguy commented Feb 28, 2019

ttanxu commented Feb 28, 2019

ttanxu commented Feb 28, 2019 • edited

ttanxu commented Feb 28, 2019

mockitoguy commented Feb 28, 2019

ttanxu commented Feb 28, 2019

TimvdLippe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mockitoguy commented Mar 2, 2019

mockitoguy commented Mar 2, 2019

TimvdLippe commented Mar 2, 2019

TimvdLippe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ttanxu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mockitoguy commented Mar 5, 2019

mockitoguy commented Mar 5, 2019

arturdryomov commented Mar 5, 2019

s2131 commented Jul 14, 2020 • edited

ttanxu commented Jul 19, 2020

ttanxu commented Feb 12, 2019 •

edited

codecov-io commented Feb 12, 2019 •

edited

ttanxu commented Feb 28, 2019 •

edited

s2131 commented Jul 14, 2020 •

edited