Added configurable interaction matching

[BoukeNijhuis]

By default Spock uses a match first algorithm to determine the defined return value of a method. So whenever there are multiple defined return values, it will pick the first (that is not exhausted). This change enables a user to change this to a match last algorithm. So Spock will pick the last defined return value (that is not exhausted). This enables easy overriding of default return values. As requested in issue #26, #251, #321 and #962.

This is a first attempt and will need some work. First I would like to know if you (the maintainers) are interested is this kind of configurable behaviour for interaction matching. If so, I think we should at least discuss the following topics:

• what to do with interactions specified in the then block having preference over the existing ones?
• documentation

[codecov]

Codecov Report

Merging #1219 into master will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master    #1219      +/-   ##
============================================
+ Coverage     76.28%   76.30%   +0.02%     
- Complexity     3678     3684       +6     
============================================
  Files           396      396              
  Lines         11265    11269       +4     
  Branches       1382     1384       +2     
============================================
+ Hits           8593     8599       +6     
+ Misses         2193     2191       -2     
  Partials        479      479              

Impacted Files	Coverage Δ	Complexity Δ
.../spockframework/mock/runtime/InteractionScope.java	100.00% <100.00%> (ø)	18.00 <9.00> (+4.00)
...rc/main/java/spock/config/RunnerConfiguration.java	100.00% <100.00%> (ø)	1.00 <0.00> (ø)
...in/java/org/spockframework/runtime/RunContext.java	96.51% <0.00%> (+1.16%)	20.00% <0.00%> (+1.00%)
...amework/mock/runtime/MockInteractionDecorator.java	66.66% <0.00%> (+6.66%)	8.00% <0.00%> (+1.00%)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2802f88...f591abb. Read the comment docs.

[leonard84]

Hi, thanks for the initiative, but I don't think that this is the right approach.
As effect is global it will switch the behavior for all mocks, which will be quite confusing to users.
Furthermore, with the new parallel execution support, those changes would affect totally unrelated specs as well.
While, that global issue could be somewhat mitigated by using the specification context instead of a global value the issue of affecting all mocks in a spec remains.
And it won't solve the problem discussed here #321 (comment)

[BoukeNijhuis]

Thanks for your reply.

The global effect is intended. I think it would be really confusing if it would be tied to a specification (context). 😊

Regarding the problem discussed in #321. What I read there is that people really would like to override default mock responses in another place then the then: block. Currently if you do this, it is just ignored. I think lots of people would like the ability to override everywhere (especially in the given: or when: block). So the PR tries to enable overriding everywhere.

In #321 you show that one cannot override '1 * get(0) >> 1' with '1 * get(0) >> 2'. In my opinion nobody is asking for this. People just want to be able to override mock response. In the mentioned issue they respectfully care less about the '1 *' part. They mostly care about the '>> 2' part. This is how I interpreted the request/problem.

So I think we disagree about two thing:

• what is requested in the mentioned issues
• should the addressed problem be changed locally or globally

Let's start with the first. Is my interpretation of the request in the issues correct (the ability to override mock responses everywhere)? If not, what do you think the interpretation should be?

[leonard84]

First, this is how spock defines the terms:

• stubbing: Stubbing is the act of making collaborators respond to method calls in a certain way. When stubbing a method, you don’t care if and how many times the method is going to be called; you just want it to return some value, or perform some side effect, whenever it gets called. For example subscriber.receive(_) >> "ok"
• mocking: Mocking is the act of describing (mandatory) interactions between the object under specification and its collaborators. For example 1 * subscriber.receive("hello")

Those can be combined 1 * subscriber.receive("hello") >> "ok"

Now to the implications of this change, what you want is that a later defined stubbing would be used instead of the one defined before. The problem I see is that this also affects combined mocking/stubbing, like in this test. This would break if we set matchFirstInteraction = false.

    given:
    def list = Mock(List)

    when:
    def result = list.get(42)
    def result2 = list.get(42)

    then:
    1 * list.get(42) >> "foo"
    1 * list.get(42) >> "bar"

    and:
    result == "foo"
    result2 == "bar"

[BoukeNijhuis]

Thanks for your elaboration and sorry for using the wrong terms. I will try to use the correct ones in the future.

The example that you provide (with matchFirstInteraction = false) should fail. It will match the last interaction first (so the one that returns "bar"). If one would switch the statements in the when: or then: block the test will succeed. For me this is logical, because we use the match last algorithm.

Do you expect your example test to succeed? If yes, why?

[leonard84]

Thanks for your elaboration and sorry for using the wrong terms. I will try to use the correct ones in the future.

It was not intended as a rebuke, just to clarify the terms to make communicating easier.

The goal of #321 is that you can override the stubbing, the way this PR achieves that, has side effects that also affect normal mocking. As the inverted interaction matching is not obvious when looking at the test I think this violates the principle of least surprise. For someone looking at the test it doesn't do what you expect it to do.

[BoukeNijhuis]

Is it a bad thing that it also affects mocking? I like the consistency in that.

Regarding the principle of least surprise, I see your point there. What would be a better way to implement it?

[leonard84]

We could open a new interaction scope at the start of the method, this way you could define some stubbing in the given that should override the one from the setup, however, if you tried to override it further down again you won't be able to. So this could also be cause for confusion.

The most reliable way would be to just extend the docs with this, and it would also be consistent with how mocks work.

class OverrideStubbing extends Specification{
  List stub = Stub() {
    get(_) >> 1
  }

  def "normal"() {
    expect:
    stub.get(0) == 1
  }

  def "override"() {
    when:
    def result = stub.get(0)

    then:
    stub.get(_) >> 42
    result == 42
  }
}

