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

Introduce a way to disable integration tests for certain build types #25025

Merged
merged 2 commits into from
Apr 21, 2022

Conversation

geoand
Copy link
Contributor

@geoand geoand commented Apr 20, 2022

Relates to: #25010

@geoand geoand requested a review from famod April 20, 2022 07:01
Copy link
Contributor

@snazy snazy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should @DisabledOnNativeImage + @DisabledOnNativeDockerImage get deprecated?

ALL,
JAR,
CONTAINER,
NATIVE_BINARY,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be nice to have NATIVE_CONTAINER + JAR_CONTAINER as well

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How come? Is there a reason you would want to run tests for jar container and not a native container?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe. Could think of a configuration/profile that uses an on-heap cache (e.g. caffeine) with an uber-jar, but no cache with a native image (because caches are not sooo great with serial GC in native images from GraalVM community).

Copy link
Contributor Author

@geoand geoand Apr 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would rather not do this unless we have a real need for it

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay


enum ArtifactType {
ALL,
JAR,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mind adding javadocs to the corresponding type-strings (like fast-jar, uber-jar, etc)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really want to do this, as these strings are internal and not meant to be used by anyone but the testing facility

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Meant the values for quarkus.package.type

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That information isn't available at this point and in any case, for tests it should not make a difference - I mean in what scenario would want to only not execute a test if it's for an uber-jar for example?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thought it could be useful information for users to associate quarkus.package.type=fast-jar with the JAR enum value.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(javadoc)

@geoand
Copy link
Contributor Author

geoand commented Apr 20, 2022

Should @DisabledOnNativeImage + @DisabledOnNativeDockerImage get deprecated?

Good point, added a second commit

Copy link
Member

@famod famod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I only left a remark w.r.t. to star imports.

geoand added 2 commits April 21, 2022 07:23
This probably should have been done when we deprecated @NativeImageTest
@geoand geoand merged commit 09897d1 into quarkusio:main Apr 21, 2022
@quarkus-bot quarkus-bot bot added this to the 2.9 - main milestone Apr 21, 2022
@geoand geoand deleted the #25010 branch April 21, 2022 08:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants