HasValue calls Scalar::value twice on mismatch

[andreoss]

https://github.com/llorllale/cactoos-matchers/blob/master/src/main/java/org/llorllale/cactoos/matchers/HasValue.java#L53

HasValue should call value only once.

[victornoel]

@andreoss indeed, that's a well known problem but quite hard to fix because of the way Matcher interface is designed: because it had two methods that can be called by assertion in case of mismatch, it's creating many problems like the one you highlight. The problem happens when testing inputs for example, they are sometimes read twice.

It's expecting a lot from the object manipulated in those methods and that they behave the same every time they are called... I suspect sometimes that it was made with datastructures in mind and not objects.

I think that because of this, even if we find a way for value to be called only once here, there will still be problems because the behaviour of the marcher will anyway be called multiple time.

So either we decide to do Matcher as the main interface for assertion or we decide it's the responsibility of the test to take care of providing objects to the matchers that behave in a consistent manner.

WDYT?

[andreoss]

@victornoel We could reimplement Satisfies to not use MatcherOf and cache extractor return value.
And reimplement HasValue with Satisfies.

    public HasValue(final Matcher<T> mtr) {
        super(new Satisfies<>("Scalar with ", Scalar::value, mtr));
    }

    public Satisfies(
        final String description,
        final Func<? super X, ? extends Y> extractor,
        final Matcher<? super Y> matcher
    ) {
        this.desc = description;
        this.conv = new UncheckedFunc<>(new StickyFunc<>(extractor));
        this.matcher = matcher;
    }

[victornoel]

@andreoss even though this would help solve the problem, I think the best would be to find an architectural solution that will prevent us from making mistakes. I'm a bit worried of using state in the matchers with caching and so on, also, this would solve the problem for one matcher, but not all of them, so we will have to be careful and write a lot of tests just to ensure things work as expected while a nice architectural solution should give us the same result by construction :)

So we have two choices:

• say it's the responsibility of the test to take care of providing objects that always behave the same
• introduce a dedicated interface for cactoos-matchers Matchers and provide an implementation that would allow also to use hamcrest matchers (and have Assertion accept hamcrest Matchers on top of cactoos-matchers').

I would be ok to go with the second solution :)

[andreoss]

@victornoel The other way to archive this is to change Assertion (#156).
new AssertThat(() -> value, matcher) - now AssertThat evaluates scalar, and matcher validates scalar's value.

[victornoel]

@andreoss I don't think the problem is with the assertion itself: the root of the problem is that Matcher interface has two methods, one to check if the match failed and one to populate a description in case of mismatch. That's why there are two calls to value() in HasValue for example.

There are 3 solutions from my point of view:

• change the interface Matcher (i.e., introduce our own)
• ensure each of the Matcher implementations of cactoos caches the value of the object under test
• ensure the user when writing tests takes care of caching the values of the object under test

The second can be quite tricky and IMHO dangerous because the behaviour of the Matcher will be kind of complex and difficult to understand. The third one is easy for this library, but won't make the user completely happy.

Do you see other solutions? Or do you think I'm missing something that would make your proposals more adequate?