-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Adds argLine to instrument Integration Tests #17677
Adds argLine to instrument Integration Tests #17677
Conversation
With the code from this PR, I've been able to define my plugins as this: And get a coverage report containing the results of both the surefire and failsafe coverage.
|
@@ -39,6 +40,7 @@ private JarLauncher(Path jarPath, Config config) { | |||
config.getValue("quarkus.http.test-port", OptionalInt.class).orElse(DEFAULT_PORT), | |||
config.getValue("quarkus.http.test-ssl-port", OptionalInt.class).orElse(DEFAULT_HTTPS_PORT), | |||
config.getValue("quarkus.test.jar-wait-time", OptionalLong.class).orElse(DEFAULT_JAR_WAIT_TIME), | |||
config.getOptionalValue("quarkus.test.argLine", String.class).orElse(null), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be added to io.quarkus.deployment.dev.testing.TestConfig
so it ends up in the 'all properties' documentation.
It would also be good to include this in the testing documentation, as we don't have any info at the moment on doing code coverage with @QuarkusIntegrationTest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright. So wrt the io.quarkus.deployment.dev.testing.TestConfig
I'm a bit confused.
I see in that class
/**
* The profile to use when testing the native image
*/
@ConfigItem(defaultValue = "prod")
String nativeImageProfile;
Which seems a bit misleading, since it's not prod
explicitly as a default -- but rather null
.
So, while I patterned the implementation of this OptionalValue off that, I feel like I probably should not define a defaultValue
for the @ConfigItem
, does that sound right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wrt the testing documentation -- where would I find that, and I'm not convinced I'm your guy for documenting that.
I've found the testing documentation to be a bit ... lacking... especially in the realm of Integration testing, and what-not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, the default value should be null. Also the property name should be something line quarkus.test.integration-jvm-arg-line
to fit in with our naming convention, and also to make it clear where this applies so you have something like:
/**
* JVM parameters that are used to launch jar based integration tests.
*/
@ConfigItem
String integrationJvmArgLine;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Documentation and TestConfig updated. Let me know what else I can do, or any other feedback you have.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stuartwdouglas I'm not sure why this is failing, it's also failing locally.
It looks like I needed to add this ConfigItem to other classes as well, but I can't quite seem to put my finger on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh wow. How'd I miss that it needs to be Optional<String>
? 🤦
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building 6d121e4
Full information is available in the Build summary check run. Test Failures⚙️ Maven Tests - JDK 11 #📦 integration-tests/maven✖ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks a lot!
Please squash the commits when you are done |
b03843b
to
887897e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks a lot
@@ -1088,6 +1088,89 @@ Finally, if a container image was created during the build (by using including t | |||
|
|||
As is the case with `@NativeImageTest`, this is a black box test that supports the same set features and has the same limitations. | |||
|
|||
=== Code Coverage for Integration Tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually can you move this to https://github.com/quarkusio/quarkus/blob/c28a2276d9fbdc5c438c19b39b5b357daee58424/docs/src/main/asciidoc/tests-with-coverage.adoc
Otherwise looks good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change pushed.
Seems like the imports are not supported. Can you please run Thanks |
… documentation, and testing documentation on how to configure. * Fix quarkusio#7536 by adding the ability to specify an argLine property when running tests. For Example, in a project I'm building: ``` <plugin> <artifactId>maven-failsafe-plugin</artifactId> <version>${surefire-plugin.version}</version> <executions> <execution> <goals> <goal>integration-test</goal> <goal>verify</goal> </goals> <configuration> <systemPropertyVariables> <java.util.logging.manager>org.jboss.logmanager.LogManager</java.util.logging.manager> <maven.home>${maven.home}</maven.home> <quarkus.test.native-image-profile>it</quarkus.test.native-image-profile> <argLine>${argLine}</argLine> </systemPropertyVariables> </configuration> </execution> </executions> </plugin> ```
c66481c
to
e6cd85e
Compare
For Example, in a project I'm building:
Fixes: #17760