Expectations with matchers in varargs methods do not work properly #292

Open
Vampire opened this Issue Jun 13, 2016 · 6 comments

Projects

None yet

2 participants

@Vampire
Vampire commented Jun 13, 2016 edited

I spent the day to find out that my problem actually is the following JMockit bug:

If I have a method signature like the following

FacesMessage getMessage(Severity severity, String msgKey, Object... msgObjects)

then it does not work to write in an Expectations block

messageFactory.getMessage(SEVERITY_ERROR, "myKey", "a,23", anyString, anyString);

or

messageFactory.getMessage(SEVERITY_ERROR, anyString, "a,23", anyString, anyString);

or

messageFactory.getMessage(withArgThat(isA(Severity.class)), "myKey", "a,23", anyString, anyString);

But it does work if I write either

messageFactory.getMessage(withArgThat(isA(Severity.class)), withEqual("myKey"), withArgThat(is("a,23")), anyString, anyString);

or also if I write

messageFactory.getMessage(SEVERITY_ERROR, "myKey", "a,23", "foo", "bar");

The problem as far as I have seen through debugging JMockit code is, that no call to mockit.internal.expectations.transformation.ActiveInvocations#moveArgMatcher is generated during instrumentation, because of the matcher being "hidden" inside the Object[].

If no matchers are present at all like in my last example, mockit.internal.expectations.invocation.ArgumentValuesAndMatchersWithVarargs#isMatch just compares all values literally with the LenientEqualityMatcher, but if there is at least one matcher and it is in the var-args range, then the matchers array in ArgumentValuesAndMatchersWithVarargs cannot be used as is.

Taken my first example messageFactory.getMessage(SEVERITY_ERROR, "myKey", "a,23", anyString, anyString);, the matchers array would have the two anyString matchers at indices 0 and 1 instead of 3 and 4 as would be necessary for the current matching algorithm.

So I guess you either have to somehow make the moveArgMatcher calls get generated correctly for varargs method calls, or change the matching algorithm, or maybe transform non-special arguments to calls to withEquals() in the InvocationBlockModifier, like any* that are transformed to the respective method calls.

@rliesenfeld
Member

Note this is not a bug, but a documented limitation of the argument matching API: http://jmockit.org/tutorial/Mocking.html#varargs (although, this fact is in need of being added to the API documentation as well).

@Vampire
Vampire commented Jun 13, 2016

Ah, ok, that's pretty bad though, don't you think?
Is there a reason this is not solved in a way that makes it intuitively work?
I'd never have expected this behaviour like it is.

Also, I have read the section you mentioned.
Am I wrong in understanding that this section is only talking about the varargs parameters, not the regular parameters?
So that in the regular range you should be able to use no, some or all matchers, but in the varargs parameters range you have to use either all values or all matchers.

If I understood this correct, this is still not what is implemented.
What is currently implemented is, that you either have to only use values in the varargs section, or you have to use only matchers in the varargs section and the last regular parameter must be a matcher too, as for this the moveArgMatcher call will be generated and thus the matcher indizes fixed.

@rliesenfeld
Member

Varargs methods are rare, and mocking is something to do only occasionaly; doesn't seem too bad to me.

The reason it's not implemented is that is difficult to.

@Vampire
Vampire commented Jun 14, 2016 edited

Hm, ok, then as you don't see this as bug, I'd like to have this as improvement request instead then. :-)

And what about the documentation, did I misunderstand the documentation or is the documentation not stating what is actually implemented as I described in my last comment?

@rliesenfeld
Member

Ok, it can be accepted as an enhancement request.

The tutorial says "it's not valid to combine both kinds of value-passing [ie, a mix of literals and matchers] for the same expectation". It's talking about all parameters, varargs or not.

@Vampire
Vampire commented Jun 14, 2016

Well, the full sentence is "However, it's not valid to combine both kinds of value-passing for the same expectation, when targeting a varargs parameter." All I'm saying regarding the documentation of the current behaviour is, that it is not too clear, because "varargs parameter" is only the optional parameters in the end, the regular parameters before are not the varargs parameters.

While actually with the current implementation these cases are valid working ones:

  • all varargs parameters are concrete values
    • each regular parameter can individually use matchers or not
  • all varargs parameters use matchers
    • the last regular parameter must use a matcher
    • all other regular parameters can individually use matchers or not
  • some varargs parameters use matchers, some concrete values
    • this requires that the varargs parameters that use matchers all come first and the varargs parameters that use concrete values come last and additionally the last regular parameter must use a matcher
  • the whole varargs array uses a matcher
    • each regular parameter can individually use matchers or not
    • this includes the possibility to use a construct like the following to only use matchers for the varargs parameters but for no regular parameter
messageFactory.getMessage(FacesMessage.SEVERITY_ERROR, "myKey", with(new Delegate<Object[]>() {
   void delegate(Object[] varargs) {
      assertThat(varargs[0], is("a,23"));
      assertThat(varargs[1], is(instanceOf(String.class)));
      assertThat(varargs[2], is(instanceOf(String.class)));
   }
}));

And these cases are not:

  • some varargs parameters are concrete values, some are matchers if not the rules of above are obeyed
  • all varargs parameters use matchers but the last regular parameter is a concrete value
    • While this actually can seem to work if the matchers of the varargs parameters match the called regular parameters, but this can lead to very strange bugs and unexpected behaviour in the tests

Maybe to make this a bit safer some check could be done, that if a method is a varargs method and there are matchers in the varargs parameters, then the last regular parameter must be a matcher or something like that and fail if it is no the case? Users tend to not read documentation (or to not have it in mind, actually I have read that part of the tutorial before, I just didn't remember) and a programmatic check and fail would be good, as long as matchers in varargs are not fully supported as requested by this enhancement request. :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment