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

Properly allow mixing @QuarkusTest and @QuarkusMainTest #28499

Merged
merged 1 commit into from
Oct 12, 2022

Conversation

geoand
Copy link
Contributor

@geoand geoand commented Oct 11, 2022

This is done by "restarting" the application when one test type follows the other.

Fixes: #28486

@quarkus-bot

This comment has been minimized.

This is done by "restarting" the application when one
test type follows the other.

Fixes: quarkusio#28486
@quarkus-bot

This comment has been minimized.

Copy link
Member

@ebullient ebullient left a comment

Choose a reason for hiding this comment

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

Should we change "in the same run" to in the same package? I have a feeling the package boundary will work, and that suggests a solution.

Does run otherwise mean build phase?

@geoand
Copy link
Contributor Author

geoand commented Oct 11, 2022

Should we change "in the same run" to in the same package? I have a feeling the package boundary will work, and that suggests a solution.

I am not sure why it worked in your case, it should not really make a difference.

Does run otherwise mean build phase?

It generally means Maven surefire execution, or whatever the Gradle equivalent is :)

@quarkus-bot
Copy link

quarkus-bot bot commented Oct 11, 2022

Failing Jobs - Building f997c67

Status Name Step Failures Logs Raw logs
✔️ JVM Tests - JDK 11
✔️ JVM Tests - JDK 17
JVM Tests - JDK 17 MacOS M1 Set up runner ⚠️ Check → Logs Raw logs
✔️ JVM Tests - JDK 18

@ebullient
Copy link
Member

Should we change "in the same run" to in the same package? I have a feeling the package boundary will work, and that suggests a solution.

I am not sure why it worked in your case, it should not really make a difference.

Does run otherwise mean build phase?

It generally means Maven surefire execution, or whatever the Gradle equivalent is :)

Right.. but is that accurate? That is one hell of a constraint (that you have to go through the work to make a whole 'nother execution in the pom.xml.. yuck).

What is the example where that would actually be required?

@geoand
Copy link
Contributor Author

geoand commented Oct 11, 2022

Right.. but is that accurate? That is one hell of a constraint (that you have to go through the work to make a whole 'nother execution in the pom.xml.. yuck).

That constraint has now been lifted :)

@ebullient
Copy link
Member

ebullient commented Oct 11, 2022

That constraint has now been lifted :)

For this case, yes, but the condition test (with the problematic/unclear wording) is still there. What theoretical case is it covering? And would putting that in a different package solve that case (not having x and y in the same package is something more understandable than having to create a new surefire test execution? and if it isn't, then we should say 'surefire test execution' instead of 'run')

@geoand
Copy link
Contributor Author

geoand commented Oct 11, 2022

The condition has been amended to allow mixing these two types of tests.

Changing the package should not make a difference, although the reason it did is probably test order

@geoand
Copy link
Contributor Author

geoand commented Oct 11, 2022

What theoretical case is it covering?

See #27247

@geoand geoand changed the title Allow mixing @QuarkusTest and @QuarkusMainTest Properly allow mixing @QuarkusTest and @QuarkusMainTest Oct 11, 2022
@ebullient
Copy link
Member

I feel like we're talking past each other. I get the original problem that instigated the change, so what is now left as the else path.. and why isn't the fix that you made for this case applicable to the general case? Are we just waiting for the next failure for some other conflict between types because we aren't sure?

@geoand
Copy link
Contributor Author

geoand commented Oct 12, 2022

I'm trying to play it safe as I'm not totally convinced that all tests can play with all other tests

@geoand geoand merged commit d411e19 into quarkusio:main Oct 12, 2022
@quarkus-bot quarkus-bot bot added this to the 2.14 - main milestone Oct 12, 2022
@geoand geoand deleted the #28486 branch October 12, 2022 05:22
@gsmet gsmet modified the milestones: 2.14.0.CR1, 2.13.6.Final Dec 14, 2022
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.

Using QuarkusTest and QuarkusMainTest in the same package breaks in 2.13.*
3 participants