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

PollingConditions.eventually should work also if defined with def #1054

Open
macieg opened this issue Nov 22, 2019 · 7 comments
Open

PollingConditions.eventually should work also if defined with def #1054

macieg opened this issue Nov 22, 2019 · 7 comments

Comments

@macieg
Copy link
Contributor

@macieg macieg commented Nov 22, 2019

Hi, in the below's example I've provided two tests:
first is going to pass, second is going to fail

The only difference is the way of creating PollingConditions object

class Playground extends Specification {

    def "not boom"() {
        given:
        def conditions = new PollingConditions()

        when:
        def x = 10

        then:
        conditions.eventually {
            1 == 2
        }
    }

    def "boom 2"() {
        given:
        PollingConditions conditions = new PollingConditions()

        when:
        def x = 10

        then:
        conditions.eventually {
            1 == 2
        }
    }
}

Is it expected behavior? There is nothing about it in the documentation of @ConditionBlock, which I strongly believe is related to that (we don't need to write assert keyword in the clojure).

If it's related to groovy itself it might be a good idea to fail when such usage is detected.

@szpak

This comment has been minimized.

Copy link
Member

@szpak szpak commented Nov 23, 2019

I've been debugging it years ago and after the code analysis it was problematic for Spock to determine that def is in fact the PollingConditions type. Maybe it will be easier to implement in Spock 2. As a workaround you need to explicitly add assert in the eventually block.
I've mentioned it in my old presentation, but I don't see any related issue, so that could stay for a reference.

Could you make a PR to mention that in the documentation part for PollingConditions?

@macieg

This comment has been minimized.

Copy link
Contributor Author

@macieg macieg commented Nov 23, 2019

Sure, I'll prepare a PR

macieg added a commit to macieg/spock that referenced this issue Nov 23, 2019
…ntation as well as test that explains the issue
macieg added a commit to macieg/spock that referenced this issue Nov 23, 2019
…ntation as well as test that explains the issue
macieg added a commit to macieg/spock that referenced this issue Nov 23, 2019
…ntation as well as test that explains the issue
@macieg

This comment has been minimized.

Copy link
Contributor Author

@macieg macieg commented Nov 23, 2019

szpak added a commit that referenced this issue Nov 25, 2019
#1055)

* Issue #1054 | Added warning to PollingConditions documentation as well as test that explains the issue

* Slightly rework pending feature with PollingConditions limitation
@Vampire

This comment has been minimized.

Copy link
Member

@Vampire Vampire commented Nov 26, 2019

Besides that the PR has "clojure" instead of "closure",
the whole PollingConditions is not documented in the user guide,
only mentioned in the change log. :-(

@szpak

This comment has been minimized.

Copy link
Member

@szpak szpak commented Nov 26, 2019

@Vampire Could you create a separate issue to document PollingConditions (or even better create a PR with an update :) )?

@szpak szpak changed the title PollingConditions.eventually doesn't work if defined with def PollingConditions.eventually should work also if defined with def Jan 9, 2020
@szpak

This comment has been minimized.

Copy link
Member

@szpak szpak commented Jan 9, 2020

Documentation is updated. I changed the title to reflect that it could be potentially improved to support definition by def one day.

@leonard84

This comment has been minimized.

Copy link
Member

@leonard84 leonard84 commented Jan 10, 2020

IIRC the issue is that we need to know the type of the receiver of the closure, so that we can perform compile time AST transformations on it. def is just an alias for Object and figuring out at runtime what type it is. So with def we don't actually know that the method eventually belongs to PollingConditions and is annotated with @ConditionBlock.

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

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.