Current coverage is 86.67% (diff: 100%)

No coverage report found for release/2.x at 57632e9.

Powered by Codecov. Last update 57632e9...48fbf89

When I tested this, it was invoking the mock-maker. The build now seems to stall completely, so not sure what is suddenly breaking all the tests. When personally testing (e.g. forcing the file and then run the tests in the IDE), the tests succeed.

The tests for core are executed - only extTest submodule fails (which AFAIR was problematic also when Travis was switched to containerized build). However, I've never seen it hung in that way.

Hmm, I get the same failing and hanging behavior when running the following locally:

export MOCK_MAKER=mock-maker-inline && ./gradlew --stop && ./gradlew clean ciBuild && unset MOCK_MAKER

It stalls at the following, indicating there are core tests failing.

Building 73% > :test > 988 tests completed, 16 failed, 9 skipped

Perhaps remove the mockMakerFile.delete() line from build.gradle to make sure the file isn't removed after all when running the tests.

As for whether the inline-mock-maker is used, adding the following test to the current release/2.x branch shows that it isn't used:

public class TemporaryTest {

    @Test
    public void testInlineEnabled() {
        if (System.getenv("MOCK_MAKER").equals("mock-maker-inline")) {
            assertThat(
                "MOCK_MAKER environment variable set but not applied",
                Plugins.getMockMaker(),
                CoreMatchers.instanceOf(InlineByteBuddyMockMaker.class)
            );
        }
    }
}

@raphw I am a bit confused with this PR. On the one hand enabling the inline mockmaker seems to work fine locally and we have not received any reports of actual exceptions when using the mockmaker. However, this CI build shows 2 failures and is endlessly looping. Could you verify that the mockmaker is working as intended or identify if there is any possible solution to fix the issues?

The failure in the CI build concerns this method. The second line crashes with an NPE when mockmaker is enabled, since deep_stubbed.iterator().next() returns null.

ListContainer deep_stubbed = mock(ListContainer.class, withSettings().defaultAnswer(RETURNS_DEEP_STUBS).serializable());
when(deep_stubbed.iterator().next().add("yes")).thenReturn(true);

I can reproduce this every time by manually enabling and disabling mockmaker by adding and removing respectively the file src/test/resources/mockito-extensions, containing mock-maker-inline, and running this single test from the IDE.

@nhaarman Ah cool, thanks for the investigation! Could you @Ignore that specific test to see if the CI succeeds without it? Then we can extract an issue for just that test to investigate why it is failing. It should not block us testing all other details of the new mockmaker

I have found why the build was stalling. When using the mock-maker and trying to mock a HashMap, a stack overflow occurs:

Exception in thread "Reference Handler" java.lang.StackOverflowError
	at org.mockito.internal.creation.bytebuddy.MockMethodDispatcher.get(MockMethodDispatcher.java:20)
	at java.util.HashMap.get(HashMap.java:556)
	at sun.reflect.Reflection.filterMethods(Reflection.java:291)
	at java.lang.Class.privateGetDeclaredMethods(Class.java:2701)
	at java.lang.Class.getDeclaredMethod(Class.java:2128)
	at java.util.HashMap.get(HashMap.java:556)
	at sun.reflect.Reflection.filterMethods(Reflection.java:291)
	at java.lang.Class.privateGetDeclaredMethods(Class.java:2701)
	at java.lang.Class.getDeclaredMethod(Class.java:2128)
	at java.util.HashMap.get(HashMap.java:556)

I will change these tests to mock Map instead of HashMap, and @Ignore any failing tests (there are 7).
I will create a separate issue for the HashMap mocking, what do you want to do with the ignored tests?

Darn, since we are using hash maps internally for the mock maker creation, this creates an infinite loop when attempting to look up the mocking logic. We need to hard-code this type exclusion instead of adding a map.

I have merged the current release/2.x branch into this branch, and (hopefully) made CI pass.
If you want me to do a rebase on release/2.x instead, let me know.

Hm that's a lot of ignored tests 😭 including for the normal mock maker. Could you do AssumeFalse(mockmakerinlineisenabled)?

@nhaarman I'd rather go for the rebase option. It helps to maintain a clean history.

@bric3 Yep, though I'm a bit cautious rewriting history on public branches. If there's no objection, I'll do so tomorrow.

It's rebasing the PR branch on top of a public branch, the only rewrite is on your profile branch. And still the previous commits are not lost (reflog, backup branch)

Also I'd remove the change on hash map, this should be fixed by the mock maker.

@TimvdLippe's vision was to make sure CI passes so this PR could be merged. What would be the final stance on this?

It's just wrong to modify a test that is supposed to be working just to allow a PR to be merged. Ideally I'll wait on #818 to be fixed.

One other thing is to document why those tests are disabled with the inline mockmaker. On this I think the team is the best to explain why

I am also okay with fixing #818 first, initially I thought there were just 2 tests failing but apparently there are a lot more failing 😭

#818 is now fixed, we should take a look without so much test changes

There are 7 tests that fail now:

MocksSerializationForAnnotationTest. should_allow_throws_exception_to_be_serializable
MocksSerializationTest. should_allow_method_delegation
ConfusedSignatureTest. should_mock_method_which_has_generic_return_type_in_superclass_and_concrete_one_in_interface
DiamondInheritanceIsConfusingMockitoTest. should_work
ClassCacheVersusClassReloadingTest. should_not_throw_ClassCastException_when_objenesis_cache_disabled
DeepStubsSerializableTest. should_discard_generics_metadata_when_serialized_then_disabling_deep_stubs_with_generics
DeepStubsSerializableTest. should_serialize_and_deserialize_parameterized_class_mocked_with_deep_stubs

This task makes me think that as devs on our local post, when we run ./gradlew :test it runs with both mock maker. And it's better for external contributors too as they don't have to think to run the test with another environment variable.

These two fail for the same reason

MocksSerializationForAnnotationTest.should_allow_throws_exception_to_be_serializable
MocksSerializationTest.should_allow_method_delegation

They don't capture the invocation of the public method on the private inner class Bar, if the visibility modifier is augmented then the mock behaves as expected.

ConfusedSignatureTest and DiamondInheritanceIsConfusingMockitoTest also don't intercept the method.

ClassCacheVersusClassReloadingTest has another problem due to classloaders config, however this test is not necessary for the inline mock maker.

Failure of tests in DeepStubsSerializableTest are difficult to debug. I do remember some difficulties regarding serialization.

I think we can close this PR for now and properly fix this when we can actually fix Java 9 compatibility. That is possible when ASM6 has been officially released

Not sure if anything has changed, but having a test suite that claims to test the inline-mock-maker but doesn't actually, is even worse than having no tests for it at all.

I was on mobile while writing my last comment and it did not show me the branch. I was under the impression this was targeting master (and would therefore fail on Java 9). However that is not the case, as I remembered incorrectly.

@raphw Can you take another look at this one?

@raphw Running export MOCK_MAKER=mock-maker-inline locally and then running our test suite still fails some tests. It seems that .travis.yml is not correctly setting this environment variable eitherway? Anyways, I think this is still an issue, but I am not sure how to tackle this problem.

We haven't received any reports lately with regards to the Inline mockmaker and potential issues. I think Travis is working okay now. Therefore, I am closing this PR.