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

Move failsafe config out of native profile #21997

Merged
merged 1 commit into from
Apr 30, 2022

Conversation

edeandrea
Copy link
Contributor

@edeandrea edeandrea commented Dec 7, 2021

This PR moves the maven-failsafe-plugin config out of the native profile and into the main profile. This PR coincides with #23488 (that one was originally part of this one before it was moved out to its own PR).

The logic is that why should integration tests only be run when building native images? Aren't their results valuable for JVM-based applications as well as apps packaged inside a container?

The history on this PR contains the conversation around this (& also the conversation as to why part of it was split off into a separate PR).

@quarkus-bot
Copy link

quarkus-bot bot commented Dec 7, 2021

Thanks for your pull request!

The title of your pull request does not follow our editorial rules. Could you have a look?

  • title should preferably start with an uppercase character (if it makes sense!)

This message is automatically generated by a bot.

@quarkus-bot quarkus-bot bot added area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/documentation area/platform Issues related to definition and interaction with Quarkus Platform area/testing labels Dec 7, 2021
@edeandrea edeandrea changed the title [Codestarts] - Change native image tests to use @QuarkusIntegrationTest Codestarts - Change native image tests to use @QuarkusIntegrationTest Dec 7, 2021
@edeandrea
Copy link
Contributor Author

edeandrea commented Dec 7, 2021

@ia3andy Have a look through what I did here. I left the native-test directory alone, but I modified some of the templates and also updated the documentation (I moved some of what was in the native-image testing guide over to the testing guide and re-worded a bit).

I also deprecated the @NativeImageTest annotation. I was on the fence about that. I can change that back if we don't want to do that yet.

@edeandrea
Copy link
Contributor Author

Just realized I didn't update the devtools-testing tests for these changes. Working on that now.

@edeandrea
Copy link
Contributor Author

Just updated with the tests for the changes. I rebased the commits into a single commit to make it easier to review.

@ia3andy
Copy link
Contributor

ia3andy commented Dec 7, 2021

@edeandrea let's also move to the test directory, then we will just test that everything is also working on gradle

@ia3andy
Copy link
Contributor

ia3andy commented Dec 7, 2021

having native-test directory doesn't make sense with this new setup IMO

@ia3andy
Copy link
Contributor

ia3andy commented Dec 7, 2021

also native-test is just for gradle, since on maven it's not actually using it (there is some kind of hack for it):
https://github.com/quarkusio/quarkus/blob/main/independent-projects/tools/codestarts/src/main/java/io/quarkus/devtools/codestarts/core/strategy/SmartPackageFileStrategyHandler.java#L40

(I love the "temporary" here :))

@edeandrea
Copy link
Contributor Author

edeandrea commented Dec 7, 2021

So instead of removing native-test, should we instead rename it something like int-test (& therefore keeping that "temporary" thingy there?

I know in Gradle it does separate into a separate source set.

@edeandrea
Copy link
Contributor Author

Although looking at https://github.com/quarkusio/quarkus/blob/main/devtools/gradle/gradle-application-plugin/src/main/java/io/quarkus/gradle/QuarkusPlugin.java#L85 it looks like its aligning the source set name with this name. If we change it, then this will break for anyone already out there.

The solution to that would be to add the ability within the gradle plugin to support both source set names. I'm not sure I want to tackle that in this PR. We could add an enhancement for that.

@ia3andy
Copy link
Contributor

ia3andy commented Dec 7, 2021

I think we should have a proper solution for gradle @gastaldi any thoughts?

@edeandrea
Copy link
Contributor Author

edeandrea commented Dec 7, 2021

It would have a rippling effect as well, since the Gradle plugin also has the testNative task, along with the nativeTestImplementation and nativeTestRuntimeOnly configurations.

To be consistent we'd have to introduce a new source set (something like int-test). We'd have to figure out a proper hierarchy (is the new source set the parent or child of the existing native-test source set?) and then apply configuration to it.

The new source set would most likely have to become the child of the existing one, so that any tweaks any current users have made will automagically "flow" into this new source set.

Then we'd have to update the documentation. And do we also introduce a new task testIntegration to be consistent? Now we have the same backwards compatibility problem there as well. We'd need to maintain backwards compatibility for a period of time.

I have lots of experience building Gradle plugins and know how to do this :) Just not sure if its something we want to bite off in this PR or not. I'll leave it to you all to decide.

@ia3andy
Copy link
Contributor

ia3andy commented Dec 7, 2021

Yeah it's maybe too much for one PR. I believe we should try to be iso between buildtools in the long term, so I'll suggest this:

  1. We keep native-test dir as it only affect gradle (by luck), on maven it was already in test and will stay there
  2. We create an issue for gradle to move toward integration test (instead of native-test) with more reflexion on what it implies and how to achieve it.

@edeandrea
Copy link
Contributor Author

That works for me. Should I set this PR as Ready for review? Or do you want to take a look thru the changes first? (Everything is still building on my local machine so I don't yet know if all tests pass)

@ia3andy
Copy link
Contributor

ia3andy commented Dec 7, 2021

@edeandrea if all the test pass locally set it ready yep

@edeandrea
Copy link
Contributor Author

Great I will do that once it finishes. Been going for quite some time already. My machine is super slow.

@edeandrea
Copy link
Contributor Author

@ia3andy Locally the tests are failing at the Quarkus - Neo4j - Deployment module. I didn't touch anything in there plus the tests got way past the modules I touched. I'm not sure what to do.

@ia3andy ia3andy marked this pull request as ready for review December 7, 2021 16:43
@ia3andy
Copy link
Contributor

ia3andy commented Dec 7, 2021

Let see what the CI says here

@edeandrea
Copy link
Contributor Author

@ia3andy looks like CI failed with things like

CreateExtensionMojoIT.testCreateStandaloneExtension:128 [Snapshot is not matching (use -Dsnap to udpate it 

@quarkus-bot
Copy link

quarkus-bot bot commented Dec 7, 2021

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building b3ce304

Status Name Step Failures Logs Raw logs
✔️ JVM Tests - JDK 11
JVM Tests - JDK 11 Windows Build Failures Logs Raw logs
✔️ JVM Tests - JDK 17
Maven Tests - JDK 11 Build Failures Logs Raw logs
Maven Tests - JDK 11 Windows Build Failures Logs Raw logs
Native Tests - Messaging2 Set up JDK 11 ⚠️ Check → Logs Raw logs

Full information is available in the Build summary check run.

Failures

⚙️ JVM Tests - JDK 11 Windows #

- Failing: devtools/cli 

📦 devtools/cli

io.quarkus.cli.CliProjectMavenTest.testCreateAppDefaults line 61 - More details - Source on GitHub

org.opentest4j.AssertionFailedError: 
Expected OK return code. Result:
result: {

io.quarkus.cli.CliProjectMavenTest.testCreateCliDefaults line 38 - More details - Source on GitHub

org.opentest4j.AssertionFailedError: expected: <false> but was: <true>
	at org.junit.jupiter.api.AssertionUtils.fail(AssertionUtils.java:55)
	at org.junit.jupiter.api.AssertFalse.assertFalse(AssertFalse.java:40)

io.quarkus.cli.CliProjectMavenTest.testCreateAppOverrides line 38 - More details - Source on GitHub

org.opentest4j.AssertionFailedError: expected: <false> but was: <true>
	at org.junit.jupiter.api.AssertionUtils.fail(AssertionUtils.java:55)
	at org.junit.jupiter.api.AssertFalse.assertFalse(AssertFalse.java:40)

io.quarkus.cli.CliProjectMavenTest.testCreateArgPassthrough line 38 - More details - Source on GitHub

org.opentest4j.AssertionFailedError: expected: <false> but was: <true>
	at org.junit.jupiter.api.AssertionUtils.fail(AssertionUtils.java:55)
	at org.junit.jupiter.api.AssertFalse.assertFalse(AssertFalse.java:40)

⚙️ Maven Tests - JDK 11 #

- Failing: integration-tests/maven 

📦 integration-tests/maven

io.quarkus.maven.it.CreateExtensionMojoIT.testCreateCoreExtension(TestInfo) line 45 - More details - Source on GitHub

java.lang.AssertionError: 
[Snapshot is not matching (use -Dsnap to udpate it automatically):CreateExtensionMojoIT/testCreateCoreExtension/dir-tree.snapshot] 
Expecting actual:

io.quarkus.maven.it.CreateExtensionMojoIT.testCreateCoreExtensionFromExtensionsDir(TestInfo) line 73 - More details - Source on GitHub

java.lang.AssertionError: 
[Snapshot is not matching (use -Dsnap to udpate it automatically):CreateExtensionMojoIT/testCreateCoreExtensionFromExtensionsDir/dir-tree.snapshot] 
Expecting actual:

io.quarkus.maven.it.CreateExtensionMojoIT.testCreateQuarkiverseExtension(TestInfo) line 102 - More details - Source on GitHub

java.lang.AssertionError: 
[Snapshot is not matching (use -Dsnap to udpate it automatically):CreateExtensionMojoIT/testCreateQuarkiverseExtension/dir-tree.snapshot] 
Expecting actual:

io.quarkus.maven.it.CreateExtensionMojoIT.testCreateStandaloneExtension(TestInfo) line 128 - More details - Source on GitHub

java.lang.AssertionError: 
[Snapshot is not matching (use -Dsnap to udpate it automatically):CreateExtensionMojoIT/testCreateStandaloneExtension/dir-tree.snapshot] 
Expecting actual:

io.quarkus.maven.it.CreateProjectCodestartMojoIT.generateCustomRESTEasyJavaProject line 95 - More details - Source on GitHub

java.lang.AssertionError: 

Expecting path:

io.quarkus.maven.it.CreateExtensionMojoIT.testCreateCoreExtension(TestInfo) line 45 - More details - Source on GitHub

java.lang.AssertionError: 
[Snapshot is not matching (use -Dsnap to udpate it automatically):CreateExtensionMojoIT/testCreateCoreExtension/dir-tree.snapshot] 
Expecting actual:

io.quarkus.maven.it.CreateExtensionMojoIT.testCreateCoreExtensionFromExtensionsDir(TestInfo) line 73 - More details - Source on GitHub

java.lang.AssertionError: 
[Snapshot is not matching (use -Dsnap to udpate it automatically):CreateExtensionMojoIT/testCreateCoreExtensionFromExtensionsDir/dir-tree.snapshot] 
Expecting actual:

io.quarkus.maven.it.CreateExtensionMojoIT.testCreateQuarkiverseExtension(TestInfo) line 102 - More details - Source on GitHub

java.lang.AssertionError: 
[Snapshot is not matching (use -Dsnap to udpate it automatically):CreateExtensionMojoIT/testCreateQuarkiverseExtension/dir-tree.snapshot] 
Expecting actual:

io.quarkus.maven.it.CreateExtensionMojoIT.testCreateStandaloneExtension(TestInfo) line 128 - More details - Source on GitHub

java.lang.AssertionError: 
[Snapshot is not matching (use -Dsnap to udpate it automatically):CreateExtensionMojoIT/testCreateStandaloneExtension/dir-tree.snapshot] 
Expecting actual:

io.quarkus.maven.it.CreateProjectCodestartMojoIT.generateCustomRESTEasyJavaProject line 95 - More details - Source on GitHub

java.lang.AssertionError: 

Expecting path:

⚙️ Maven Tests - JDK 11 Windows #

- Failing: integration-tests/maven 

📦 integration-tests/maven

io.quarkus.maven.it.CreateExtensionMojoIT.testCreateCoreExtension(TestInfo) line 45 - More details - Source on GitHub

java.lang.AssertionError: 
[Snapshot is not matching (use -Dsnap to udpate it automatically):CreateExtensionMojoIT/testCreateCoreExtension/dir-tree.snapshot] 
Expecting actual:

io.quarkus.maven.it.CreateExtensionMojoIT.testCreateCoreExtensionFromExtensionsDir(TestInfo) line 73 - More details - Source on GitHub

java.lang.AssertionError: 
[Snapshot is not matching (use -Dsnap to udpate it automatically):CreateExtensionMojoIT/testCreateCoreExtensionFromExtensionsDir/dir-tree.snapshot] 
Expecting actual:

io.quarkus.maven.it.CreateExtensionMojoIT.testCreateQuarkiverseExtension(TestInfo) line 102 - More details - Source on GitHub

java.lang.AssertionError: 
[Snapshot is not matching (use -Dsnap to udpate it automatically):CreateExtensionMojoIT/testCreateQuarkiverseExtension/dir-tree.snapshot] 
Expecting actual:

io.quarkus.maven.it.CreateExtensionMojoIT.testCreateStandaloneExtension(TestInfo) line 128 - More details - Source on GitHub

java.lang.AssertionError: 
[Snapshot is not matching (use -Dsnap to udpate it automatically):CreateExtensionMojoIT/testCreateStandaloneExtension/dir-tree.snapshot] 
Expecting actual:

io.quarkus.maven.it.CreateProjectCodestartMojoIT.generateCustomRESTEasyJavaProject line 95 - More details - Source on GitHub

java.lang.AssertionError: 

Expecting path:

io.quarkus.maven.it.CreateExtensionMojoIT.testCreateCoreExtension(TestInfo) line 45 - More details - Source on GitHub

java.lang.AssertionError: 
[Snapshot is not matching (use -Dsnap to udpate it automatically):CreateExtensionMojoIT/testCreateCoreExtension/dir-tree.snapshot] 
Expecting actual:

io.quarkus.maven.it.CreateExtensionMojoIT.testCreateCoreExtensionFromExtensionsDir(TestInfo) line 73 - More details - Source on GitHub

java.lang.AssertionError: 
[Snapshot is not matching (use -Dsnap to udpate it automatically):CreateExtensionMojoIT/testCreateCoreExtensionFromExtensionsDir/dir-tree.snapshot] 
Expecting actual:

io.quarkus.maven.it.CreateExtensionMojoIT.testCreateQuarkiverseExtension(TestInfo) line 102 - More details - Source on GitHub

java.lang.AssertionError: 
[Snapshot is not matching (use -Dsnap to udpate it automatically):CreateExtensionMojoIT/testCreateQuarkiverseExtension/dir-tree.snapshot] 
Expecting actual:

io.quarkus.maven.it.CreateExtensionMojoIT.testCreateStandaloneExtension(TestInfo) line 128 - More details - Source on GitHub

java.lang.AssertionError: 
[Snapshot is not matching (use -Dsnap to udpate it automatically):CreateExtensionMojoIT/testCreateStandaloneExtension/dir-tree.snapshot] 
Expecting actual:

io.quarkus.maven.it.CreateProjectCodestartMojoIT.generateCustomRESTEasyJavaProject line 95 - More details - Source on GitHub

java.lang.AssertionError: 

Expecting path:

@ia3andy
Copy link
Contributor

ia3andy commented Dec 8, 2021

@edeandrea run all the failing test (due to snapshot mismatch) with mvn test -Dsnap it will update the comparison snapshots. Then you add them to your commit and it should be fine.

@edeandrea
Copy link
Contributor Author

I'll try that again today. That's what I did yesterday locally and after 2 hours the neo4j tests were failing

@edeandrea
Copy link
Contributor Author

edeandrea commented Mar 24, 2022

I mean if a project includes QuarkusIntegrationTests, what would be the point of having skipITs=false by default? AFAIU, the ITs are supposed to run in integration-test/verify phase. What is the reason to disable the conventional flow?

Its to alleviate some of @gsmet 's concerns about lots of projects may not fully understand that they shouldn't have prod config in their main properties file, and executing these tests whenever verify is run may inadvertently execute those tests against production resources.

I agree with you - but I also understand his concern (took me some time to grasp it). The change is a no-op for any existing projects, or for anyone in the future who doesn't want to run ITs outside of native images, and opt-in for people who want to do it for more than native images. If we enabled it by default we'd be enabling functionality for users who may not realize it. I think its a good compromise.

@gsmet
Copy link
Member

gsmet commented Mar 24, 2022

@edeandrea

about lots of projects may not fully understand that they shouldn't have prod config in their main properties file

Well, they have perfectly understood what we have been documented since day 1.
We have always said that the prod profile should be used to define configuration properties for production.
It's all over the place in the doc. And in the quickstarts.

For instance:

Production databases need to be configured as normal, so if you want to include a production database config in your
application.properties and continue to use Dev Services we recommend that you use the `%prod.` profile to define your
database settings

@aloubyansky
Copy link
Member

Sorry, I just noticed I was writing -DskipITs=false while we are setting it to true by default.
I get Guillaume's point about the profile. I don't see a point in -DskipITs=true by default. I don't see what it breaks. If we want to make it safer, we should stop making ITs extend unit tests, ASAP.
So, -1 to skipITs=true by default.

@aloubyansky
Copy link
Member

That's the problem @gsmet Looks like it's time re-evaluate that. There is no magic here with the QuarkusIntegrationTest unless we want to build the app twice, which, imo, isn't a good idea.

@edeandrea
Copy link
Contributor Author

we should stop making ITs extend unit tests, ASAP

+100 to this

@aloubyansky
Copy link
Member

I'd consider introducing an integration-test profile that would be enabled automatically, similar to test, when the ITs are run. The purpose of that profile would be to complete or adjust the prod profile (if necessary) with env-specific config for ITs to run.

@maxandersen
Copy link
Member

@gsmet this PR does not enable the running of IT by default, is that viable for you? or are you against @QuarkusIntegration* annotations uses "prod" as it has been doing for many releases now?

@maxandersen
Copy link
Member

@aloubyansky "introducing an integration-test profile" is that a quarkus profile or a maven profile?

@aloubyansky
Copy link
Member

@aloubyansky "introducing an integration-test profile" is that a quarkus profile or a maven profile?

A Quarkus one. With a higher priority than prod.

@maxandersen
Copy link
Member

after chat with @aloubyansky @geoand the current PR isn't hurting and makes sense for 2.8 but do agree it would be interesting looking into to introduce a "integration" quarkus profile that gets combined with the generated artifact (jar/nativeimage/container with prod static init). But that would also introduce change in behaviours.

@ebullient
Copy link
Member

I still feel like we're missing the point.

a) things are baked into the built image. We know this. We do it on purpose.
b) We were using failsafe to run tests against the built native image (with baked in prod settings) already.
c) Writing integration tests that run against the final image (which uses "prod" build-time settings) is reasonable, and allows IT to serve some purpose.
d) Supporting an it profile would make it easier to specify/set up test resources that would take precedence over prod just as dev/test do (for things like test settings for testcontainer-based backing services)

