New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Spying on externally created objects doesn't handle internal calls of stubbed methods #771

Closed
szpak opened this Issue Sep 14, 2017 · 7 comments

Comments

Projects
None yet
2 participants
@szpak
Contributor

szpak commented Sep 14, 2017

Issue description

Spying on externally created objects don't handle internal calls (from withing the object) of stubbed methods. That works for spies created with Spy(Clazz) and GroovySpy(Clazz).

With clean design it should not be needed, but working with legacy code (or fighting with the limitations of some frameworks - such as Gradle) is can be handy.

How to reproduce

class FooSpec extends Specification {

    def "should see stubbed methods from within"() {
        given:
//            Self s = Spy()  //works fine
            Self s = Spy(new Self())  //2 is returned
            s.callMeBaby() >> 3
        expect:
            s.go() == 3
    }

    static class Self {

        int go() {
            return callMeBaby()
        }

        int callMeBaby() {
            return 2
        }
    }

Additional Environment information

Spock 1.1-groovy-2.4 and 1.2-groovy-2.4-SNAPSHOT (from https://github.com/leonard84/spock/tree/fix-769)
Byte Buddy 1.7.5, Objenesis 2.6, Groovy 2.4.10, Java 1.8.0_144

@szpak szpak changed the title from Spying on externally created objects don't handle internal calls of stubbed methods to Spying on externally created objects doesn't handle internal calls of stubbed methods Sep 14, 2017

@leonard84

This comment has been minimized.

Show comment
Hide comment
@leonard84

leonard84 Sep 14, 2017

Member

@szpak this was the original reason why Spock didn't support spying on externally created objects, AFAIK it is not possible to do this, since the Spy is only wrapping the instance and can't intercept internal calls. However, this limitation should be added to the docs.

Member

leonard84 commented Sep 14, 2017

@szpak this was the original reason why Spock didn't support spying on externally created objects, AFAIK it is not possible to do this, since the Spy is only wrapping the instance and can't intercept internal calls. However, this limitation should be added to the docs.

@szpak

This comment has been minimized.

Show comment
Hide comment
@szpak

szpak Sep 14, 2017

Contributor

At first I thought the same. It's common to pass internal calls directly with different types of proxy. However, later I realized if it works with Spy(Clazz) there should be no big difference how communication with the real object (even if created by Spock) is performed (unless Spock enhance somehow that real object to get know if the real method calls another real method internally). It works even in Mockito which (probably) doesn't modify a bytecode of a real objects passed to it to be spied (nonetheless, I didn't check the implementation).

Contributor

szpak commented Sep 14, 2017

At first I thought the same. It's common to pass internal calls directly with different types of proxy. However, later I realized if it works with Spy(Clazz) there should be no big difference how communication with the real object (even if created by Spock) is performed (unless Spock enhance somehow that real object to get know if the real method calls another real method internally). It works even in Mockito which (probably) doesn't modify a bytecode of a real objects passed to it to be spied (nonetheless, I didn't check the implementation).

@leonard84

This comment has been minimized.

Show comment
Hide comment
@leonard84

leonard84 Sep 14, 2017

Member

Actually mockito can't do it too, and it seems they don't really support spying on existing objects either.

https://github.com/mockito/mockito/blob/9e4de546d7ea2ec675e15d8b6960a3b8ffe324f7/src/main/java/org/mockito/internal/util/MockUtil.java#L38-L40

They just copy all the fields into the newly created spy. This means that any changes done to the state of the spy are not reflected in the real object and vice-versa.

public class SpyTest {
  @Test
  public void one() {
    Foo spy = Mockito.spy(new Foo());

    Assert.assertEquals(42, spy.bar());
    when(spy.foo()).thenReturn(1);
    Assert.assertEquals(1, spy.bar());
  }

  @Test
  public void two() {
    Foo realObject = new Foo();
    Foo spy = Mockito.spy(realObject);

    spy.setFoo(1);
    Assert.assertEquals(1, spy.bar());
    Assert.assertEquals(1, realObject.bar()); //fails
  }

  @Test
  public void three() {
    Foo realObject = new Foo();
    Foo spy = Mockito.spy(realObject);

    realObject.setFoo(1);
    Assert.assertEquals(1, realObject.bar());
    Assert.assertEquals(1, spy.bar()); //fails
  }

  public static class Foo {

    private int foo = 42;

    public int bar() {
      return foo();
    }

    private int foo() {
      return foo;
    }

    public void setFoo(int foo) {
      this.foo = foo;
    }
  }
}

I don't know which behavior is preferable since I see use cases for both, what do you think @twicksell ? Since, the main reason of using an already instantiated object is that you didn't have control over the creation process, you probably don't have control over everywhere the object is used. The question is whether you still need the changes to be reflected in both the spy and the real object.

Member

leonard84 commented Sep 14, 2017

Actually mockito can't do it too, and it seems they don't really support spying on existing objects either.

https://github.com/mockito/mockito/blob/9e4de546d7ea2ec675e15d8b6960a3b8ffe324f7/src/main/java/org/mockito/internal/util/MockUtil.java#L38-L40

They just copy all the fields into the newly created spy. This means that any changes done to the state of the spy are not reflected in the real object and vice-versa.

public class SpyTest {
  @Test
  public void one() {
    Foo spy = Mockito.spy(new Foo());

    Assert.assertEquals(42, spy.bar());
    when(spy.foo()).thenReturn(1);
    Assert.assertEquals(1, spy.bar());
  }

  @Test
  public void two() {
    Foo realObject = new Foo();
    Foo spy = Mockito.spy(realObject);

    spy.setFoo(1);
    Assert.assertEquals(1, spy.bar());
    Assert.assertEquals(1, realObject.bar()); //fails
  }

  @Test
  public void three() {
    Foo realObject = new Foo();
    Foo spy = Mockito.spy(realObject);

    realObject.setFoo(1);
    Assert.assertEquals(1, realObject.bar());
    Assert.assertEquals(1, spy.bar()); //fails
  }

  public static class Foo {

    private int foo = 42;

    public int bar() {
      return foo();
    }

    private int foo() {
      return foo;
    }

    public void setFoo(int foo) {
      this.foo = foo;
    }
  }
}

I don't know which behavior is preferable since I see use cases for both, what do you think @twicksell ? Since, the main reason of using an already instantiated object is that you didn't have control over the creation process, you probably don't have control over everywhere the object is used. The question is whether you still need the changes to be reflected in both the spy and the real object.

@szpak

This comment has been minimized.

Show comment
Hide comment
@szpak

szpak Sep 15, 2017

Contributor

Yes, Mockito forbids an usage of the original object as it' not connected. In my original report I referred to the use case where you only use a spy instead of the original object.

I switched to Spock from Mockito and therefore that approach is intuitive (natural) for me (also for interactions verification - no side calls). It's also rather easier in implementation, more powerful and it's enough in most of the cases. Of course in some other specific situations (where replacing the original object is hard) someone could opt for the other behavior...

Contributor

szpak commented Sep 15, 2017

Yes, Mockito forbids an usage of the original object as it' not connected. In my original report I referred to the use case where you only use a spy instead of the original object.

I switched to Spock from Mockito and therefore that approach is intuitive (natural) for me (also for interactions verification - no side calls). It's also rather easier in implementation, more powerful and it's enough in most of the cases. Of course in some other specific situations (where replacing the original object is hard) someone could opt for the other behavior...

@leonard84

This comment has been minimized.

Show comment
Hide comment
@leonard84

leonard84 Sep 15, 2017

Member

@szpak do you want to provide a PR for this behavior. -> Don't wrap the original object, just copy all fields.

Member

leonard84 commented Sep 15, 2017

@szpak do you want to provide a PR for this behavior. -> Don't wrap the original object, just copy all fields.

@leonard84

This comment has been minimized.

Show comment
Hide comment
@leonard84

leonard84 Dec 25, 2017

Member

I've gone ahead and implemented it in #800 would you like to review it @szpak

Member

leonard84 commented Dec 25, 2017

I've gone ahead and implemented it in #800 would you like to review it @szpak

@szpak

This comment has been minimized.

Show comment
Hide comment
@szpak

szpak Dec 26, 2017

Contributor

Great. LGTM. I had just a few minor comments.

Thanks @leonard84 for keeping the Spock development alive!

Contributor

szpak commented Dec 26, 2017

Great. LGTM. I had just a few minor comments.

Thanks @leonard84 for keeping the Spock development alive!

@leonard84 leonard84 closed this in #800 Dec 31, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment