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

Tests for rest-client-reactive URL override feature #2275

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mocenas
Copy link
Contributor

@mocenas mocenas commented Jan 17, 2025

Summary

Implement test coverage for quarkusio/quarkus#43331

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

@mocenas mocenas requested a review from fedinskiy January 17, 2025 10:25
@mocenas mocenas force-pushed the test_rest_url_override branch from 5ff1054 to 299fead Compare January 17, 2025 11:36
import io.quarkus.test.services.QuarkusApplication;

@QuarkusScenario
public class UrlOverrideClientIT {
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. What if we use non-existent url for override? How good/terrible is the error?
  2. Does app writes overridden URL into log?

Copy link
Contributor Author

@mocenas mocenas Jan 17, 2025

Choose a reason for hiding this comment

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

  1. Trying to access non-existing URL causes exception on the service. It is possible to catch it and pretend it is OK, but that might cover some actual failures going on.
  2. No it does not not log rewriting URL.

Copy link
Contributor

Choose a reason for hiding this comment

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

  1. I thought, that maybe we should check that error, and that it really happens, to have some non-happy-path tests. Do not know, whether it is a good idea, but please think about it.
  2. And what about tracing? Is there any way for an observer to distinguish these calls?

@mocenas mocenas requested a review from fedinskiy January 17, 2025 13:07
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.

The existing tests are good, but please consider advanced topics suggested in the comment. If you they do not make much sense in this context — you're free to merge this issue.

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