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.

Sign up for GitHub

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

Jump to bottom

Unification of all test-framework-related options #791

Merged
merged 1 commit into from
Feb 1, 2025
Merged

Unification of all test-framework-related options #791

merged 1 commit into from
Feb 1, 2025

Conversation

fedinskiy
Copy link
Contributor

@fedinskiy fedinskiy commented May 24, 2023 •
edited
Loading

Summary

Since all of them are now gathered in one place
it would become easier to monitor and document them
Required for potential automation of #369
Future changes in test suite require accommodation of #309

Please check the relevant options

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Dependency update
  • Refactoring
  • Breaking change (fix or feature that would cause existing functionality to change)
  • This change requires a documentation update
  • This change requires execution against OCP (use run tests phrase in comment)

Checklist:

  • Example scenarios has been updated / added
  • Methods and classes used in PR scenarios are meaningful
  • Commits are well encapsulated and follow the best practices

@fedinskiy
Copy link
Contributor Author

run tests

@fedinskiy fedinskiy force-pushed the feature/properties-unification branch 5 times, most recently from a7c6afa to 37a5005 Compare May 29, 2023 07:02
@fedinskiy fedinskiy force-pushed the feature/properties-unification branch 2 times, most recently from 70fd173 to 61233bd Compare July 24, 2023 13:18
@fedinskiy
Copy link
Contributor Author

run tests

@fedinskiy fedinskiy force-pushed the feature/properties-unification branch from 61233bd to 7b85220 Compare July 26, 2023 08:25
@fedinskiy
Copy link
Contributor Author

run tests

@fedinskiy fedinskiy force-pushed the feature/properties-unification branch from 7b85220 to bf6a131 Compare July 31, 2023 06:12
@mocenas
Copy link
Contributor

mocenas commented Oct 31, 2023

Is this PR still active / relevant @fedinskiy ?

@fedinskiy
Copy link
Contributor Author

@mocenas this MR is relevant, but not very active.

@fedinskiy fedinskiy force-pushed the feature/properties-unification branch 2 times, most recently from 2564ceb to 8f7a931 Compare March 20, 2024 14:14
@fedinskiy
Copy link
Contributor Author

run tests

1 similar comment
@fedinskiy
Copy link
Contributor Author

run tests

@michalvavrik michalvavrik mentioned this pull request May 12, 2024
Open
@QuarkusQE
Copy link
Contributor

Can one of the admins verify this patch?

@rsvoboda
Copy link
Member

@mjurc @michalvavrik @fedinskiy what about this effort?

@michalvavrik
Copy link
Member

@mjurc @michalvavrik @fedinskiy what about this effort?

framework has changed a little, I think it requires additional work and I doubt @fedinskiy has time for it; let's hear from him

@fedinskiy
Copy link
Contributor Author

I am working on this, when I have time, lately I didn't have any. Are we in a hurry with this?

@rsvoboda
Copy link
Member

I was just wondering if this is abandoned or still on radar.

@fedinskiy fedinskiy force-pushed the feature/properties-unification branch 2 times, most recently from 7f1cffe to e4071fa Compare July 29, 2024 06:56
@fedinskiy fedinskiy force-pushed the feature/properties-unification branch 2 times, most recently from 3b757a3 to ea96b56 Compare January 14, 2025 13:37
@michalvavrik
Copy link
Member

@fedinskiy please update this PR with a property added in the #1467. Thank you

@michalvavrik
Copy link
Member

I have finished first review, I'll need answers to my comments and have a second look. I think overall it looks very good and there is very little to change, but I am nervous about service-specific properties being gone, I'll wait for replies and think about it a lot.

@fedinskiy fedinskiy force-pushed the feature/properties-unification branch 2 times, most recently from ffe59eb to 145b078 Compare January 28, 2025 13:20
@rsvoboda rsvoboda requested a review from mjurc January 28, 2025 13:37
@fedinskiy
Copy link
Contributor Author

The jenkins tests are green, but they are failing, for unclear, but PR-related reasons. Will investigate further

@fedinskiy fedinskiy force-pushed the feature/properties-unification branch from 145b078 to 36f0cc1 Compare January 29, 2025 10:04
@fedinskiy
Copy link
Contributor Author

run tests

@michalvavrik
Copy link
Member

michalvavrik commented Jan 29, 2025 •
edited
Loading

friendly reminder @fedinskiy , I can see you are pushing changes and running OpenShift tests, but I still believe you must address my comment on the OpenShift Funqy regarding system properties. Thanks

@fedinskiy fedinskiy force-pushed the feature/properties-unification branch 2 times, most recently from 8d8d3a2 to 3546ff8 Compare January 30, 2025 14:59
@fedinskiy
Copy link
Contributor Author

run tests

@fedinskiy
Unification of all test-framework-related options
50bf10b 
Since all of them are now gathered in one place
it would become easier to monitor and document them
Required for potential automation of quarkus-qe#369
@fedinskiy fedinskiy force-pushed the feature/properties-unification branch from 3546ff8 to 50bf10b Compare January 31, 2025 14:52
@fedinskiy
Copy link
Contributor Author

run tests

michalvavrik
michalvavrik approved these changes Jan 31, 2025
View reviewed changes
Copy link
Member

@michalvavrik michalvavrik left a comment

Choose a reason for hiding this comment

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

Good job. Thanks.

@michalvavrik
Copy link
Member

We need @mjurc && green OCP. Then this can be merged.

@michalvavrik
Copy link
Member

BTW I'd also recommend to check TS, but it's no biggie, IMHO we can just fix it in follow-ups, this is big enough.

mjurc
mjurc approved these changes Feb 1, 2025
View reviewed changes
Copy link
Member

@mjurc mjurc left a comment

Choose a reason for hiding this comment

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

LGTM

@mjurc
Copy link
Member

mjurc commented Feb 1, 2025

Native failure unrelated; merging

@mjurc mjurc merged commit c8b821f into quarkus-qe:main Feb 1, 2025
6 of 7 checks passed
@michalvavrik michalvavrik mentioned this pull request Feb 3, 2025
Merged
9 tasks
michalvavrik
michalvavrik reviewed Feb 3, 2025
View reviewed changes
quarkus-test-core/src/main/java/io/quarkus/test/configuration/Configuration.java
// There is no properties file: this is not mandatory.
} catch (Exception exception) {
if (exception instanceof NullPointerException && exception.getMessage().equals("inStream parameter is null")) {
System.err.println("No properties file: " + propertiesFile);
Copy link
Member

Choose a reason for hiding this comment

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

I just saw No properties file: test.properties when running mvn clean verify -Dit.test=MssqlHandlerIT in quarkus-test-suite/sql-db/vertx-sql. I am not on a fence, I didn't realize this can be normal situation that there are no test properties, but on the other hand, the message is not noisy and does not indicate something went wrong. We should probably keep it as is 🤷‍♂️

This was referenced Feb 3, 2025
Merged
Merged
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michalvavrik michalvavrik michalvavrik approved these changes

@mjurc mjurc mjurc approved these changes

Assignees
No one assigned
Labels
release/breaking-change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@fedinskiy @mocenas @QuarkusQE @rsvoboda @michalvavrik @mjurc