Support @TestProfile in native mode #12974 #13154
Conversation
|
cc @lburgazzoli |
|
The CI failure unveils an issue that I need to fix. |
|
Good to review now |
|
Any chance to get a review @geoand or somebody else? |
|
Although I understand this took a fair amount of work (very good work BTW) and I do remember that Camel had a use case where you wanted to be able to build the run the native tests for all profiles, I can't say I'm thrilled about this... I'd like to hear what @stuartwdouglas and @gsmet have to say as well. |
|
I admit this not a feature for common application developers. It is rather something for extension developers like us in Camel Quarkus, where we would like to test various sets of configurations also in native mode. I guess we are not the only ones in the whole ecosystem who might need this, so Quarkus seems to be most natural place for this feature. In theory 1. and 2. are easy to workaround on our side, but 3. would mean a lot of duplication. But maybe the whole approach is wrong and somebody sees some other simple and effective solution? |
|
I don't know... I'm not -1, but I'm definitely not +1 either. In your shoes I would probably just use different Maven profiles and / or failsafe configurations. Let's see what others think as well. |
|
Pinging @gsmet and @stuartwdouglas again :) |
|
Do I get it right that this basically just enable using @testprofile for the same native images with varied configurations and if a configuration changes build time properties it fails ? That seems like a good natural first step and matches what is done for java mode does it not ? What is the objection to do that ? |
|
The issue I have is that the PR also looks at the configuration profiles. It feels very heavy handed |
Yes
Not exactly. This proposal also allows for testing multiple sets of build time configs. This is possible thanks to the change in the native image builder that is now able to build a native image for each application.properties profile that has |
|
Could you guys be more specific about what this PR needs to become mergeable? I am happy to discuss possible changes in this PR, but I'd actually need to know what you want/not want. @lburgazzoli has thoroughly explained the objective in the mailing list, so the background and motivation for this PR should be clear enough. In his last message Luca even said, he is fine to limit the scope to runtime options only and have some sort of warning/failure if you attempt to change build time only options. This would mean that the new ability of the native image builder to build multiple executables with distinct build time configs is not necessary. I can remove that part, but I'd like to hear some authoritative voice saying that you want me to do that (plus any other changes you wish). |
|
I need to take another look. |
|
After looking at this again, I think my main objection is that the PR is coupling config profiles with test profiles. I won't block this (as I understand it fills a need for Camel), but I just can't get behind it either. I would for sure like to hear what others think as well. |
|
Thanks for the statement @geoand. Let me try to remove the multi-native-image capability from the image builder and from the test extension - that's basically what Luca proposed on the ML. I guess it could eliminate your objection wholly or partly. |
|
That does sounds much better to me :) |
ghost
added
area/config
area/core
area/infra-automation
Done in 9aa9f86. Please re-review. Questions/tasks still open:
|
|
We should always try and keep maven and gradle experience equal capable wherever possible. |
|
Actually, I think after the last iteration nothing needs to be done for Gradle because all changes are done in non-Maven code that should be the same for Gradle. |
| additional.putAll(profileInstance.getConfigOverrides()); | ||
| final Set<Class<?>> enabledAlternatives = profileInstance.getEnabledAlternatives(); | ||
| if (!enabledAlternatives.isEmpty()) { | ||
| additional.put("quarkus.arc.selected-alternatives", enabledAlternatives.stream() |
There was a problem hiding this comment.
I don't think this can be supported as it is a build time property only
There was a problem hiding this comment.
Good point, but I think the current solution is not that bad: If enabledAlternatives is a non-empty set containing anything different than what the test app was compiled with, the test app will fail at boot. I think that's better than ignoring silently or warning. The user should do a conscious decision what to do in such a case. If he wants to test with the alternatives in JVM mode and ignore them in the native mode, he can create subclass of his QuarkusTestProfile for the native mode in which he returns an empty set of alternatives.
Do you think warn+ignore would be better?
There was a problem hiding this comment.
You are probably right, Let's keep it for now and see.
|
I added a small comment. Other than that, I'm fine with this |
|
Is there still anything missing here @geoand ? |
Other people's feedback :) |
|
Could you please be more specific, so that we move forward? |
|
Like I said above, I want @stuartwdouglas and or @gsmet to OK this as well |
|
@stuartwdouglas or @gsmet would you be so kind and review this so that we can move forward? |
|
@ppalaga can you please rebase onto master? I want to make sure nothing broke in the meantime (although it's very unlikely) |
|
2478d22: rebased. Thanks! |
## Description In quarkusio#13154, the annotation `@TestProfile` was supported also in Native tests. However, the native extension was not processing the test resources in the test profile like: ``` public class ConfluentTestProfile implements QuarkusTestProfile { @OverRide public String getConfigProfile() { return "confluent"; } @OverRide public List<TestResourceEntry> testResources() { return Collections.singletonList(new TestResourceEntry(ConfluentKafkaResource.class)); } } ``` This PR makes the above to be supported. The solution behaves the same as done in the Quarkus Test extension.
## Description In quarkusio#13154, the annotation `@TestProfile` was supported also in Native tests. However, the native extension was not processing the test resources in the test profile like: ``` public class ConfluentTestProfile implements QuarkusTestProfile { @OverRide public String getConfigProfile() { return "confluent"; } @OverRide public List<TestResourceEntry> testResources() { return Collections.singletonList(new TestResourceEntry(ConfluentKafkaResource.class)); } } ``` This PR makes the above to be supported. The solution behaves the same as done in the Quarkus Test extension.
## Description In quarkusio#13154, the annotation `@TestProfile` was supported also in Native tests. However, the native extension was not processing the test resources in the test profile like: ``` public class ConfluentTestProfile implements QuarkusTestProfile { @OverRide public String getConfigProfile() { return "confluent"; } @OverRide public List<TestResourceEntry> testResources() { return Collections.singletonList(new TestResourceEntry(ConfluentKafkaResource.class)); } } ``` This PR makes the above to be supported. The solution behaves the same as done in the Quarkus Test extension.
From quarkusio/quarkus#13154, the `@TestProfile` annotation supports Native tests, therefore the modules 001, 301 and 303 can be extended for Native test coverage. As part of this work, we have supported also the test resources for Native: quarkusio/quarkus#18077 And also, I've removed the workaround for quarkusio/quarkus#16810 that had been fixed a while ago. Moreover, for 301: - We've fixed the native failures and got rid of the application.properties file in the test resources. This can be closed now: quarkusio/quarkus#17829. - Reduce the number of traces to ease the troubleshot of failures. And for 303: - Refactored the test resources to not rely on system properties to load/unload application properties.
## Description In quarkusio#13154, the annotation `@TestProfile` was supported also in Native tests. However, the native extension was not processing the test resources in the test profile like: ``` public class ConfluentTestProfile implements QuarkusTestProfile { @OverRide public String getConfigProfile() { return "confluent"; } @OverRide public List<TestResourceEntry> testResources() { return Collections.singletonList(new TestResourceEntry(ConfluentKafkaResource.class)); } } ``` This PR makes the above to be supported. The solution behaves the same as done in the Quarkus Test extension.
From quarkusio/quarkus#13154, the `@TestProfile` annotation supports Native tests, therefore the modules 001, 301 and 303 can be extended for Native test coverage. As part of this work, we have supported also the test resources for Native: quarkusio/quarkus#18077 And also, I've removed the workaround for quarkusio/quarkus#16810 that had been fixed a while ago. Moreover, for 301: - We've fixed the native failures and got rid of the application.properties file in the test resources. This can be closed now: quarkusio/quarkus#17829. - Reduce the number of traces to ease the troubleshot of failures. And for 303: - Refactored the test resources to not rely on system properties to load/unload application properties.
## Description
In quarkusio/quarkus#13154, the annotation `@TestProfile` was supported also in Native tests. However, the native extension was not processing the test resources in the test profile like:
```
public class ConfluentTestProfile implements QuarkusTestProfile {
@OverRide
public String getConfigProfile() {
return "confluent";
}
@OverRide
public List<TestResourceEntry> testResources() {
return Collections.singletonList(new TestResourceEntry(ConfluentKafkaResource.class));
}
}
```
This PR makes the above to be supported.
The solution behaves the same as done in the Quarkus Test extension.
Fix #12974
Edit 2020-12-17:
This has two parts:
ConfigChangeRecordercan now be configured to make the application fail at boot if the build time config changed. (Originally it only warned, which is still the default). The new mode is used by theNativeTestExtensionto make sure that native tests are run with the same build time config as was used during the compilation.@TestProfilesupport in theNativeTestExtension.This PR originally included building of multiple native images based on
%profile.quarkus.package.output-name. That part was left out based on reviews.Original description before 2020-12-17
This has several parts:
ConfigChangeRecorder can now be configured to make the application fail at boot if the build time config changed. (Originally it only warned, which is still the default). The new mode is used by the NativeTestExtension to make sure that native tests are run against the right native image.
BuildMojo builds a separate native image for each profile found in application.properties that has quarkus.package.output-name property set.
NativeTestExtension can start the right one of the images built by BuildMojo, passing the configuration from TestProfile to it.
TODO: