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

Fix build-time analytics module execution failure as we should only set QE FW test properties in the test properties #2293

Conversation

michalvavrik
Copy link
Member

@michalvavrik michalvavrik commented Feb 3, 2025

Summary

As result of the quarkus-qe/quarkus-test-framework#791 we expect that all QE FW properties are listed as enum constants in Property, however in past it was possible to set also Quarkus properties as ts.global, which now results in failure. Due to this issue, daily build fails https://github.com/quarkus-qe/quarkus-test-suite/actions/runs/13111089359/job/36575004262 over [ERROR] java.util.ServiceConfigurationError: org.junit.platform.launcher.TestExecutionListener: Provider io.quarkus.test.listener.QuarkusTestResourceCleaningFilter could not be instantiated. I have debugged it and found out that everytime new PropertyLookup constructor is instantiated in this module, loading of the global configuration fails as no such element.

This is expected to fail until quarkus-qe/quarkus-test-framework#1485 is merged and this PR CI is re-run.

Please select the relevant options.

  • Bug fix (non-breaking change which fixes an issue)
  • Dependency update
  • Refactoring
  • Backport
  • New scenario (non-breaking change which adds functionality)
  • This change requires a documentation update
  • This change requires execution against OCP (use run tests phrase in comment)

Checklist:

  • Methods and classes used in PR scenarios are meaningful
  • Commits are well encapsulated and follow the best practices

@michalvavrik michalvavrik requested review from fedinskiy and removed request for fedinskiy February 3, 2025 13:51
@michalvavrik
Copy link
Member Author

Sorry, I haven't seen failures as I had cached page result, I need to inspect them first.

@michalvavrik michalvavrik marked this pull request as draft February 3, 2025 13:52
@michalvavrik michalvavrik force-pushed the feature/fix-build-time-analytics-execution branch from f3875a4 to b49baf7 Compare February 3, 2025 14:15
@michalvavrik michalvavrik marked this pull request as ready for review February 3, 2025 14:15
@michalvavrik michalvavrik force-pushed the feature/fix-build-time-analytics-execution branch from b49baf7 to 3288975 Compare February 3, 2025 14:33
@michalvavrik
Copy link
Member Author

I think the property has to be global, so service scope won't cut it. I added application.properties file which should be most precise solution.

fedinskiy
fedinskiy previously approved these changes Feb 3, 2025
Copy link
Contributor

@fedinskiy fedinskiy left a comment

Choose a reason for hiding this comment

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

Thank you!

@michalvavrik
Copy link
Member Author

Thank you!

thanks, yeah, I think CLI takes application properties from a different file or source. I had to go so I pushed it to see CI, I'll just find where CI is taking app properties from and fix it.

@michalvavrik
Copy link
Member Author

I think either we stop relying on global scope and use service context for the property lookup, or we need to set system property and it still will be just a trick. I'll provide a FW PR and once we agree on solution, I'll update this one.

@michalvavrik michalvavrik dismissed fedinskiy’s stale review February 3, 2025 16:52

thanks, but there will be changes

@michalvavrik michalvavrik marked this pull request as draft February 3, 2025 16:52
@michalvavrik
Copy link
Member Author

hey @fedinskiy , thanks for the first review. Now CI is green, let's have a look. Thanks

@fedinskiy fedinskiy merged commit 0c5f2b5 into quarkus-qe:main Feb 4, 2025
7 checks passed
@michalvavrik michalvavrik deleted the feature/fix-build-time-analytics-execution branch February 4, 2025 12:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants