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

@DisabledOnNativeImage is not working on class level #5586

Closed
rsvoboda opened this issue Nov 19, 2019 · 9 comments · Fixed by #5590
Closed

@DisabledOnNativeImage is not working on class level #5586

rsvoboda opened this issue Nov 19, 2019 · 9 comments · Fixed by #5590
Assignees
Labels
kind/bug Something isn't working
Milestone

Comments

@rsvoboda
Copy link
Member

@DisabledOnNativeImage is not working on class level, works only on method level, @Target is set to ElementType.TYPE and ElementType.METHOD.

I expect that no tests are executed in native mode when this annotation is put on class level.

@Target({ ElementType.TYPE, ElementType.METHOD })
@Retention(RetentionPolicy.RUNTIME)
@Documented
public @interface DisabledOnNativeImage {

@gwenneg can you take a look ?

Steps to reproduce:

mvn io.quarkus:quarkus-maven-plugin:999-SNAPSHOT:create \
    -DprojectGroupId=org.acme \
    -DprojectArtifactId=getting-started \
    -DclassName="org.acme.quickstart.GreetingResource" \
    -Dpath="/hello"

Add DisabledOnNativeImage annotation

@QuarkusTest
@DisabledOnNativeImage
public class GreetingResourceTest {
    @Test
    public void testHelloEndpoint() {
        given()
          .when().get("/hello")
          .then()
             .statusCode(200)
             .body(is("hello"));
    }
}

@NativeImageTest
public class NativeGreetingResourceIT extends GreetingResourceTest {
    // Execute the same tests but in native mode.
}

NativeGreetingResourceIT is executed:

mvn clean verify -Dnative
...
[INFO] Running org.acme.quickstart.NativeGreetingResourceIT
Executing [/Users/rsvoboda/tmp/getting-started/target/getting-started-1.0-SNAPSHOT-runner, -Dquarkus.http.port=8081, -Dtest.url=http://localhost:8081, -Dquarkus.log.file.path=target/quarkus.log]
2019-11-19 13:11:43,452 INFO  [io.quarkus] (main) getting-started 1.0-SNAPSHOT (running on Quarkus 999-SNAPSHOT) started in 0.013s. Listening on: http://0.0.0.0:8081
2019-11-19 13:11:43,452 INFO  [io.quarkus] (main) Profile prod activated.
2019-11-19 13:11:43,452 INFO  [io.quarkus] (main) Installed features: [cdi, resteasy]
[INFO] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 1.394 s - in org.acme.quickstart.NativeGreetingResourceIT
[INFO]
[INFO] Results:
[INFO]
[INFO] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0
@rsvoboda rsvoboda added the kind/bug Something isn't working label Nov 19, 2019
@gastaldi
Copy link
Contributor

It looks like the annotation is missing a @Inherited annotation, I'll have a look.

@gastaldi gastaldi self-assigned this Nov 19, 2019
@mkouba
Copy link
Contributor

mkouba commented Nov 19, 2019

Well, it does not make any sense to disable all the tests, or?

@rsvoboda
Copy link
Member Author

@mkouba
Copy link
Contributor

mkouba commented Nov 19, 2019

I think that @Disabled semantics is a little bit different. @DisabledOnNativeImage basically means do not run the test methods inherited from the test class and in that case the test class annotated with @NativeImageTest does not have to extend the @QuarkusTest at all. What's your use case?

@rsvoboda
Copy link
Member Author

To me @DisabledOnNativeImage on class level is shortcut to disable all the tests in that class for native mode, I think user should have this option available.

If @DisabledOnNativeImage was just ElementType.METHOD I wouldn't file this issues, but it can be put on class level too.

in that case the test class annotated with @NativeImageTest does not have to extend the @QuarkusTest at all

Do understand what you mean by this.

@gastaldi
Copy link
Contributor

PR created: #5590

@mkouba
Copy link
Contributor

mkouba commented Nov 19, 2019

To me @DisabledOnNativeImage on class level is shortcut to disable all the tests in that class for native mode, I think user should have this option available.

So the use case is that you would like to inherit some instance methods from the GreetingResourceTest but you don't want to run any of the test declared there? Looks like a legitimate but very rare use case.

If @DisabledOnNativeImage was just ElementType.METHOD I wouldn't file this issues, but it can be put on class level too.

That's a good point. But it could be a mistake too.

Do understand what you mean by this.

@rsvoboda in your example, if you don't want to "inherit" the methods from GreetingResourceTest then simply remove the exends GreetingResourceTest from the NativeGreetingResourceIT declaration.

@rsvoboda
Copy link
Member Author

The benefit of disabled tests is that you see warnings in the log, e.g [WARNING] Tests run: 131, Failures: 0, Errors: 0, Skipped: 1. I lose this when I remove extends GreetingResourceTest. And of course, disabling tests should be just temporary thing ;)

@gsmet gsmet added this to the 1.1.0 milestone Nov 20, 2019
@mkouba
Copy link
Contributor

mkouba commented Nov 20, 2019

you see warnings in the log

Hm, I don't see any benefit in seeing warnings for methods annotated with @DisabledOnNativeImage. Rather the opposite - these methods are not supposed to be "temporarily disabled". They're mostly supposed to be always skipped. Most probably because they're using injected values or something like that.

And of course, disabling tests should be just temporary thing ;)

And my point is that @DisabledOnNativeImage is not a temporary thing ;-).

Anyway, I do understand your arguments. So +0 for the impl.

Simulant87 pushed a commit to Simulant87/quarkus that referenced this issue Nov 23, 2019
Simulant87 pushed a commit to Simulant87/quarkus that referenced this issue Nov 23, 2019
mmusgrov pushed a commit to mmusgrov/quarkus that referenced this issue Dec 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants