Disallow assignments in expect- and then-blocks (was: Interaction return argument fails when used with 'as' operator)

[GoogleCodeExporter]

The following statement:

dependencyMock.getDependencies(getTask()) >> [task1, task2] as Set

lead to:

java.lang.NullPointerException: Cannot invoke method rightShift() on null object
    at org.gradle.api.tasks.AbstractSpockTaskTest.testDependentTaskDidWork(AbstractSpockTaskTest.groovy:363)

If I put parens around the return argument everything works fine:

dependencyMock.getDependencies(getTask()) >> ([task1, task2] as Set)

I known that in this case I don't need the as Set. But it points to an issue.

Original issue reported on code.google.com by m...@dockter.biz on 13 Jul 2010 at 7:00

[GoogleCodeExporter]

This is the expected behavior. '>>' binds stronger than 'as', so the expression 
is parsed as:

(dependencyMock.getDependencies(getTask()) >> [task1, task2]) as Set

The part in parentheses is recognized as an interaction definition and replaced 
with:

mockController.addInteraction(...)

Since 'addInteraction' has return type void,  trying to coerce its result to 
Set results in an exception.

Do you think this needs some extra (error) handling?

>I known that in this case I don't need the as Set
Just to make sure we are on the same page: if the mocked method has a return 
type of Set, the List will be auto-coerced to a Set.

Original comment by pnied...@gmail.com on 13 Jul 2010 at 12:43

[GoogleCodeExporter]

Yes. In that case it would be auto-coerced. But we often have LinkedHashSet's 
as return argument. And people might also want to do frequently 'as File'.

The exception makes it hard though to figure it out. On the other hand it is a 
minor thing. Not sure whether it is worth to spend too much time on it. 

What happens that we get the current exception?: 
java.lang.NullPointerException: Cannot invoke method rightShift() on null object

Original comment by m...@dockter.biz on 13 Jul 2010 at 1:49

[GoogleCodeExporter]

>Yes. In that case it would be auto-coerced. But we often have LinkedHashSet's 
as return argument. And people might also want to do frequently 'as File'.

I see.

>What happens that we get the current exception?: 
java.lang.NullPointerException: Cannot invoke method rightShift() on null object

Good question. I had missed that rightShift() is called. Will have to look into 
this.

Original comment by pnied...@gmail.com on 13 Jul 2010 at 2:00

• Changed state: Accepted

[GoogleCodeExporter]

> What happens that we get the current exception?
Without parentheses, "as" is the outermost operator and hence the expression is 
no longer considered a mock expectation. When the expression is evaluated, 
getDependencies() returns null because it doesn't match any expectation, 
resulting in an NPE.

An equally hard to understand exception occurs if one forgets >> in the 
following expectation:
foo.bar(5) { "return value" }

The best solution I can think of right now is to mention these pitfalls in the 
documentation.

Original comment by pnied...@gmail.com on 30 Jul 2010 at 12:21

• Added labels: Milestone-0.5

[GoogleCodeExporter]

Thanks for the explanation. What we are considering for Gradle in regard to 
such pitfalls is to analyze the AST and issue warnings. My most frequent 
mistake with Spock is to write = instead of ==. I know that sometimes this is 
something you want to do in an expect block. But to have some checks that point 
out the places where I can validate whether this is something I really want to 
do would be cool.

Original comment by m...@dockter.biz on 2 Aug 2010 at 7:45

[GoogleCodeExporter]

I agree that writing = instead of == in an expect/then block is a common 
mistake. We are considering to disallow this in 0.5 (except together with a 
'def'). 
We could go further and only allow boolean expressions in expect/then block. Is 
this something you'd like to see? So far our position was that a Groovy-based 
testing framework should support Groovy truth, but it also has its drawbacks.

Original comment by pnied...@gmail.com on 17 Aug 2010 at 2:17

[GoogleCodeExporter]

I sometimes have logic in the then block. I guess this could always be moved 
somewhere else. So only allowing boolean expressions has its appeal.

Original comment by m...@dockter.biz on 17 Aug 2010 at 2:27

[GoogleCodeExporter]

Assignments in expect- and then-blocks are now flagged as compile errors 
(unless they are part of a variable declaration). If you would like to see 
warnings/errors for other common pitfalls. please raise new issues.

Original comment by pnied...@gmail.com on 30 Oct 2010 at 10:20

• Changed title: Disallow assignments in expect- and then-blocks (was: Interaction return argument fails when used with 'as' operator)
• Changed state: Fixed