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

[robfletcher]

Originally reported on Google Code with ID 105

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.

Reported by mail@dockter.biz on 2010-07-13 07:00:33

[robfletcher]

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.

Reported by pniederw on 2010-07-13 12:43:27

[robfletcher]

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

Reported by mail@dockter.biz on 2010-07-13 13:49:01

[robfletcher]

>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.

Reported by pniederw on 2010-07-13 14:00:37

• Status changed: Accepted

[robfletcher]

> 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.

Reported by pniederw on 2010-07-30 00:21:23

• Labels added: Milestone-0.5

[robfletcher]

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.

Reported by mail@dockter.biz on 2010-08-02 07:45:13

[robfletcher]

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.

Reported by pniederw on 2010-08-17 14:17:50

[robfletcher]

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.

Reported by mail@dockter.biz on 2010-08-17 14:27:58

[robfletcher]

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.

Reported by pniederw on 2010-10-30 22:20:01

• Status changed: Fixed