spockframework
Interactions in when blocks are not preserved if the then block contains an interaction
Describe the bug
Interactions in a when: block are not active after the following then: block, if the then: block contains an interactions,
although the documentation states:
"Interactions declared outside a then: block are active from their declaration until the end of the
containing feature method."
The two features below describe the problem "When block interaction still active after when block without verification" "When block interactions are only active during the when block, if verification present"
The single verification line 1 * m.start() in the then: block, changes the behavior of the m.isStarted() method for the rest of the feature execution.
Internals:
If there is a interaction in the then: block, there is a second InteractionScope in the MockController and the interaction in the when: will be added to the top InteractionScope (the second scope from the then:), which will be deactivated by the leaveScope() of the then:. This will drop the interaction from the when: block.
So it does not reflect the behavior stated in the documentation.
Also see the discussion in #1728.
To Reproduce
def "When block interaction still active after when block without verification"() {
given:
def m = Mock(Engine)
when:
m.isStarted() >> true
def result = m.isStarted()
m.start()
then:
m.isStarted()
result
when: "Here the isStarted() still reflect the when block above"
result = m.isStarted()
then:
m.isStarted()
result
}
def "When block interactions are only active during the when block, if verification present"() {
given:
def m = Mock(Engine)
when:
m.isStarted() >> true
def result = m.isStarted()
m.start()
then:
//If you remove the next line the behavior of isStarted() is different
1 * m.start()
!m.isStarted()
result
when: "Now the isStarted() does not reflect the when block"
result = m.isStarted()
then:
!m.isStarted()
!result
}
static class Engine {
private boolean started
boolean isStarted() { return started }
void start() { started = true }
void stop() { started = false }
}
Expected behavior
The interactions in when block shall be preserved up until the end of the feature method.
Actual behavior
The when: block interactions are dropped after the when: block.
So I would expected that the m.isStarted() >> true holds up until the end of the feature, and should not be affected by the 1 * m.start() in the then: block.
Java version
17.0.8
Buildtool version
Gradle 8.1
Build time: 2023-04-12 12:07:45 UTC Revision: 40ba32cde9d6daf2b92c39376d2758909dd6b813
Kotlin: 1.8.10 Groovy: 3.0.15 Ant: Apache Ant(TM) version 1.10.11 compiled on July 10 2021 JVM: 17.0.8 (Amazon.com Inc. 17.0.8+7-LTS) OS: Windows 10 10.0 amd64
What operating system are you using
Windows
Dependencies
Spock-master
Additional context
No response
The question is, whether it makes actually sense to define interactions in the when block or whether we maybe should simply forbid that (and expect if it also works).
What is the use-case for defining an interaction in the when block?
They make sense as given / setup to prepare the test and they make sense in the then to assert something.
But the when block is for doing the stimulus of the SUT, so it might be more approriate to simply forbid this usage?
@Vampire for a clean test you are right, but I guess that there are tests out there which are not that clean.
If we forbid such a thing, that would break existing tests, which did not verify something in the then block.
Also you can't have multiple given blocks (which is a good thing),
so if someone wants to change the return value of the mock during a longer test with multiple when/then (I know not clean, but people do such stuff)
there would be no way to do that anymore, if we forbid it in when or you would duplicate the interactions in every then.
For me the fix shall make the behavior more alligned with the documentation and remove the current surprise, when you add a single unrelated interaction in the then block your when block interaction sematics change.
I would find the change to forbid interactions in when too invasive in regard to existing tests.
If we forbid such a thing, that would break existing tests, which did not verify something in the then block.
Well, we break existing tests either way.
Your changes are breaking too.
Just that they potentially break things that did do verifications in the then block.
If you for example had
def foo() {
given:
def mock = Mock(Object)
when:
mock.hashCode() >> 1
mock.toString()
def result = mock.hashCode()
then:
1 * mock.toString()
result == 1
when:
mock.hashCode() >> 2
mock.toString()
result = mock.hashCode()
then:
1 * mock.toString()
result == 2 // <=== fails with the suggested changes, worked before
}
then your suggestion would make the test suddenly fail as the hashCode would still return 1 in the second when block.
so if someone wants to change the return value of the mock during a longer test with multiple when/then (I know not clean, but people do such stuff) there would be no way to do that anymore, if we forbid it in when or you would duplicate the interactions in every then.
Not considering #1762 as imho the outcome for it is totally unclear right now, whether there will be any changes done at all and if so, which, Given that, it is currently already not possible to change the return value using an interaction. This will fail as in the second then the first interaction is still used, unless you use some interaction in the then block to "hack" an interaction reset:
def foo() {
given:
def mock = Mock(Object)
when:
mock.hashCode() >> 1
then:
mock.hashCode() == 1
when:
mock.hashCode() >> 2
then:
mock.hashCode() == 2 // <=== fails
}
so if someone wants to change the return value of the mock during a longer test with multiple when/then (I know not clean, but people do such stuff) there would be no way to do that anymore, if we forbid it in when or you would duplicate the interactions in every then.
(Yes, same quote again)
Actually it is not true that there would be no way, and is no way.
This is for example a proper way to change the return value of a mocked method during a lengthy test that already works now with or without having an interaction in then and would also work with interaction being forbidden in when:
def foo() {
given:
def mockedHashCode
def mock = Mock(Object) {
hashCode() >> { mockedHashCode }
}
when:
mockedHashCode = 1
def result = mock.hashCode()
then:
result == 1
when:
mockedHashCode = 2
result = mock.hashCode()
then:
result == 2
}
For me the fix shall make the behavior more alligned with the documentation and remove the current surprise, when you add a single unrelated interaction in the then block your when block interaction sematics change.
I fully agree that the current behaviour does not match the documentation. I fully agree that the current behaviour is surprising, non-intuitive, and bad. I just suggest an alternative that imho makes more sense. :-)
I would find the change to forbid interactions in when too invasive in regard to existing tests.
As I demonstrated, both changes are invasive and breaking changes.
We were never shy of doing breaking changes if they make sense, as long as they are properly documented.
So yes, I think we should change the current behavior and have a breaking change.
I just think that forbidding interactions in when blocks could make more sense.
I'm also curious what @leonard84 thinks about the topic.
For me the fix shall make the behavior more alligned with the documentation and remove the current surprise, when you add a single unrelated interaction in the then block your when block interaction sematics change.
I think we should fix the documentation instead of changing Spock's behavior. I touched on the why this happens in my answer here https://github.com/spockframework/spock/pull/1762#issuecomment-1686867237, TL;DR when-then blocks open an InteractionScope in the when which is closed at the beginning of the then block, all interactions are actually moved in front of the when block content.
@leonard84 @Vampire I am still not 100% convinced, because it is not consistent. If the then block has not interaction. your TL;DR is not the case, as the first test case above shows. The when-then only opens an interactionScope, if there was at least one then interaction.
So only a documentation fix, would not resolve this discrepancy.
As described above, I fully agree with @AndreasTu that the current behavior is very strange and unintuitive. And I also think that a documentation change for this one is not enough. (Just in case you confused it, #1762 is only lightly related to this one and not about the same issue)
I just don't agree with the suggested solution, but suggest to instead disallow defining interactions in when blocks.
Making the when block open a new interaction scope would be yet another way to mitigate the strange current behavior, but also that would be a breaking change.
I think simply we can't simply forbid adding interactions in when, as you can define new mock objects and interactions in other interactions. So, a simple adding a InteractionScope.freeze() and calling it before entering the when block or similar would break this. And this is also an example that we wouldn't want to leak these interactions from the when-then block.
import spock.lang.*
class ASpec extends Specification {
def "hello world"() {
given:
List a = Mock()
when:
def result = a.get(0).size()
then:
1 * a.get(_) >> {
Stub(List) {
size() >> 42
}
}
result == 42
}
}
We could disallow the writing of interaction instructions in the when block, but this won't work well if the instructions are moved to methods.
The when-then only opens an interactionScope, if there was at least one then interaction.
We could make it always open an interactions scope, even though there are no interactions defined in the then block. The downside of that is some unnecessary object instantiation for a lot of tests that don't deal with any mocks.
import spock.lang.*
class ASpec extends Specification {
def "hello world"() {
given:
List a = Mock()
List b = Stub {
size() >> 42
}
when:
def result = a.get(0).size()
then:
1 * a.get(_) >> b
result == 42
}
}
:-)
But yeah, I get the point, if you need to dynamically calculate the stub somehow it might get awkward with when not allowing to add new interactions and doing it at compile time is not really nice because of helper methods that could be called and call other helper methods. :-/
So yeah, forbidding is probably not a good solution :-(