armeria icon indicating copy to clipboard operation
armeria copied to clipboard

Add verifications of `ClientRequestContext`s to `RetryClientTest` and `RetryingRpcClientTest`

Open schiemon opened this issue 6 months ago • 3 comments

Motivation:

RetryingClientTest and RetryingRpcClientTest validate the respective retrying clients under various (failure) conditions.
However, not all test cases check whether the client-side request log is properly propagated by the client, e.g., whether:

  • the log is completed,
  • all child logs are added and completed as well,
  • all child logs have the expected status code and exception.

Verifying the logs becomes even more important for #6252, where the retrying client must ensure it properly completes concurrent pending requests—either when a response is received or when a request fails unrecoverably, thereby also failing all other pending requests. Aforementioned tests would increase our confidence that the upcoming hedging client does not cause regressions.

Modifications:

All changes are test-only.

  • Introduced a small testing utility (RequestContextUtils) for verifying ClientRequestContexts, especially the log, in a concise way.
  • Extended test cases in RetryingClientTest and RetryingRpcClientTest by incorporating verifications using that utility class.

Result:

  • 26 + 10 test cases now also verify ClientRequestContext under various failure scenarios.
  • RequestContextUtils now helps in writing concise tests that validate ClientRequestContext, e.g., for request hedging.

schiemon avatar Jun 25 '25 15:06 schiemon

Codecov Report

:x: Patch coverage is 79.06977% with 18 lines in your changes missing coverage. Please review. :white_check_mark: Project coverage is 74.56%. Comparing base (8150425) to head (b4be00e). :warning: Report is 195 commits behind head on main.

Files with missing lines Patch % Lines
.../armeria/internal/testing/RequestContextUtils.java 79.06% 13 Missing and 5 partials :warning:
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #6296      +/-   ##
============================================
+ Coverage     74.46%   74.56%   +0.10%     
- Complexity    22234    22611     +377     
============================================
  Files          1963     2022      +59     
  Lines         82437    83734    +1297     
  Branches      10764    10866     +102     
============================================
+ Hits          61385    62439    +1054     
- Misses        15918    16094     +176     
- Partials       5134     5201      +67     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

codecov[bot] avatar Jun 25 '25 15:06 codecov[bot]

🔍 Build Scan® (commit: b4be00e93044f9cfb231ec112dea8779ddaffdb6)

Job name Status Build Scan®
build-ubicloud-standard-16-jdk-8 ❌ (failure) https://ge.armeria.dev/s/dqghra67fjtsk
build-ubicloud-standard-16-jdk-21-snapshot-blockhound ❌ (failure) https://ge.armeria.dev/s/rrguexm23oxyg
build-ubicloud-standard-16-jdk-17-min-java-17-coverage https://ge.armeria.dev/s/znpkolwzojiuu
build-ubicloud-standard-16-jdk-17-min-java-11 https://ge.armeria.dev/s/quaafphnrn4qk
build-ubicloud-standard-16-jdk-17-leak https://ge.armeria.dev/s/oa67ss5pwg76s
build-ubicloud-standard-16-jdk-11 https://ge.armeria.dev/s/sg6tap2frnvh4
build-macos-latest-jdk-21 https://ge.armeria.dev/s/yk3uusguvq5kq

github-actions[bot] avatar Jun 25 '25 16:06 github-actions[bot]

@jrhee17 Thank you for the review 🤙 . I addressed your comments. I am not sure if you like me to close the comments myself or leave them open. Feel free to educate me if there some Armeria-wide convention for this

schiemon avatar Jul 02 '25 11:07 schiemon