0

I just put this test method together:

@Unroll
def 'super start edit should be called if cell is not empty'( boolean empty ){
    given:
    DueDateEditor editor = GroovySpy( DueDateEditor ){
        isEmpty() >> empty
    }

    when:
    editor.startEdit()

    then:
    if( empty){
        0 * editor.callSuperStartEdit()
    }
    else {
        1 * editor.callSuperStartEdit()
    }

    where:
    empty | _
    true  | _
    false | _
}

... it works OK in terms of the two tests passing... but when you make it fail it's very odd: the output if the parameter empty is false is

super start edit should be called if cell is not empty[1]

... and it is 0 if the parameter empty is true. Is this a bug?

mike rodent
  • 14,126
  • 11
  • 103
  • 157

2 Answers2

2

I am writing an additional answer because

  • there is a small bug in Tim's solutions with regard to the title (but still his answer is technically absolutely correct!),
  • you don't need a GroovySpy here, a simple Spy absolutely suffices,
  • I want to show you an alternative way of testing without stubbing isEmpty(),
  • I want to show you how you can use just one interaction with the number of calls in a ternary expression instead of an if-else (even though error reporting is ugly then),
  • I want to comment on your way of testing in general (see the end of this post).
package de.scrum_master.stackoverflow.q61032514;

import java.time.LocalDate;

public class DueDateEditor {
  String text;

  public boolean isEmpty() {
    return text == null || text.trim() == "";
  }

  public void startEdit() {
    if (!isEmpty())
      callSuperStartEdit();
  }

  public void callSuperStartEdit() {}
}
package de.scrum_master.stackoverflow.q61032514

import spock.lang.Specification
import spock.lang.Unroll

class DueDateEditorTest extends Specification {
  @Unroll
  def 'super start edit #shouldMsg be called if the cell is #cellStateMsg'() {
    given:
    DueDateEditor editor = Spy() {
      isEmpty() >> empty
    }

    when:
    editor.startEdit()

    then:
    (empty ? 0 : 1) * editor.callSuperStartEdit()

    where:
    empty << [true, false]
    shouldMsg = empty ? 'should not' : 'should'
    cellStateMsg = empty ? 'empty' : 'not empty'
  }

  @Unroll
  def "super start edit #shouldMsg be called if cell text is '#text'"() {
    given:
    DueDateEditor editor = Spy()
    editor.text = text

    when:
    editor.startEdit()

    then:
    (editor.isEmpty() ? 0 : 1) * editor.callSuperStartEdit()
    // Or, if 'isEmpty()' has a side effect:
    // (text ? 1 : 0) * editor.callSuperStartEdit()

    where:
    text << ["foo", "", null, "line 1\nline 2"]
    shouldMsg = text ? 'should' : 'should not'
    cellStateMsg = text ? 'not empty' : 'empty'
  }
}

General remarks:

  • I would not test the internal wiring of a single class with interactions. The test will be brittle and if you refactor the class internally without changing the API at all, the test might break if the interactions are no longer as expected. I think this is over-specification and I would only use interaction testing for crucial interactions between different classes or maybe different instances of one class - "crucial" meaning things like the functionality of design patterns like Observer.
  • Having an if-else for differentiating two cases by two different interaction patterns if the whole test only knows exactly those two cases just makes the test less readable and more complex, see your own code as well as mine and Tim's. In such a case I would rather write two feature methods with simple titles and simple functionality, but without if-else or ternary expressions, without helper variables for titles etc.

P.S.: Sorry, I had to make up a sample class under test DueDateEditor in order to make my test compile and run as expected. As usual, Mike unfortunately didn't provide an MCVE but just a part of it.


Update: We talked about GroovySpy in our comments and, as I said, it will not work if your classes are Java classes and there is a final method in you want to stub, see the Spock manual. Here is proof for you:

package de.scrum_master.stackoverflow.q61032514;

public class TreeTableCell<A, B> {
  String text;

  public final boolean isEmpty() {
    return text == null || text.trim() == "";
  }
}
package de.scrum_master.stackoverflow.q61032514;

import java.time.LocalDate;

public class DueDateEditor extends TreeTableCell<String, LocalDate> {
  public void startEdit() {
    if (!isEmpty())
      callSuperStartEdit();
  }

  public void callSuperStartEdit() {}
}
package de.scrum_master.stackoverflow.q61032514

import spock.lang.Specification
import spock.lang.Unroll

