Implement and use `Matcher#describeMismatch` throughout `axon-test` module

[fernanfs]

When using the Hamcrest Matcher based validation logic in ResultValidator, like expectEventsMatching(Matcher<? extends List<? super EventMessage<?>>> matcher), it is hard to see the actual mismatch, if something goes wrong.

This is due to the following implementation:

https://github.com/AxonFramework/AxonFramework/blob/e567af75fb44be19006533ba2aac1d0db614edd8/test/src/main/java/org/axonframework/test/aggregate/ResultValidatorImpl.java#L118-L129

It would be better to request the Matcher to describe the mismatch using the describeMismatch(Object item, Description description) method. This will result in a message that highlights the actual mismatch, not the whole matching logic.

I would suggest to change the implementation to:

    public ResultValidator<T> expectEventsMatching(Matcher<? extends List<? super EventMessage<?>>> matcher) {
        if (!matcher.matches(publishedEvents)) {
            final StringDescription description = descriptionOf(matcher);
            description.appendText("\nMismatch:\n");
            matcher.describeMismatch(item, description);
            reporter.reportWrongEvent(publishedEvents, descriptionOf(publishedEvents, matcher), actualException);
        }
        return this;
    }

    private StringDescription descriptionOf(Object item, Matcher<?> matcher) {
        StringDescription description = new StringDescription();
        matcher.describeMismatch(item, description);
        return description;
    }

[smcvb]

Thanks for filing this issue with us, @fernanfs. I have two main question before we'd start working on this suggestion of yours:

1. Do you have an example of such a mismatch? So, what it looks like right now, and what it would look like with the recommended adjustment?
2. I'm pretty confident this is not restricted to event matching only. Wouldn't this change require adjusting the entire description logic throughout Axon Framework's test module?

[fernanfs]

Thanks for your feedback @smcvb . I've created a sample project demonstrating the problem, which can be found here: https://github.com/fernanfs/axon-framework-issue2400-sample

In the example you'll find the following validation logic:

@Test
  internal fun `perform validation within the fixture using hamcrest`() {

    AggregateTestFixture(ExampleAggregate::class.java)
      .given(ExampleCreatedEvent("example"))
      .`when`(
        CommandWithValues(
          id = "example",
          intValue = 42,
          stringValue = "Arthur Dent",
          boolValue = false,
          longValue = 4711
        )
      )
      .expectEventsMatching(
        exactSequenceOf(
          messageWithPayload(
            eventMatcher()
          )
        )
      )

  }

  private fun eventMatcher(): Matcher<ValuesReceivedEvent> =
    allOf<ValuesReceivedEvent>(
      isA(ValuesReceivedEvent::class.java),
      hasProperty("intValue", equalTo(42)),
      hasProperty("stringValue", equalTo("Arthur Dent")),
      hasProperty("boolValue", equalTo(false)),
      // this is the value failing
      hasProperty("longValue", equalTo(4712))
    )

This test will have a mismatch on the very last check, which is the longValue where it expects a value of 4712 instead of 4711. Executing this test, will yield the following result:

The published events do not match the expected events.Expected :
list with exact sequence of: <Message with payload <(is an instance of de.digitalfrontiers.axon.issue2400.ValuesReceivedEvent and hasProperty("intValue", <42>) and hasProperty("stringValue", "Arthur Dent") and hasProperty("boolValue", <false>) and hasProperty("longValue", <4712>))>> (FAILED!)
But got:
GenericDomainEventMessage: GenericDomainEventMessage{payload={ValuesReceivedEvent(id=example, intValue=42, stringValue=Arthur Dent, boolValue=false, longValue=4711)}, metadata={}, messageIdentifier='d3ea69b7-f621-4b94-a5ac-fb602a9bd512', timestamp='2022-09-28T10:29:11.116255Z', aggregateType='ExampleAggregate', aggregateIdentifier='example', sequenceNumber=1}
org.axonframework.test.AxonAssertionError: The published events do not match the expected events.Expected :
list with exact sequence of: <Message with payload <(is an instance of de.digitalfrontiers.axon.issue2400.ValuesReceivedEvent and hasProperty("intValue", <42>) and hasProperty("stringValue", "Arthur Dent") and hasProperty("boolValue", <false>) and hasProperty("longValue", <4712>))>> (FAILED!)
But got:
GenericDomainEventMessage: GenericDomainEventMessage{payload={ValuesReceivedEvent(id=example, intValue=42, stringValue=Arthur Dent, boolValue=false, longValue=4711)}, metadata={}, messageIdentifier='d3ea69b7-f621-4b94-a5ac-fb602a9bd512', timestamp='2022-09-28T10:29:11.116255Z', aggregateType='ExampleAggregate', aggregateIdentifier='example', sequenceNumber=1}
    at app//de.digitalfrontiers.axon.issue2400.ExampleAggregateTest.perform validation within the fixture using hamcrest$axon_framework_issue2400_sample(ExampleAggregateTest.kt:54)
    at java.base@17.0.2/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at java.base@17.0.2/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)

