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

Wrote WebClientHttpRoutingFilterTests. Relates to GH-73 #3334

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

Conversation

NadChel
Copy link
Contributor

@NadChel NadChel commented Apr 2, 2024

I ran a coverage tool and noticed that one of the key GlobalFilers, WebClientHttpRoutingFilter, has a coverage of zero (at least, excluding integration tests). I bumped it up to 100%

Also wrote a missing Javadoc

Important note. I want you to delay the merge until we figure out how to assert on ClientResponse's body. The last test, filter_ifNotRouted_ifSchemeHttps_receivedResponseStoredAsAttribute(), was supposed to have this assertion as well

    StepVerifier.create(storedClientResponse.bodyToMono(String.class))
        .expectNext(responseBody)
        .verifyComplete();

However, the body mono completes immediately so the assertion fails. I tried to pinpoint it on my own for a while, but then remembered that I no longer work on some pet project of mine and can continue solving that puzzle together with the Spring Cloud community. Maybe, it's not a puzzle at all

@NadChel NadChel force-pushed the test/GH-73_test_for_WebClientHttpRoutingFilter branch from f159d8c to 8a65834 Compare April 3, 2024 06:28
@spencergibb
Copy link
Member

Yeah, there's a reason it's not tested. I'm likely going to deprecate it for removal.

@NadChel
Copy link
Contributor Author

NadChel commented Apr 11, 2024

Do you mean deprecating WebClientHttpRoutingFilter or ClientResponse?

@spencergibb
Copy link
Member

WebClientHttpRoutingFilter

@NadChel
Copy link
Contributor Author

NadChel commented Apr 11, 2024

Well, but shouldn't it be covered while it's still part of the framework?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants