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

@Ignore / @IgnoreIf not working as expected #1169

Closed
jameskleeh opened this issue May 22, 2020 · 10 comments · Fixed by #1215
Closed

@Ignore / @IgnoreIf not working as expected #1169

jameskleeh opened this issue May 22, 2020 · 10 comments · Fixed by #1215
Labels
Milestone

Comments

@jameskleeh
Copy link

jameskleeh commented May 22, 2020

Issue description

Tests annotated with @ignore or @IgnoreIf({ << a true condition >> }) are executed in the github actions CI

How to reproduce

I can't reproduce the issue locally. I can only reproduce on CI

https://github.com/micronaut-projects/micronaut-core/runs/697317541?check_suite_focus=true#step:7:701

Here is the test at that commit:
https://github.com/micronaut-projects/micronaut-core/blob/2a721c30de89ad52b3a7a9294341a1fd1c841f56/discovery-client/src/test/groovy/io/micronaut/discovery/propertystore/AWSParameterStoreClientSpec.groovy#L48

Note that with Spock 1 / Groovy 2.5 this is not an issue

Additional Environment information

Gradle 6, Spock 2.0 M2, Groovy 3

Java/JDK

That specific build is failing on JDK8

Groovy version

3.0.3

Gradle

6.3

Operating System

The CI is running on a variant of Linux

IDE

N/A

Build-tool dependencies used

Gradle/Grails

testImplementation 'org.spockframework:spock-core:2.0-M2-groovy-3.0'

Given that this only fails on CI it seems there is probably some odd set of circumstances, however I wanted to see if perhaps you had any ideas for me to debug, etc.. Thanks!

@Vampire
Copy link
Member

Vampire commented May 25, 2020

My guess would be, that with 1.x the shared fields were not initialized if the spec was ignored and that now the shared fields are initialized unconditionally.
It probably does not fail for you locally as you have the region configured locally for example in ~/.aws/config.
So the bug here probably is that the shared field initializers are executed for ignored specs.

@Vampire
Copy link
Member

Vampire commented May 25, 2020

Jep, 1.3 code in BaseSpecRunner:

public int run() {
  // Sometimes a spec run is requested even though the spec has been excluded
  // (e.g. if JUnit is in control). In such a case, the best thing we can do
  // is to treat the spec as skipped.
  if (spec.isExcluded() || spec.isSkipped()) {
    supervisor.specSkipped(spec);
    return OK;
  }

  createSpecInstance(true);
  runSharedInitializer();
  runSpec();

  return resetStatus(SPEC);
}

And now it is triggered unconditionally in SpecNode#prepare.
@leonard84 can you have a look into this as you did all the JUnit 5 changes?
I guess this should better be fixed before 2.0 final.

@jameskleeh
Copy link
Author

@Vampire Thanks for looking into it

@leonard84 leonard84 added the bug label Jul 1, 2020
@leonard84 leonard84 added this to the 2.0 milestone Jul 1, 2020
@leonard84
Copy link
Member

@marcphilipp any idea why it might ignore the shouldBeSkipped check?

@marcphilipp
Copy link
Member

Node.prepare is called before Node.shouldBeSkipped. Is that the underlying issue?

@leonard84
Copy link
Member

Probably, so we should check the skip state there and don't create the shared instance if we want to skip the spec anyway.

@jameskleeh
Copy link
Author

Thanks for the fix @leonard84 !

@leonard84
Copy link
Member

@jameskleeh could you verify that it fixes you problem?

@jameskleeh
Copy link
Author

@leonard84 I'd love to. Can you do a release with this change?

@Vampire
Copy link
Member

Vampire commented Sep 30, 2020

Just try it in the snapshot version. Add https://oss.sonatype.org/content/repositories/snapshots as repository and use 2.0-SNAPSHOT as version.

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

Successfully merging a pull request may close this issue.

4 participants