Skip to content
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

AST transformation messes up setter/getter in dot-property notation #1158

Closed
kriegaex opened this issue May 6, 2020 · 10 comments · Fixed by #1225
Closed

AST transformation messes up setter/getter in dot-property notation #1158

kriegaex opened this issue May 6, 2020 · 10 comments · Fixed by #1225

Comments

@kriegaex
Copy link
Contributor

kriegaex commented May 6, 2020

I found this on StackOverflow. Here is my slightly modifed version of the sample code:

package de.scrum_master.stackoverflow.q61599143

import spock.lang.Specification

class PropertyAccessTest extends Specification {
  def 'test setter on interface'() {
    given:
    def foo = Mock(Foo)
    when:
    foo.prop = 'val'
    then:
    1 * foo.setProp(_)
  }

  def 'test getter on interface'() {
    given:
    def foo = Mock(Foo)
    when:
    foo.prop
    then:
    1 * foo.getProp()
  }

  def 'test setter on abstract'() {
    given:
    def foo = Mock(FooBase)
    when:
    foo.prop = 'val'
    //foo.setProp('val')
    then:
    1 * foo.setProp(_)
    //1 * foo.setProperty('prop', 'val')
  }

  def 'test getter on abstract'() {
    given:
    def foo = Mock(FooBase)
    when:
    foo.prop
    //foo.getProp()
    then:
    1 * foo.getProp()
    //1 * foo.getProperty('prop')
  }

  def 'test setter on concrete'() {
    given:
    def foo = Mock(FooImpl)
    when:
    foo.prop = 'val'
    //foo.setProp('val')
    then:
    1 * foo.setProp(_)
    //1 * foo.setProperty('prop', 'val')
  }

  def 'test getter on concrete'() {
    given:
    def foo = Mock(FooImpl)
    when:
    foo.prop
    //foo.getProp()
    then:
    1 * foo.getProp()
    //1 * foo.getProperty('prop')
  }

  static interface Foo {
    String getProp()
    void setProp(String val)
  }

  static abstract class FooBase implements Foo {
    abstract String getProp();
    abstract void setProp(String val);
  }

  static class FooImpl extends FooBase {
    private String prop
    String getProp() {
      println('Foo.getProp')
      return prop
    }

    void setProp(String val) {
      println('Foo.setProp')
      this.prop = val
    }

    static void main(String[] args) {
      def fooImpl = new FooImpl()
      fooImpl.prop = "test1"
      println fooImpl.prop
      fooImpl.setProp("test2")
      println fooImpl.prop
    }
  }
}

Running this test in Spock 1.3-groovy-2.5 yields 4 failed tests:

Too few invocations for:

1 * foo.setProp(_)   (0 invocations)

Unmatched invocations (ordered by similarity):

1 * foo.setProperty('prop', 'val')
methodName == "setProp"
|          |
|          false
|          4 differences (63% similarity)
|          setProp(erty)
|          setProp(----)
setProperty

<too many arguments>
	at org.spockframework.mock.runtime.InteractionScope.verifyInteractions(InteractionScope.java:98)
	at org.spockframework.mock.runtime.MockController.leaveScope(MockController.java:77)
	at de.scrum_master.stackoverflow.q61599143.PropertyAccessTest.test setter on abstract(PropertyAccessTest.groovy:28)


Too few invocations for:

1 * foo.getProp()   (0 invocations)

Unmatched invocations (ordered by similarity):

1 * foo.getProperty('prop')
methodName == "getProp"
|          |
|          false
|          4 differences (63% similarity)
|          getProp(erty)
|          getProp(----)
getProperty

<no args expected>
	at org.spockframework.mock.runtime.InteractionScope.verifyInteractions(InteractionScope.java:98)
	at org.spockframework.mock.runtime.MockController.leaveScope(MockController.java:77)
	at de.scrum_master.stackoverflow.q61599143.PropertyAccessTest.test getter on abstract(PropertyAccessTest.groovy:39)


Too few invocations for:

1 * foo.setProp(_)   (0 invocations)

Unmatched invocations (ordered by similarity):

1 * foo.setProperty('prop', 'val')
methodName == "setProp"
|          |
|          false
|          4 differences (63% similarity)
|          setProp(erty)
|          setProp(----)
setProperty

<too many arguments>
	at org.spockframework.mock.runtime.InteractionScope.verifyInteractions(InteractionScope.java:98)
	at org.spockframework.mock.runtime.MockController.leaveScope(MockController.java:77)
	at de.scrum_master.stackoverflow.q61599143.PropertyAccessTest.test setter on concrete(PropertyAccessTest.groovy:50)


Too few invocations for:

1 * foo.getProp()   (0 invocations)

Unmatched invocations (ordered by similarity):

1 * foo.getProperty('prop')
methodName == "getProp"
|          |
|          false
|          4 differences (63% similarity)
|          getProp(erty)
|          getProp(----)
getProperty

<no args expected>
	at org.spockframework.mock.runtime.InteractionScope.verifyInteractions(InteractionScope.java:98)
	at org.spockframework.mock.runtime.MockController.leaveScope(MockController.java:77)
	at de.scrum_master.stackoverflow.q61599143.PropertyAccessTest.test getter on concrete(PropertyAccessTest.groovy:61)

Running the exact same test on Spock 2.0-M2-groovy-3.0 yields only 2 failed tests:

Too few invocations for:

1 * foo.setProp(_)   (0 invocations)

Unmatched invocations (ordered by similarity):

1 * foo.setProperty('prop', 'val')
methodName == "setProp"
|          |
|          false
|          4 differences (63% similarity)
|          setProp(erty)
|          setProp(----)
setProperty

<too many arguments>
	at org.spockframework.mock.runtime.InteractionScope.verifyInteractions(InteractionScope.java:98)
	at org.spockframework.mock.runtime.MockController.leaveScope(MockController.java:77)
	at de.scrum_master.stackoverflow.q61599143.PropertyAccessTest.test setter on abstract(PropertyAccessTest.groovy:33)


Too few invocations for:

1 * foo.setProp(_)   (0 invocations)

Unmatched invocations (ordered by similarity):

1 * foo.setProperty('prop', 'val')
methodName == "setProp"
|          |
|          false
|          4 differences (63% similarity)
|          setProp(erty)
|          setProp(----)
setProperty

<too many arguments>
	at org.spockframework.mock.runtime.InteractionScope.verifyInteractions(InteractionScope.java:98)
	at org.spockframework.mock.runtime.MockController.leaveScope(MockController.java:77)
	at de.scrum_master.stackoverflow.q61599143.PropertyAccessTest.test setter on concrete(PropertyAccessTest.groovy:60)

Running the main method on both platforms shows that it is not a Groovy problem but probably related to a Spock AST transformation glitch:

Foo.setProp
Foo.getProp
test1
Foo.setProp
Foo.getProp
test2
@haridsv
Copy link

haridsv commented May 6, 2020

Thanks @kriegaex, appreciate you taking the time to confirm the bug in different versions as well as opening this bug.

@kriegaex
Copy link
Contributor Author

kriegaex commented May 6, 2020

Update: Actually I just noticed that if the application classes are implemented in Java, it works as expected with the property notation for setters/getters. The problem seems to be related to GroovyObjects only.

@szpak
Copy link
Member

szpak commented May 6, 2020

Thanks for your report with detailed use cases! It is a known reason I spotted working on Groovy 3 compatibility - #1076. There is a workaround (a hack) to handle common cases (for Groovy 3 - which explains that more tests pass), but as it is not a problem which hits the majority of people, I would wait until https://issues.apache.org/jira/browse/GROOVY-9369 is fixed on the Groovy side.

@kriegaex
Copy link
Contributor Author

kriegaex commented May 7, 2020

From the discussion in the other ticket and the ML it sounds as if this is purely a Groovy 3 problem, which it is not. On the contrary, more tests fail in Groovy 2 than in Groovy 3, but - as I said - only in Spock, not in pure Groovy. So to me it sounds as if even a future Groovy 3 fix will not help us on Spock 1.x with Groovy 2 or Spock 2.x with Groovy 2.

I am not an AST expert, but my gut feeling tells me that this ought to (and could) be fixed in Spock. Maybe it can be analysed and differentiated in an AST phase earlier than the one used at the moment, i.e. before the AST has been transformed to a point where you can no longer differentiate what the user wrote in the source code and which intention she has.

@szpak
Copy link
Member

szpak commented May 7, 2020

Please take a look at the following code and again at my description in the reported issue in Groovy. As the Groovy developers confirmed (on Slack) it could be a problem in 3.x and proposed to report the bug to take a look at it later on, I would prefer to avoid "additional hacks" to handle edge cases. However, I definitely could miss something, so if you have a good idea how to easily fix it (in a sensible way) on the Spock side please propose required changes.

@kriegaex
Copy link
Contributor Author

kriegaex commented May 7, 2020

Actually I am just a Spock user and not a Groovy pro, so I am afraid I cannot say anything intelligent about your code. But again the question: How can this be a Groovy 3 problem if it also happens in Groovy 2? My sample code and the provided logs prove it.

@szpak
Copy link
Member

szpak commented May 7, 2020

Trying to fix the issues with Groovy 3, I also spotted that some corner cases with mocking were already broken in the previous Spock versions with Groovy 2. Unfortunately, AFAIR I didn't have a good idea how to fix it without more "fragile hacks" (and it seems to be not very often used). Maybe it will be easier to cover in some sensible way at least for Groovy 3.

@kriegaex
Copy link
Contributor Author

kriegaex commented May 7, 2020

What about my idea to look into it in an earlier AST phase and transform it into something unambiguous for the later phase Spock usually operates on? AFAIR Spock only works in one phase.

@bernhof
Copy link

bernhof commented Oct 7, 2020

I'd like to add that this issue practically breaks all our stubs of classes with abstract superclasses. With a String property "bar" on stub "foo", the following happens:

  • foo.bar returns an object 😞
  • foo.getBar() returns the expected empty string

@kriegaex
Copy link
Contributor Author

I just built master and the above specification PropertyAccessTest passes on my local machine now. Thank your very much.

(Some other spec for parallel test execution failed on my Windows machine, but that is off-topic here.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants