Mocking in given is ignored when mocked in setup method

[AndreasTu]

The Interactions now allow to override other interactions, if these are the same and do not specify a cardinality.

This can possibly be a breaking change, when someone relies on the existing strange behavior, but I think this can be neglected.
None of the existing spock test in this repo broke.

This fixes #962

[codecov]

Codecov Report

Patch coverage: 91.95% and project coverage change: +0.18% 🎉

Comparison is base (e3ec836) 79.76% compared to head (250e4df) 79.94%.

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #1762      +/-   ##
============================================
+ Coverage     79.76%   79.94%   +0.18%     
- Complexity     4054     4114      +60     
============================================
  Files           425      425              
  Lines         12886    12968      +82     
  Branches       1622     1633      +11     
============================================
+ Hits          10278    10367      +89     
+ Misses         1999     1986      -13     
- Partials        609      615       +6     

Files Changed	Coverage Δ
...rk/mock/constraint/WildcardArgumentConstraint.java	50.00% <0.00%> (-16.67%)	⬇️
.../mock/constraint/WildcardMethodNameConstraint.java	50.00% <0.00%> (-16.67%)	⬇️
...ockframework/mock/constraint/TargetConstraint.java	84.21% <50.00%> (-9.13%)	⬇️
...k/mock/constraint/NamedArgumentListConstraint.java	88.63% <60.00%> (-3.68%)	⬇️
...mework/mock/constraint/TypeArgumentConstraint.java	80.00% <80.00%> (ø)
...va/org/spockframework/mock/DefaultInteraction.java	100.00% <100.00%> (+88.88%)	⬆️
...mework/mock/constraint/CodeArgumentConstraint.java	81.25% <100.00%> (+6.25%)	⬆️
...ework/mock/constraint/EqualArgumentConstraint.java	100.00% <100.00%> (ø)
...ork/mock/constraint/EqualMethodNameConstraint.java	100.00% <100.00%> (ø)
...k/mock/constraint/EqualPropertyNameConstraint.java	100.00% <100.00%> (ø)
... and 10 more

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Vampire]

I'm not so sure this simply can be "neglegted". If I got this right, it is a quite deep and subtle change. You for example currently weaken the rule that order of interactions is significant.

If you for example have

class Foo {
  String foo(String foo) { null }
  String bar(String bar) { null }
}

and

Foo foo = Mock {
  _(_ as String) >> 'bar'
  foo(_) >> 'foo'
  _(_ as String) >> 'baz'
}

then in the old logic foo and bar always return 'bar', as that interaction comes first and the other two interactions are never hit.

With the new logic foo and bar always return 'baz', even though that interaction is the last one.

So I guess if we do such a change, at least the order should also adapt, so that the former interaction would be removed and not replaced by the override.

[AndreasTu]

Very good point, thanks! How about we change the override logic, that it only overrides, if the interaction originated from another scope/block. Similar to override methods in languages.
So your sample defines it in the same scope, so no override applies, but if you define stuff in setup, but redefine it in given, it would be overridden, if it has no cardinality.

[Vampire]

To be honest, I'm totally unsure how to properly do it in a better and intuitive way.
I'm mainly saying this needs really thorough thoughts and many thinking to maybe do it "the right" way this time, or at least in a better / more intuitive way.

What would for example happen if you have

  class Foo {
    String foo(String foo) { null }
    String bar(String bar) { null }
  }

  Foo foo

  def setup() {
    foo = Mock {
      _(_ as String) >> 'bar'
      foo(_) >> 'foo'
    }
  }

  def foo() {
    with(foo) {
      foo(_) >> 'baz'
      _(_ as String) >> 'bam'
    }

    //...
  }

?
Would all methods return 'bam' because setup() defines the order and foo() just overrides the return values?
Or does foo() also do a "reordering"?
What if in the upper or lower block there are more, how will it work in a good way?
...?

I also always wished given could override setup() mocks, and I think I just generally don't do it but either copy things or use helper methods or similar.

But the more I think about it due to this PR, the more I consider whether the current way is fine.
Even if it feels a bit awkward at times, it is at least simple, once you got the grasp.
Interactions are looked at in order of definition (with then ones taking precedence over others) and the first one that matches and is not exhausted by cardinality is used.
Once you have that at heart, it is usually easy to understand what is going on and why and how to write it the way you want. :-)

[leonard84]

This is a breaking change, as I discussed in #321 we already have a way to override stubs.

I agree, that is is not ideal, as most users try to override stubs in the given blocks initially until they learn how to do it "correctly", but it has been this way for a long time and changing it could break tests for a lot of people, in hard to debug ways.

In all the time the both issues "only" gathered 14 +1 votes. So, while there is some demand, it is not overwhelmingly high.

None of the existing spock test in this repo broke.

That is a good first indicator, but our testsuite is not exhaustive, and we maybe should add a test.

Very good point, thanks! How about we change the override logic, that it only overrides, if the interaction originated from another scope/block.

That is basically what happens if you add interactions to the then block, as it opens another interaction scope, and interactions in the top scope have priority. I though about opening an interaction scope on entering the method as well, this would allow to override actions in the given block, but only for interactions defined in the field/setup phase. However, I dismissed it, as it may be confusing and not necessarily better than what we currently have.

[AndreasTu]

Ok, shall I close that PR then?

Also we should probably close the ticket #962 then.

[AndreasTu]

Closed this PR, because it does not really solve the issue in a satisfying way.