I think we've surfaced that we generally have a problem with testing final images that we made worse when we added @QuarkusIntegrationTest, but we basically kept it hidden by only using failsafe for native builds (which already uses prod settings... sooo... )

IMO, I'm +1 for doing this now (for fresh projects) and chasing the fallout. Eric's changes at least make it easier for people to enable integration tests more broadly without having to refactor their pom (for new projects). The fact that we have muddy water around the use of the prod profile is already a problem for native tests right now. So if we're going to fix it, lets fix it.

@maxandersen
Copy link
Member

@ebullient unless i'm missing something what you write is the same update I made?

We have several +1's on this PR; just need to clarify with @gsmet if anything needs updating on docs before 2.8 and if "int" profile idea gets added in for 2.8 or 2.9.

@ebullient
Copy link
Member

same. broke it out differently to make sure it was still the same. ;)

@maxandersen
Copy link
Member

ack - yup. adding failsafe this way is the rational safe next step IMO.

@gsmet
Copy link
Member

gsmet commented Mar 24, 2022

IMO, I'm +1 for doing this now (for fresh projects) and chasing the fallout.

It's a big -1 on my side. We fix things and then we change the generated projects once we have something working reasonably well. It's definitely a very bad idea to do it the other way around and get the projects of everyone not working properly with prod being suddenly unusable.
If we want to suggest how to use ITs, we can add a commented section with proper warnings about how it works but it shouldn't be there by default if it does not work correctly.

