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

Enable cacheability of surefire and failsafe goals #35033

Merged
merged 2 commits into from
Jul 27, 2023

Conversation

jprinet
Copy link
Contributor

@jprinet jprinet commented Jul 26, 2023

Issue

Most of the maven-surefire-plugin and maven-failsafe-plugin test goal executions rely on SystemPropertiesVariables with paths as values, see here for an example.
If not declared as inputs or ignored, the goal is marked as not cacheable.

Screenshot 2023-07-26 at 2 26 39 PM

The cost is 1h17mn of CPU time in my local experiment for surefire and 7mn42s for failsafe

Screenshot 2023-07-26 at 2 49 04 PM

Fix

Configure the test goal executions to ignore the SystemPropertiesVariables maven.repo.local. The local repository can't be identical across builds if running install goal, and fingerprinting it would lead to very large build scans (not even considering the time it would take).
The same goes for git.dir which does not make sense to add as an input.

To be discussed

Some other inputs have been ignored but could be added as inputs, this can be discussed case by case while reviewing the PR. The question being, is it ok to get a cache hit (meaning not re-execute the tests) if something changed in one of those folders:

  • vale.dir
  • org.jboss.cdi.tck.libraryDirectory
  • native.image.path
  • quarkus.kubernetes-service-binding.root
  • javax.net.ssl.trustStore
  • rest-client.trustStore
  • classpathEntriesRecordingFile

@quarkus-bot quarkus-bot bot added area/arc Issue related to ARC (dependency injection) area/dependencies Pull requests that update a dependency file area/documentation area/graphics labels Jul 26, 2023
@quarkus-bot
Copy link

quarkus-bot bot commented Jul 26, 2023

/cc @Karm (awt), @galderz (awt), @zakkak (awt)

@gsmet gsmet force-pushed the fix/ignore-maven-local-as-test-input branch from 9c14c3b to 871a47b Compare July 26, 2023 15:33
native.image.path is used for all IT modules so we should handle it in
the parent.
@gsmet
Copy link
Member

gsmet commented Jul 26, 2023

@jprinet could you have a look at the commit I added? The AWT config was needlessly verbose as everything should be handled by the parent. But, that being said, I think the configuration you added there should have been added to the parent as we are defining native.image.path in the parent.

Let me know if this works for you and I'll merge once CI is green.

@quarkus-bot
Copy link

quarkus-bot bot commented Jul 26, 2023

Failing Jobs - Building 9f3ba19

Status Name Step Failures Logs Raw logs
✔️ JVM Tests - JDK 11
JVM Tests - JDK 17 Build Failures Logs Raw logs
✔️ JVM Tests - JDK 19

Full information is available in the Build summary check run.

Failures

⚙️ JVM Tests - JDK 17 #

- Failing: integration-tests/grpc-hibernate 

📦 integration-tests/grpc-hibernate

com.example.grpc.hibernate.VertxBlockingMutinyTest.shouldAddItems - More details - Source on GitHub

io.smallrye.mutiny.CompositeException: 
Multiple exceptions caught:
	[Exception 0] io.grpc.StatusRuntimeException: INTERNAL: Half-closed without a request

@jprinet
Copy link
Contributor Author

jprinet commented Jul 27, 2023

@jprinet could you have a look at the commit I added? The AWT config was needlessly verbose as everything should be handled by the parent. But, that being said, I think the configuration you added there should have been added to the parent as we are defining native.image.path in the parent.

Let me know if this works for you and I'll merge once CI is green.

Confirming that the outcome is identical 👍

Please check before merging that the following ignored systemPropertiesVariable are safe to be ignored while computing the build cache key:

  • vale.dir
  • org.jboss.cdi.tck.libraryDirectory
  • native.image.path
  • quarkus.kubernetes-service-binding.root
  • javax.net.ssl.trustStore
  • rest-client.trustStore
  • classpathEntriesRecordingFile

@gsmet
Copy link
Member

gsmet commented Jul 27, 2023

I'm not completely sure about classpathEntriesRecordingFile, we will have to discuss this with @aloubyansky . But let's merge this so that we can make progress with the experiment. I'll rebase my build cache PR on top of this and run it twice.

@gsmet gsmet merged commit 340b9a8 into quarkusio:main Jul 27, 2023
@quarkus-bot quarkus-bot bot added this to the 3.3 - main milestone Jul 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/arc Issue related to ARC (dependency injection) area/dependencies Pull requests that update a dependency file area/documentation area/graphics
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants