Improvements in native.surefire.skip usage to skip unit tests with -Dnative

[zakkak]

Fixes the issue described in #10791 (comment)

[zakkak]

I moved:

<native.surefire.skip>${skipTests}</native.surefire.skip>

inside each integration test's pom.xml to be consistent with future integration tests generated using the create-extension mojo.

[zakkak]

This however does not seem to work because in:

                         <configuration>
                             <native.surefire.skip>${skipTests}</native.surefire.skip>
                             <skipTests>${native.surefire.skip}</skipTests>
                         </configuration>

native.surefire.skip will (at least the way I understand it) be assigned the value of skipTests, but in the next line skipTests will not be assigned the freshly assigned value of native.surefire.skip.

As a result I think that

<native.surefire.skip>${skipTests}</native.surefire.skip>

must be in a parent pom file of the pom that includes:

<skipTests>${native.surefire.skip}</skipTests>

so that it can get prioritized.

[zakkak]

cc @aloubyansky

[aloubyansky]

Yes, this change alone doesn't look right. I know there was a create-extension issue but keep in mind that that integration-test-pom.xml template is a Quarkus-specific part. create-extension however is something that can be used outside of the quarkus repo.

[gsmet]

The commit message shouldn't have the fixup! prefix.

[zakkak]

@aloubyansky In that case, if integration-test-pom.xml template is a Quarkus-specific part then I think that #10791 is OK and we don't need any further patching, since in Quarkus (#10791) <native.surefire.skip>${skipTests}</native.surefire.skip> is included in integration-tests/pom.xml.

[aloubyansky]

Yes, i think you can leave it for now. BTW, couldn't we move

                    <plugin>
                        <groupId>org.apache.maven.plugins</groupId>
                        <artifactId>maven-surefire-plugin</artifactId>
                        <configuration>
                            <skipTests>${native.surefire.skip}</skipTests>
                        </configuration>
                    </plugin>

to the pluginManagement of the parent pom (in the profile)?

[zakkak]

@aloubyansky I am not sure we can. We want this to be enabled only for the native-image profile.

[aloubyansky]

I thought we could add pluginManagement to the native profile. Or even configure the plugin in the native profile and customize the configs where necessary in specific test projects. But perhaps that won't work, i'm not sure. I thought i'd ask whether you know.

[zakkak]

I am not that familiar with maven so I will have to give it a try and come back :)

[aloubyansky]

Great, please do :)

[gsmet]

Just a note that the commit message and the PR title need some love. We don't want any fixup! in there.

[gsmet]

I hate to say that and I'm sorry about it but my opinion on this patch (and given the discussion that took place in the other thread) is that it's not really worth the added complexity and the time spent on it.

I think your time would be more valuable on other more pressing issues.

Just my 2 cents though, others may have different opinion.

[zakkak]

@gsmet the way I see it this is not "spent time" for the following reasons:

• It reduces the run time of native-image CI jobs by 2% to 8% as shown in Introduces native.surefire.skip to skip unit tests with -Dnative #10791 (comment) (note that Mandrel runs only native-image tests, so the benefits there are more obvious)
• It reduces the time spent to run native-image integration tests locally
• it's an exercise that helps me better understand how maven works, which I need to in order to achieve my end goal of making quarkus testing more "portable" as discussed in https://groups.google.com/forum/#!searchin/quarkus-dev/github$20ci%7Csort:date/quarkus-dev/_KZe1ydg4C0/tzazd4b0CAAJ.

Just a note that the commit message and the PR title need some love. We don't want any fixup! in there.

Noted. Once we conclude our discussions/experiments this PR will either get out of Draft mode and those will get fixed, or just get closed.

[famod]

@zakkak
Just to clearly state this (but maybe I am just too confused by this change): This has no effect whatsoever.
The custom configuration element (= plugin parameter) native.surefire.skip is nothing that maven-surefire-plugin is aware of and therefore it is simply ignored.