class DueDateEditorTest extends Specification {
  @Unroll
  def 'super start edit #shouldMsg be called if the cell is #cellStateMsg'() {
    given:
    DueDateEditor editor = GroovySpy() {
      isEmpty() >> empty
    }

    when:
    editor.startEdit()

    then:
    (empty ? 0 : 1) * editor.callSuperStartEdit()

    where:
    empty << [true, false]
    shouldMsg = empty ? 'should not' : 'should'
    cellStateMsg = empty ? 'empty' : 'not empty'
  }
}

The test would work if your application classes were Groovy classes only. But if they are Java classes like in my example, the test will fail like this:

Too few invocations for:

(empty ? 0 : 1) * editor.callSuperStartEdit()   (0 invocations)

Unmatched invocations (ordered by similarity):

1 * editor.startEdit()
methodName == "callSuperStartEdit"
|          |
startEdit  false
           10 differences (44% similarity)
           (s---------)tartEdit
           (callSuperS)tartEdit

So in this case you cannot just use Groovy magic to check interactions. But as I said, you shouldn't do that anyway. Rather make sure that both startEdit() and callSuperStartEdit() do the right things. Check their results or, if they are void, check their side effects on the state of the subject under test or its collaborators.


Update 2: Regarding your original question about indexed method naming, actually @tim_yates gave the correct answer. I just want to add the corresponding Spock manual link explaining method unrolling and how you can influence naming using variables from the where: block.

kriegaex
  • 63,017
  • 15
  • 111
  • 202
  • Thanks. In fact `DueDateEditor` subclasses `TreeTableCell`, of which `isEmpty()` is `final`: the sole reason why I switched to `GroovySpy`. Once again I have carefully tried to understand (and learn from) your reasoning about why this is not a "good" test. As ever there is a tension (for me, at my low TDD level) between "write nothing without a test" and "don't overspecify". Sorry about not providing an MCVE, but in the case of this question I was really just puzzled by one thing: the output when using `Unroll`. – mike rodent Apr 05 '20 at 07:34
  • Mike, an MCVE always makes sense. It is necessary to run your code and IMO a matter of courtesy towards the people who want to help you. Make their life as easy as possible if you ask for help. As for `GroovySpy`, it will not work if `DueDateEditor` is a Java class, only if it is a Groovy class, because in Java a Groovy mock/spy behaves like a normal mock/spy, which is also documented. Even the Spock manual (not just me) discourages the use of Groovy mocks/spies in favour of refactoring, warning like "think twice before using this". IMO Groovy mocks are a code smell. – kriegaex Apr 05 '20 at 08:01
  • With regard to "not a good test" I think I explained enough in my answer. Yes, test everything, all your code. But test it the right way. Why do you think you need to test class-internal interactions? Test methods results or side effects instead. Why would it be essential to know that one method calls another one? I am sure you can test that the class does the right thing in another, better way. But as usual, you don't share your code under test, so I cannot explain in more detail. – kriegaex Apr 05 '20 at 08:07
  • Please note my answer updates regarding `GroovySpy` and method unrolling. – kriegaex Apr 05 '20 at 08:21
1

No it's not a bug

You didn't tell spock how to name your tests differently so it adds the iteration (0 then 1) to the name

Change it to

@Unroll
def 'super start edit should be called if isEmpty for the cell returns #empty'(){
    given:
    DueDateEditor editor = GroovySpy( DueDateEditor ){
        isEmpty() >> empty
    }

    when:
    editor.startEdit()

    then:
    if( empty){
        0 * editor.callSuperStartEdit()
    }
    else {
        1 * editor.callSuperStartEdit()
    }

    where:
    empty << [true, false]
}

I changed the where section, as a table with one column felt odd

edit

You can also do this:

@Unroll
def 'super start edit should be called if the cell is #msg'(){
    given:
    DueDateEditor editor = GroovySpy( DueDateEditor ){
        isEmpty() >> empty
    }

    when:
    editor.startEdit()

    then:
    if( empty){
        0 * editor.callSuperStartEdit()
    }
    else {
        1 * editor.callSuperStartEdit()
    }

    where:
    empty << [true, false]
    msg = empty ? 'empty' : 'not empty'
}
mike rodent
  • 14,126
  • 11
  • 103
  • 157
tim_yates
  • 167,322
  • 27
  • 342
  • 338
  • Thanks. Had no idea that you could do a `where` block like that. – mike rodent Apr 04 '20 at 20:03
  • Added an edit showing how you can make the titles more descriptive have fun! – tim_yates Apr 04 '20 at 20:07
  • Sorry for nitpicking about an otherwise correct answer, but both cases don't quite cut it because you need an extra variable for negation (should [not] be called) if you want to make the title not just descriptive but also correct. Otherwise in one of two cases the title is outright wrong. – kriegaex Apr 05 '20 at 02:58