When using the same validation of the payload (yes, I'm skipping exactSequenceOf and messageWithPayload, I'll come back to that further down), but invoking describeMismatch, the result is something way more helpful:

hasProperty("longValue", <4712>)  property 'longValue' was <4711L>

With that information, I would be able to easily identify, where the mismatch actually is.

Coming back to exactSequenceOf and messageWithPayload, as well as your second remark: What certainly has to be reworked would be all the matchers provided by axon-test. All these matchers do not implement describeMismatch method and thus won't contain useful information.

Additionally axon Matcher implementations that extend org.axonframework.test.matchers.ListMatcher, try to do something similar with making the matcher stateful (which it certainly should never be) and store mismatches in the field failedMatchers. Just to show that information after a unsuccessful validation.

Thus all Matcher implementations must be changed and adapted to work correctly.

[smcvb]

Awesome description here, @fernanfs! Very helpful of you to go into these depths here. :+1:

Concerning the topic at hand: I assumed as much that this wasn't an easy adjustment around just event matching. I can also take a guess how this came into the framework: the Matchers are one of the oldest components in the framework (originating from 1.1, no less). They worked fine for the majority of their life cycle, but currently are simply aged.

Combining that bit of history with your arguments, I think it's fair to state we should put in some effort here. However, this is not necessarily a simple exercise, which takes some time to get it right. As it does not address anything that's broken at the moment, I feel the "Could" priority is justified.

We will do planning sessions for AF 4.7 soon(ish). I'll check whether we can move this into that release. However, I cannot make any promises that the AxonIQ team will be able to pick this up for 4.7.

Having said that, it's obviously completely fine if you, or anybody else, would want to provide a pull request to introduce this enhancement. If you do, know that the chances should not contain any breaking changes. I'd wager that's feasible, although I am not a 100% of that fact yet.

I hope to have clarified our stance on the matter, @fernanfs. And, again, if you'd want to do a PR for this, we'd more than welcome the contribution.

[fernanfs]

Thank you for your reply, @smcvb! I'm absolutely aware, that the number of changes to be made, are quite substantial, to have it consistent. Especially when trying to main a backwards compatible API within the custom matcher implementations. I'm currently giving that a shot, but do not count on me delivering a PR for the whole story anytime soon.

But what I can provide is a pull request that will only address the expectEventsMatching implementation by adding the output of describeMismatch as an additional information. This would not affect the backwards compatibility. For the Matcher-Implementations provided by axon, this still wouldn't yield any substantial improvements. But when using Matchers implemented by Hamcrest (or custom ones) that support the describeMismatch, this would massively improve the output quality.

[fernanfs]

And one last remark: For a 4.7 release it would make sense to plan on adding a way for users to use their own matching logic. Something like the following (not tested, just typing it here in the comment):

@FunctionalInterface
interface EventValidator {
  void call(List<EventMessage<?>> eventMessages);
}

public ResultValidator<T> expectEvents(EventValidator validator) {
  try {
    validator.call(publishedEvents);
  } catch(Exception e) {
    reporter.reportWrongEvent(publishedEvents, e);
  }
  return this;
}

This implementation would be massively flexible and allow for custom validation logic in any way. This would give the freedom to perform validation with assertj, assertk or any other kind of validation library that I'm feeling comfortable with using.

[smcvb]

Feel free to do a smaller form of PR, if you have the time for that, @fernanfs. For the other idea, I'd like you to ask to make a new issue. That'll make tracking it for us, and the community, a bit easier. :-)

[fernanfs]

I've created a PR, @smcvb. In this, I implemented the usage of the describeMismatch mechanic of Hamcrest Matchers. All changes should be backwards compatible. In the Reporter class, I've added a new method, that will receive the mismatch in addition to the description. The old method is not used anywhere in the codebase, thus I've deprected that method. Thus, it should be possible to accept the PR and still maintain backwards compatibility.