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

New <T, R> R createMock(Class<T> toMock) signature fails when used for vararg calls #259

Open
EdGue42 opened this issue Jun 26, 2020 · 7 comments

Comments

@EdGue42
Copy link

EdGue42 commented Jun 26, 2020

See this code:

import java.util.Arrays;
import org.easymock.EasyMock;

class B { }

public class Mcve {
    static void foo(B... someBs) {
        System.out.println("someBs = " + Arrays.toString(someBs));
    }
    public static void main(String[] args) {
        foo((B) EasyMock.createMock(B.class));
        foo(EasyMock.createMock(B.class));
    }
}

When I compile and run this with EasyMock 3.4, the output I get is:

someBs = [EasyMock for class B]
someBs = [EasyMock for class B]

But when I run it with EasyMock 4, the output is:

someBs = [EasyMock for class B]
Exception in thread "main" java.lang.ClassCastException: B$$EnhancerByCGLIB$$e255c05a cannot be cast to [LB;
        at Mcve.main(Mcve.java:15)

In other words: the old <T> T createMock(Class<T> toMock) ensures that the compiler sees B required, and thus ensures that the argument is pushed into the required array. With the new signature, things go haywire.

Sure, it can be mitigated by adding that cast, but that feels really wrong.

@henri-tremblay
Copy link
Contributor

Interesting. I need to put some more thought into this new typing.

The real mitigation is foo(EasyMock.<B>createMock(B.class)) (with the latest EasyMock version). The type witness will remove the inference ambiguity.

@EdGue42
Copy link
Author

EdGue42 commented Jun 26, 2020

Interesting, too. I have not used type witnesses in years.

I guess the change to the signature was made to improve things, and that this example is very specific.
On the other hand, it is not "artificial", we hit this in our test suite, and it took me quite a bit of time what exactly was going on.

It seems very counter intuitive that I have to do anything when mocking arguments to a method call on the fly, just because the method takes a vararg.

@henri-tremblay
Copy link
Contributor

Yes. The change made is possible to mock generics (e. g. List<String> list = mock(List.class);) But it means in some cases the inference doesn't work well and you need a type witness. The worst case is var a = mock(A.class).

So I might rollback the change and add some mockGeneric or mockInfered or I don't know what... If you have a name idea I'm all in.

@EdGue42
Copy link
Author

EdGue42 commented Jun 26, 2020

I have to admit: our team is mostly into Mockito these days.
So I guess you should ask some more "active" EasyMock users for their input,
I am not in a position to give legit guidance here ;-(

Having said that: the problem with "yet another" mockXXX() method that I see: having createMock, createNiceMock, createStrictMock ... is confusing enough already.

Unfortunately more degrees of freedom ... mean more things one has to know about. It really depends on what you think the underlying "theme" of EasyMock is, and how that comes into play here.

@henri-tremblay
Copy link
Contributor

Mockito has the same problem.

It is super annoying to use because of that. I mock generics a lot.

There are 3 solutions I'm aware of:

  1. @SuppressWarning("unchecked"): The mockito way
  2. T mock(Class<?> c): The EasyMock way
  3. T mock(TypeRef<T> ref): The jackson way (you then do List<String> l = mock(new TypeRef<List<String>>())

@EdGue42
Copy link
Author

EdGue42 commented Jun 29, 2020

I guess the real answer is somehow different ... for us. We simply changed the way we write our unit tests.

The test I had in my production code that failed was like:

@Test(expected=NullPointerException.class)
public void testCtorWithNullWhatever() {
      new Foo(null, createMock(Bar.class));
}

Where the Bar parameter is actually a vararg. Nowadays, I write the same test in a way that doesn't care about the signature of "create" methods:

@Mock
private Bar someBar;

@Test(expected=NullPointerException.class)
public void testCtorWithNullWhatever() {
      new Foo(null, someBar);
}

My "main" point of this issue is the fact that our existing tests stopped working, and required a strange change to be working again ...

@henri-tremblay
Copy link
Contributor

Yes. I broke compatibility in some inference cases like this one. My hope was that they were pretty rare but that you can now remove a lot of@SuppressWarnings. It's true that the @Mock annotations is another way to fix it.

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

No branches or pull requests

2 participants