Interaction argument-matching treats all Mocks/Stubs of Comparables as equivalent

[jdpopkin]

Issue description

Hi! It looks like interaction argument-matching treats all Mocks/Stubs of classes that implement Comparable as equivalent to one another.

How to reproduce

Here's a test case to repro this:

  @FailsWith(TooFewInvocationsError)
  def "expect non-matching Comparables"() {
    ComparableExample bar = Mock()
    Comparable baz = Mock()
    Comparable qux = Mock()

    when:
    bar.foo(baz)
    then:
    1 * bar.foo(qux)
  }

  interface ComparableExample {
    void foo(Comparable arg)
  }

This test fails because bar.foo(baz) actually matches bar.foo(qux). This wouldn't be true if baz, qux, and the argument to foo weren't Comparable.

This probably has something to do with the way that Groovy == aliases .equals for all objects except for Comparables - == means compareTo when used with Comparables. (I don't know much about Groovy, so this was a big surprise for me; if you're in that boat, see this issue for more information.) More specifically, it seems to me that EqualArgumentConstraint's call to GroovyRuntimeUtil.equals ends up calling the same logic that Groovy uses when it sees a ==. Generally speaking, I think that's intuitive, but in this case you end up with broken-looking default behavior.

I have a commit that fixes this and will make a PR soon. I ended up adding another DefaultInteraction to DefaultJavaLangObjectInteractions that makes compareTo compare System.identityHashcode for any mocks that are instanceof Comparable. That felt cleaner than adding more complexity to EqualArgumentConstraint in order to handle the special case where expected is a mock of a Comparable.

Link to a gist or similar (optional)

PR to follow with a test that reproduces the bug.

Additional Environment information

I used IntelliJ IDEA on macOS. My global Java version is openjdk 1.8.0_232. The versions of the other tools are whatever the Spock project defaults to after a ./gradlew build and ./gradlew cleanIdea idea run - I don't think I have them installed globally.

Java/JDK

java -version

openjdk version "1.8.0_232"
OpenJDK Runtime Environment (AdoptOpenJDK)(build 1.8.0_232-b09)
OpenJDK 64-Bit Server VM (AdoptOpenJDK)(build 25.232-b09, mixed mode)

Groovy version

groovy -version
I don't have groovy installed globally.

Build tool version

Gradle

gradle -version

$ ./gradlew -version
 
------------------------------------------------------------
Gradle 6.8.3
------------------------------------------------------------
 
Build time:   2021-02-22 16:13:28 UTC
Revision:     9e26b4a9ebb910eaa1b8da8ff8575e514bc61c78
 
Kotlin:       1.4.20
Groovy:       2.5.12
Ant:          Apache Ant(TM) version 1.10.9 compiled on September 27 2020
JVM:          1.8.0_232 (AdoptOpenJDK 25.232-b09)
OS:           Mac OS X 10.15.2 x86_64

Apache Maven

mvn -version
I don't have this installed globally.

Operating System

Linux, Windows, Mac etc.
Mac

IDE

IntelliJ, Eclipse etc.
IntelliJ

Build-tool dependencies used

Whatever's in this project's build.gradle? I don't use Gradle for work, so I don't really know how to fill out this section.

[leonard84]

The contract of Comparable is that if the result of compareTo is 0 then equals should also be true and vice-versa.
Spocks logic for a plain argument is to use equals instead of identity, as this is what you want most of the time. If you explicitly need the identity, then you need to write it as code argument constraint {it === qux} (groovy 3) { qux.is(it) } (groovy 2).

The underlying problem is that you've used a mock without also mocking the method you are using, so by default it returns 0 or null. The change you are proposing would break the code of people who rely on that behavior.

If you add 1 * qux.compareTo(_) >> 1 the test will also pass.

[jdpopkin]

The contract of Comparable is that if the result of compareTo is 0 then equals should also be true and vice-versa.

This is "strongly recommended" but not a requirement. For example, BigDecimal has a compareTo that's inconsistent with its equals method (equals checks for matching scale, compareTo does not). That's the reason why Groovy uses compareTo instead of equals when == is used on Comparables - see this comment.

Anyway, in Spock's current form, Mocks of Comparables do not meet the requirement you're describing - if foo and bar are both Mock(Comparable), foo.compareTo(bar) will return 0 (because compareTo's behavior was never explicitly mocked) and foo.equals(bar) will return false (because DefaultEqualsInteraction intercepts .equals and checks system-identity). This is what my pull request changes (by adding a DefaultCompareToInteraction that behaves equivalently to DefaultEqualsInteraction).

Spocks logic for a plain argument is to use equals instead of identity, as this is what you want most of the time.

That's what the documentation says, but in practice that's not what Spock is doing. EqualArgumentConstraint actually calls DefaultTypeTransformation.compareEqual here. For most objects, that's going to resolve to a call to .equals, but not for Comparables. If Spock actually were using equals here, DefaultEqualsInteraction would save us, and the test would pass.

The underlying problem is that you've used a mock without also mocking the method you are using, so by default it returns 0 or null.

I'm not "using" compareTo within my test other than implicitly in the sense that Spock is actually calling compareTo instead of equals to decide whether or not baz and qux are the same object. I think there are good reasons for Spock to do that that (at least, messing with EqualArgumentConstraint to explicitly call .equals led to a stack overflow when I tested it). So rather than messing with EqualArgumentConstraint to special-case out Mock(Comparable), I just added default behavior for compareTo in new Mocks to make it consistent with equals.

The change you are proposing would break the code of people who rely on that behavior.

Yeah, that's the painful tradeoff here. I think that the current behavior of Mock(Comparable) is pretty bad - as-is, there's no difference between 1 * foo.bar(baz) and 1 * foo.bar(_ as Comparable) if baz is Comparable, because EqualArgumentConstraint cannot distinguish between different Mock(Comparable)s - and Spock does stub out some common-sense behavior for a couple of other methods (like equals), so I think there's some precedent for providing default behavior for compareTo. But it's definitely possible that there are people who rely on compareTo always returning 0 when called on a Mock.

I haven't looked too deeply into the possibility of messing with EqualArgumentConstraint itself, but maybe that's what needs to be done?

[leonard84]

Anyway, in Spock's current form, Mocks of Comparables do not meet the requirement you're describing

Yes, mocks don't really adhere to any contract unless you tell them to. You actually will get a NPE if you replace Comparable by List as the equality check will try to iterate over the lists to to compare them, and then get null when calling iterator(). And I'm sure there are more cases which will lead to errors. The question always is, how far can we sensibly support special cases and how confused are the users, when this special handling goes against their expectations.

Spocks logic for a plain argument is to use equals instead of identity, as this is what you want most of the time.

That's what the documentation says, but in practice that's not what Spock is doing. EqualArgumentConstraint actually calls DefaultTypeTransformation.compareEqual here. For most objects, that's going to resolve to a call to .equals, but not for Comparables. If Spock actually were using equals here, DefaultEqualsInteraction would save us, and the test would pass.

Let me rephrase, Spocks EqualArgumentConstraint behaves consistent to you having written {it == other}.

The PR #1323 would change the default behavior of all mocks, not just in the EqualArgumentConstraint case to treat compareTo differently, with arbitrary results as Integer.compare(System.identityHashCode(invocation.getMockObject().getInstance()), System.identityHashCode(invocation.getArguments().get(0))) will result in different results between runs.

Any thoughts @spockframework/supporter ?

[kiview]

While I understand the problem with the provided test case, I fail to imagine the real-world examples, in which this becomes an issue and this might be a smell with regards to test design. However, of course, Spock should adhere to some kind of reasonable contract.

Since this behaviour is reported upstream and consistent with Groovy (and it does not look like it will change there), I don't think Spock should diverge from this. Maybe one could clarify in the docs, that Spock relies on the GroovyRuntimeUtil.equals logic and adheres to its contract.

[leonard84]

I checked the documentation and I think it already clearly states:

The equality constraint uses groovy equality to check the argument, i.e, argument == constraint.

We could add a note for the Comparable behavior.

[erdi]

FWIW, I've just hit this issue. Good to see it fixed in 2.3!