[BoukeNijhuis]

Your proposal to extend the docs is a solution, but it does not solve the request to be able to override everywhere. So this solution is not the solution I am looking for (or the people in the mentioned issues).

I am willing to spend time to implement the override everywhere, but then we have to find a solution that is acceptable for you. So how would you like to implement this?

[Vampire]

Could maybe the interaction block be extended for doing it explicitly on a case by case basis? Something like

interaction(priority: 5) {
   1 * subscriber.receive(message)
}

or

interaction(overwrite: true) {
   1 * subscriber.receive(message)
}

or something like that?

[leonard84]

but it does not solve the request to be able to override everywhere.

@BoukeNijhuis could you give more examples of where you'd actually like to override this, that couldn't be solved by using the then block override?

The main challenge to a solution IMO is that Spock doesn't add the interactions to a mock, so you can't just reset the mock like you can do in mockito. Instead the interactions are added to the Specifications MockController which in turn has a list of InteractionScope to order them.

To illustrate

class AClass extends Specification {
  List stub = Stub() {
   get(_) >> 1                                            //1
  }

  def "atest"() {
    given:
    stub.get(_) >> 2                                      //2
    
    when:
    def result = stub.get(42)
    
    then:
    result == 3
    _.get(_) >> 3                                         //3
  }
}

produces

@org.spockframework.runtime.model.SpecMetadata(filename = 'script1603882731842.groovy', line = 3)
public class AClass extends spock.lang.Specification { 

    @org.spockframework.runtime.model.FieldMetadata(name = 'stub', ordinal = 0, line = 4, initializer = true)
    private java.util.List stub 

    private java.lang.Object $spock_initializeFields() {
        stub = this.StubImpl('stub', java.util.List, { 
            this.getSpecificationContext().getMockController().addInteraction(new org.spockframework.mock.runtime.InteractionBuilder(5, 4, 'get(_) >> 1').addEqualTarget(it).addEqualMethodName('get').setArgListKind(true, false).addEqualArg(_).addConstantResponse(1).build())
        })
    }

    @org.spockframework.runtime.model.FeatureMetadata(name = 'atest', ordinal = 0, line = 8, blocks = [org.spockframework.runtime.model.BlockKind.SETUP[]org.codehaus.groovy.ast.AnnotationNode@d9f41, org.spockframework.runtime.model.BlockKind.WHEN[]org.codehaus.groovy.ast.AnnotationNode@d9f41, org.spockframework.runtime.model.BlockKind.THEN[]org.codehaus.groovy.ast.AnnotationNode@d9f41], parameterNames = [])
    public void $spock_feature_0_0() {
        org.spockframework.runtime.ErrorCollector $spock_errorCollector = new org.spockframework.runtime.ErrorCollector(false)
        org.spockframework.runtime.ValueRecorder $spock_valueRecorder = new org.spockframework.runtime.ValueRecorder()
        try {
            this.getSpecificationContext().getMockController().addInteraction(new org.spockframework.mock.runtime.InteractionBuilder(10, 5, 'stub.get(_) >> 2').addEqualTarget(stub).addEqualMethodName('get').setArgListKind(true, false).addEqualArg(_).addConstantResponse(2).build())
            this.getSpecificationContext().getMockController().enterScope()
            this.getSpecificationContext().getMockController().addInteraction(new org.spockframework.mock.runtime.InteractionBuilder(17, 5, '_.get(_) >> 3').addEqualTarget(_).addEqualMethodName('get').setArgListKind(true, false).addEqualArg(_).addConstantResponse(3).build())
            java.lang.Object result = stub.get(42)
            this.getSpecificationContext().getMockController().leaveScope()
            try {
                org.spockframework.runtime.SpockRuntime.verifyCondition($spock_errorCollector, $spock_valueRecorder.reset(), 'result == 3', 16, 5, null, $spock_valueRecorder.record($spock_valueRecorder.startRecordingValue(2), $spock_valueRecorder.record($spock_valueRecorder.startRecordingValue(0), result) == $spock_valueRecorder.record($spock_valueRecorder.startRecordingValue(1), 3)))
            } 
            catch (java.lang.Throwable throwable) {
                org.spockframework.runtime.SpockRuntime.conditionFailedWithException($spock_errorCollector, $spock_valueRecorder, 'result == 3', 16, 5, null, throwable)} 
            finally { 
            } 
            this.getSpecificationContext().getMockController().leaveScope()
        } 
        finally { 
            $spock_errorCollector.validateCollectedErrors()} 
    }
}

One Scope is implicitly created at the start of the feature, the interactions 1 and 2 are added to it, then the when-then blocks create another scope, and 3 is added to it. Inner scopes take priority over the outer scopes, so the wildcard wins in the end. If you'd remove 3 then you'd get 1 answer no matter how often you'd call the stub, since stubs interactions are unbounded and thus never exhaust. I also used 3 to show that you can have wildcard/regex targets that would be quite difficult to replace.

[BoukeNijhuis]

Personally I would like to override in the setup and when block. This is where I specify my stub responses. In the then block I verify the actual outcome with the expected outcome. Therefore it is unnatural for me to override a stub response in the then block.

To answer your question: I think everything can be solved by overriding in the then block. But I prefer to do it somewhere else.

Then regarding the scopes and adding of interactions. I think I understand the current mechanism. I propose another mechanism that is more in line with Mockito's style. This new mechanism would enable the override everywhere request. The new mechanism would not add to a list of interactions, but it would just have one interaction which is overridable. By using the new mechanism you lose the ability to use example 3 (wildcard/regex targets). The current mechanism would be the default mechanism, but a user can choose to use the proposed mechanism.