See test goal parameter reference: https://maven.apache.org/surefire/maven-surefire-plugin/test-mojo.html

@aloubyansky is on the right track:

I thought we could add pluginManagement to the native profile. Or even configure the plugin in the native profile and customize the configs where necessary in specific test projects. But perhaps that won't work, i'm not sure.

As long as all jarsubmodules under integration-tests shall execute native tests when the profile is active, the most DRY solution would be to add the profile to the parent (integration-tests), but not with pluginManagement (see paragraph at the bottom).
This way we could drop most of the native-profile definitions from the submodules.
The ones that need special config would need to either define the profile (with activation!) and add their parts to the plugin (and inherit the other defaults) or a few custom properties at parent level would need to be added so that the submodules in question could override those properties.

It becomes more complicated if some jar-submodules should not execute/test any native stuff.
You cannot "un-inherit" a profile from a parent. You could only add "skip" properties for all plugins in that profile.
Or you could add a "negative" file activation to the profile in the parent so that a submodule can place a marker file so that the profile is disabled for it.
Not very pretty, but that is how the (limited) profile mechanism works.

Downside of pluginManagement:

• you need to redefine the version
• you still need to define the profile in each submodule no, that would work

[zakkak]

@famod Thanks for the detailed explanation, I'll try that then.

@zakkak
Just to clearly state this (but maybe I am just too confused by this change): This has no effect whatsoever.
The custom configuration element (= plugin parameter) native.surefire.skip is nothing that maven-surefire-plugin is aware of and therefore it is simply ignored.

See test goal parameter reference: https://maven.apache.org/surefire/maven-surefire-plugin/test-mojo.html

Note that surefire is already (by #10791) made aware of this in the integration-tests/*/pom.xml files, e.g.

quarkus/integration-tests/amazon-lambda-http/pom.xml

Lines 90 to 92 in 05a0ec5

	<configuration>
	<skipTests>${native.surefire.skip}</skipTests>
	</configuration>

[gsmet]

@zakkak the problem is that it also requires other people attention such as Alexey's and again I don't think it's worth it.

As we mentioned it with Georgios, locally, we want to run the Java tests before the native ones because it's a safe check that avoids waiting for 5 minutes if there's an obvious issue.

That lets us with CI and IMHO the benefit is not really worth it.

[zakkak]

As we mentioned it with Georgios, locally, we want to run the Java tests before the native ones because it's a safe check that avoids waiting for 5 minutes if there's an obvious issue.

FWIW this is still the case since this is an opt-in "feature" where one needs to use -Dnative.surefire.skip to skip the tests.

[zakkak]

@aloubyansky done (there were some conflicts, so please make sure to review once more)

[aloubyansky]

@gsmet it's in your hands

[zakkak]

@gsmet are there any standing issues with this PR?

[zakkak]

@gsmet this one got away, should I rebase it to get it merged?

[aloubyansky]

@gsmet is on PTO this week. Good question though.

[zakkak]

@gsmet this is a kind reminder

[gastaldi]

@zakkak go ahead and rebase it so we can merge it

[zakkak]

@gastaldi this is now rebased :)

[gastaldi]

@zakkak weird, I still see conflicts in the PR. Are you sure you have rebased on the latest master?

[zakkak]

@zakkak weird, I still see conflicts in the PR. Are you sure you have rebased on the latest master?

Sorry for that, it looks like I only fetched and never pulled master :/

Should be OK now

[zakkak]

@gastaldi this is a kind reminder

[gastaldi]

@zakkak sorry, that fell through the cracks. It looks good to me

[gastaldi]

@gsmet I'll let you push the Merge button once your concerns are addressed

[zakkak]

Hello, should I proceed with rebasing?

[famod]

I must have forgotten about this! I'm in the process of unifying the native profile in #14501 which definitely clashes with this one...

[zakkak]

I must have forgotten about this! I'm in the process of unifying the native profile in #14501 which definitely clashes with this one...

Nice, I am closing this one in favour of #14501 then :)