You cannot have the notion of prod profiles completely broken in a release. It is just not acceptable.

@gsmet
Copy link
Member

gsmet commented Mar 24, 2022

And in any case, it's too late for 2.8. I won't get structural changes to the generated project merged post CR1.

So we have a whole cycle to figure this out and do things properly.

Copy link
Member

@maxandersen maxandersen left a comment

Choose a reason for hiding this comment

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

I'm going to +1 this for merging into main targeting 2.9.

In addition we got agreement on the following for 2.9:

  • Document what kind of settings should put in prod profile (@geoand )
    avoid putting settings in prod code that is a security concern
    Review all the existing guides to make sure things are clearMake sure the quickstarts
    don’t promote bad practices (or add proper warnings in the application.properties if it
    makes sense)
    Use quarkus.profile for explicit config pickup

for 2.9+ (as in probably more like 2.10+):

@edeandrea
Copy link
Contributor Author

edeandrea commented Apr 1, 2022

Hey @maxandersen I was working on something completely unrelated and came across https://quarkus.io/guides/maven-tooling#build-tool-maven and realized I hadn't updated it as part of this PR. I've just pushed a new commit (& rebased from main).

Please take a look (https://github.com/quarkusio/quarkus/pull/21997/files#diff-22914b1e1fa444916b50c249fd5e9ac895c5953f40602207dd83cb0f59c10e5a) and make sure the documentation on that page in this PR looks ok.

@edeandrea
Copy link
Contributor Author

Hey @maxandersen with 2.9 coming soon, I thought we agreed to target this for 2.9?

@maxandersen
Copy link
Member

I honestly thought this was merged shortly after we agreed. We'll need to put it in 2.10 :/

@maxandersen maxandersen dismissed stuartwdouglas’s stale review April 30, 2022 14:10

agreement to merge in

@maxandersen maxandersen merged commit e4767e6 into quarkusio:main Apr 30, 2022
@quarkus-bot quarkus-bot bot added this to the 2.10 - main milestone Apr 30, 2022
@geoand
Copy link
Contributor

geoand commented Apr 30, 2022

There are some follow up actions we agreed to take that I signed up for. Unfortunately it fell off my radar so I'll do them for 2.10

@edeandrea edeandrea deleted the gh-18820 branch June 22, 2022 12:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/cli Related to quarkus cli (not maven/gradle/etc.) area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/documentation area/maven area/platform Issues related to definition and interaction with Quarkus Platform area/testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Generated projects are using @NativeImageTest, @QuarkusIntegrationTest should be pushed.