spockframework / spock

The Enterprise-ready testing and specification framework.
https://spockframework.org
Apache License 2.0
3.54k stars 466 forks source link

GroovyMock/Stub constructors return null -> undocumented caveat #1159

Open kriegaex opened 4 years ago

kriegaex commented 4 years ago

Preface

Original content taken from my StackOverflow answer. I am copying both question and answer here for your convenience.

I would like to know if it is intended that for a global Groovy mock/stub constructor calls to the target class return null instead of a mock object. I find it counter-intuitive, it is completely undocumented and also has zero test coverage in the Spock source code.

Original StackOverflow question

I'm using the following test as an example to showcase a similar issue I'm seeing. I think it's just a misunderstanding on my part about how global mocks work in SpockFramework.

  void "test"() {
    when:
    TestStage stage = new TestStage("John")
    GroovyMock(TestStep.class, global: true) {
      getName() >> "Joe"
    }
    then:
    stage.run() == "Joe"
  }

This test should create a test stage supplying a default name. But then I create a global mock of a class inside of TestStage to override the return value. IE: I'm only trying to test the functionality of TestStage not TestStep. If TestStep were to make only changes, I don't want to know about them, I'll test those separately. However when I run this test, it looks like the global mock never takes effect as the returned name is still "John", which is what I supplied originally.

stage.run() == "Joe"
|     |     |
|     John  false

Here's the two sample classes used to test this.

class TestStage {
  TestStep step

  TestStage(String name) {
    this.step = new TestStep(name)
  }

  String run() {
    return step.getName()
  }
}
class TestStep {
    private String name

    TestStep(String name) {
      this.name = name
    }

    String getName() {
      return this.name
    }
}

Original StackOverflow answer

Actually, you are asking a good question here because according to the Spock manual it seems as if you could use GroovyMock and GroovyStub in order to globally replace instances and stub their methods as you tried to do, even though if I were you I would have created the global mock object first before implicitly using it in the constructor of the object depending on it. But anyway, it does not work as expected, like you said.

When I searched the Spock manual and the Spock source code for examples concerning GroovyMock, nowhere did I find a single one involving anything else than static methods. The test coverage there is quite bad, actually. Usually if the manual does not help me, I look if I can infer from the tests how to use a feature. In this case I had to try by myself.

The first thing I noticed is the completely counter-intuitive fact that when calling a constructor on a global GroovyMock or GroovyStub, it returns null!!! This is a real caveat. In a way constructors are treated like normal mock methods here, also returning null. Nowhere does any official source mention that and I also think it should be changed to default to returning a normal Spock mock instead (if the class is mockable, i.e. non-final).

Now this is also the key to the solution: You need to stub one or more constructors to return something else than null, e.g. a previously created normal instance or a Spock mock/stub/spy.

Here is a slightly altered version of your source code (I renamed the application classes so as not to contain "Test" in their names because all those "Test"s were a little confusing to me, especially because I also named my test class "Test", as I usually do instead of "Spec" because then Maven Surefire/Failsafe pick it up automatically without extra configuration.

I also added a static method to the class to be mocked in order to show you the syntax for stubbing that too. that is just a free add-on and not directly related to your question.

My test shows three variants:

package de.scrum_master.stackoverflow.q61667088

class Step {
  private String name

  Step(String name) {
    this.name = name
  }

  String getName() {
    return this.name
  }

  static String staticMethod() {
    return "original"
  }
}
package de.scrum_master.stackoverflow.q61667088

class Stage {
  Step step

  Stage(String name) {
    this.step = new Step(name)
  }

  String run() {
    return step.getName()
  }
}
package de.scrum_master.stackoverflow.q61667088

import spock.lang.Specification

class GlobalMockTest extends Specification {

  def "use Spock mock"() {
    given:
    def step = Mock(Step) {
      getName() >> "Joe"
    }
    def stage = new Stage("John")
    stage.step = step

    expect:
    stage.run() == "Joe"
  }

  def "use global GroovySpy"() {
    given:
    GroovySpy(Step, global: true) {
      getName() >> "Joe"
    }
    Step.staticMethod() >> "stubbed"
    def stage = new Stage("John")

    expect:
    Step.staticMethod() == "stubbed"
    stage.run() == "Joe"
  }

  def "use global GroovyMock"() {
    given:
    GroovyMock(Step, global: true)
    new Step(*_) >> Mock(Step) {
      getName() >> "Joe"
    }
    Step.staticMethod() >> "stubbed"
    def stage = new Stage("John")

    expect:
    Step.staticMethod() == "stubbed"
    stage.run() == "Joe"
  }

}

P.S.: I think you probably read the Spock manual, but just in case: If your GroovyMock/Stub/Spy target is implemented in a language other than Groovy such as Java or Kotlin, this will not work because then Groovy* will behave like a regular Spock mock/stub/spy.

Vampire commented 4 years ago

For a GroovyMock I would understand that null is returned as it works by default with ZeroOrNullResponse.

For a GroovyStub which uses EmptyOrDummyResponse by default I would have expected a "local mock" to be returned from the constructor, but I guess constructor has return type void which then makes it return null which is probably the main problem here.

For only stubbing selectively you can always use the GroovySpy as you also said in your answer.

If you wanted to stub everything but the constructor, I think you could either use a GroovyMock or GroovySpy and add an interaction like new Step(*_) >> { callRealMethod() }, or use an own IDefaultResponse, e. g. one that extends EmptyOrDummyResponse or ZeroOrNullResponse and in its respond method checks for constructor and then either calls the real method or forwards to the super method.

kriegaex commented 4 years ago

The normal use case for most people should be that they cannot directly inject a dependency into a class under test because there is no property, setter or constructor parameter for it. Then they expect that a dummy is automatically created upon each constructor call unless otherwise specified by overriding the constructor with something more specific. What the user intuitively expects when he writes something like this...

GroovyMock(FooType, global: true) {
  doSomething(_) >> "stub result"
}

... is that a Spock mock with one stubbed method is created globally, not that whenever a FooType is created the result is null. ZeroOrNullResponse for the constructor does not make any intuitive sense there as a default, only for the methods (both static and non-static).

Vampire commented 4 years ago

I don't really agree. I'm mostly user too and as user I expect from a Mock, that all mockable callables return null as documented.

@leonard84 & @szpak what are your thought about the points in here?

kriegaex commented 4 years ago

Usually when I create a mock or stub, the Mock() or similar call yields an instance, not null. It is completely counter-intuitive that I create something which defaults to null. What is the point, especially if it is undocumented. If at least the manual said: "Hey, you might think you create a so-called "global mock" but what you are creating by default is null, unless in addition to creating that global mock (a.k.a. global null) you also define a specific constructor result." Hm, I really am kind of an advanced Spock user (not an expert maybe), but that does strike me as very odd.

Vampire commented 4 years ago

Maybe you slightly misinterpret what a global mock is. The Mock call itself indeed returns a mock object, but not the one the SUT receives on constructor call. As documented the mock you receive is just for recording interactions and the interactions you record and the default responses work for all instances, constructor calls and static methods, whether the instances were created before the mock definition or not does not matter. A global mock does not mean, that only instances created afterwards are like local mock objects.

leonard84 commented 4 years ago

You actually get an instance of the mock when you call Step mock = GroovyMock(Step, global: true). And the contract for Mock is that you have to define every interaction that you want to have a non-null return value, just that it also includes the constructor for global mocks. That is also the reason why you only see GroovySpy(global:true) in spock-specs and only one example of GroovyMock(global: true) for testing mocking a static method, as the spy will correctly call the constructor.

This is also reflected in the docs

Since global mocks have a somewhat, well, global effect, it’s often convenient to use them together with GroovySpy. This leads to the real code getting executed unless an interaction matches, allowing you to selectively listen in on objects and change their behavior just where needed.

GroovyStub does not return null for constructor calls, but the return type of the constructor is Object so you get a stub for Object as well.

We may need to enhance EmptyOrDummeResponse to detect constructor calls if ("<init>".equals(invocation.getMethod().getName()) && invocation.getMethod().isStatic()) and return a properly typed stub there as well (would need to be derived from the IMockObject).

And it looks like part of the docs are wrong, I don't know if it ever worked this way or just broke in recent groovy versions with changes to the MOP:

def publisher = new Publisher()
publisher << new RealSubscriber() << new RealSubscriber()

RealSubscriber anySubscriber = GroovyMock(global: true)

when:
publisher.publish("message")

then:
2 * anySubscriber.receive("message")

Another reason why we need more code backed docs.

kriegaex commented 4 years ago

Sorry to be so stubborn, but @leonard84, this part of the documentation you just quoted is actually part of how I expect a global mock to work: I can define interactions on the mock directly and stub actions directly, but in fact I cannot. The example code just does not work, as you said. Why would the semantics of GroovyMock and GroovySpy suddenly become different for global: true when they are the same for Mock and Spy or for non-global GroovyMock and GroovySpy?

Just saying "use a global spy instead" is not helpful in all situations. If I want to mock and not to spy, the real-object-based spy just doesn't cut it. So if I want to achieve that today, I have to, as we say in German, shoot into the eye from behind through the breast:

package de.scrum_master.stackoverflow.q61667088

import spock.lang.Specification

class PublishSubscribeTest extends Specification {

  def "test publish - subscribe protocol with global Groovy mock"() {
    GroovyMock(Subscriber, global: true)
    def subscriber = Mock(Subscriber)
    new Subscriber() >> subscriber

    def publisher = new Publisher()
    publisher << new Subscriber() << new Subscriber()

    when:
    publisher.publish("message")

    then:
    2 * subscriber.receive("message")
  }

  static class Subscriber {
    void receive(String message) {
      println "received: $message"
    }
  }

  static class Publisher {
    List<Subscriber> subscribers = new ArrayList<>()

    Publisher leftShift(Subscriber subscriber) {
      subscribers << subscriber
      this
    }

    void publish(String message) {
      subscribers.each { it.receive(message) }
    }
  }

}

Now if that isn't ugly, I don't know what is. If later I decide to refactor for an integration test by changing the global mock to a global spy, all of a sudden I need to define the interactions on the spy directly again.

Also, for static methods I define interactions on the global mock. It would absolutely be possible to evolve the DSL and the AST transformations involved to a state in which a user could use global mocks and stubs in the intuitively expected way which is also advertised by the Spock manual. Now I have to build the global mock behaviour most users would expect by wiring a mock into a mock via stubbed constructor and also exposing a reference to the inner mock. Why not hide the complexity of stubbing constructors and inner mocks from the user and let him work on the global mock as expected? Right now this feels so un-spockish. If the user wants different behaviour like different mocks for different constructors, it is still nice to be able to stub constructors. But by default I want one global mock, hence the name.

kriegaex commented 3 years ago

This issue is still open. I just stumbled upon it, because someone upvoted my old StackOverflow answer and I did not quite remember what it was about in detail. Last year, when I had more free cycles for playing with mocks, I implemented my version of global mocks in Sarek, but with regard to Spock core this issue is still dangling in the air. I re-read it and still find my ideas valid. After my last post with example code, there was no more reaction, so @leonard84, do you think we can evolve Spock into something resembling my vision, being more user-friendly, intuitive and hiding complexity, while at the same time being closer to users' expectations? Otherwise, of course you can close the issue, but I would very much regret that.

leonard84 commented 3 years ago

It is all a matter of priority and resources, and as this is an issue that doesn't affect many users it isn't that high on my list, that doesn't mean it is not valid.