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

Make URL inherit TestHTTPEndpoint from class level #46157

Merged

Conversation

gian1200
Copy link
Contributor

@gian1200 gian1200 commented Feb 8, 2025

Closes #34935
Continuation of #35095

Functionality:

If a Test Class is annotated with @TestHTTPEndpoint then all URL fields inherit it's configuration.
If URL is also annotated, URL's annotation takes precedence.

Local tests (Windows):

Additional notes:

I couldn't find the tests for original functionality, so I included both (new and original).
I reused some resources/endpoints. Not sure if that's acceptable/expected or not (hopefully it doesn't make other tests fail)

This comment was marked as resolved.

@gian1200 gian1200 changed the title feat: make URL inherit TestHTTPEndpoint from class level Draft: feat: make URL inherit TestHTTPEndpoint from class level Feb 8, 2025
@gian1200 gian1200 marked this pull request as draft February 8, 2025 21:40
@gian1200 gian1200 changed the title Draft: feat: make URL inherit TestHTTPEndpoint from class level Draft: Make URL inherit TestHTTPEndpoint from class level Feb 8, 2025
@gian1200 gian1200 changed the title Draft: Make URL inherit TestHTTPEndpoint from class level Make URL inherit TestHTTPEndpoint from class level Feb 8, 2025
@gian1200 gian1200 force-pushed the feature/testhttpendpoint-inheritance branch from 8c4075e to f60ff81 Compare February 8, 2025 21:44
@geoand
Copy link
Contributor

geoand commented Feb 9, 2025

Seems reasonable!

@gian1200 gian1200 force-pushed the feature/testhttpendpoint-inheritance branch from f60ff81 to c75ead2 Compare February 16, 2025 02:22
@gian1200 gian1200 marked this pull request as ready for review February 16, 2025 02:47
Copy link

quarkus-bot bot commented Feb 17, 2025

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit c75ead2.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

Copy link
Contributor

@geoand geoand left a comment

Choose a reason for hiding this comment

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

Thanks!

@geoand geoand merged commit 3555cbc into quarkusio:main Feb 17, 2025
55 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.21 - main milestone Feb 17, 2025
@quarkus-bot quarkus-bot bot added the kind/enhancement New feature or request label Feb 17, 2025
@geoand
Copy link
Contributor

geoand commented Feb 18, 2025

I am actually going to revert this because it can cause issues like #46336, meaning it can break existing tests and users have no way to recover.

Sorry I didn't anticipate this earlier

@gian1200
Copy link
Contributor Author

Sure, no problem. I didn't either 😔. A simple type validation should fix it (only inject URL fields). Will see if I can do it this weekend (unless someone is willing to do it earlier 🙃)

PS: during my testing I noticed that injection happens once for each test, not necessarily once per class (eg. 2 times for 2 tests in the same class). Is this expected or should I open an issue? It feels inefficient.

@geoand
Copy link
Contributor

geoand commented Feb 18, 2025

A simple type validation should fix it (only inject URL fields). Will see if I can do it this weekend (unless someone is willing to do it earlier 🙃)

I know, but that is unfortunately not enough. There could very well be tests in the wild that use URL (or String) as a field that is set by the test itself. Those cases would now break.

@gian1200
Copy link
Contributor Author

Made a new PR. Limited it's scope to only inject field when @TestHTTPResource is explicitly use (AS-IS functionality).
Also added additional tests to cover #46336 and similar.

PR: #46410

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/testing kind/enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make TestHTTPEndpoint at class level affect all URL fields in test classes
2 participants