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

schiemon · 2025-06-25T15:03:15Z

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.

codecov · 2025-06-25T15:24:42Z

Codecov Report

❌ Patch coverage is 79.06977% with 18 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.56%. Comparing base (8150425) to head (b4be00e).
⚠️ 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 ⚠️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

github-actions · 2025-06-25T16:08:44Z

🔍 Build Scan® (commit: `b4be00e`)

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

…est`

…ntextUtils`

jrhee17

I appreciate the additional validation in retrying tests. I've reviewed mainly RequestContextUtils mainly considering how we can use this utility more extensively within the code-base.

jrhee17 · 2025-07-02T08:55:21Z

testing-internal/src/main/java/com/linecorp/armeria/internal/testing/RequestContextUtils.java

+        if (childLogVerifiers.length == 0) {
+            childLogVerifiers = new RequestLogVerifier[ctx.log().children().size()];
+            Arrays.fill(childLogVerifiers, VERIFY_NOTHING);
+        }
+
+        assertValidRequestContextWithVerifier(
+                ctx,
+                childLogVerifiers.length == 0 ?
+                VERIFY_NOTHING
+                        : verifyAllValid(
+                        childLog -> {
+                            // Default parent log verifier.
+                            final HttpRequest req = ctx.request();
+                            assertThat(req).isNotNull();
+                            assert req != null;
+                            assertThat(req.isComplete()).isTrue();
+
+                            if (ctx.rpcRequest() != null) {
+                                final HttpRequest lastHttpReq =
+                                        ctx.log().children()
+                                           .get(ctx.log().children().size() - 1).context().request();
+
+                                if (lastHttpReq != null) {
+                                    assertThat(lastHttpReq).isSameAs(ctx.log().context().request());
+                                }
+                            }
+                        },
+                        childLogVerifiers[childLogVerifiers.length - 1]
+                ), childLogVerifiers
+        );


Question) I was a little surprised this validation only exists in this overriding variant. Would it make sense that this method simply calls assertValidRequestContextWithVerifier without the parent verifier?

Can the lastHttpReq check be included in the method at L180, or provided as a separate verifyLastChildHasSameHttpRequest() verifier if necessary?

e.g.

Suggested change

if (childLogVerifiers.length == 0) {

childLogVerifiers = new RequestLogVerifier[ctx.log().children().size()];

Arrays.fill(childLogVerifiers, VERIFY_NOTHING);

}

assertValidRequestContextWithVerifier(

ctx,

childLogVerifiers.length == 0 ?

VERIFY_NOTHING

: verifyAllValid(

childLog -> {

// Default parent log verifier.

final HttpRequest req = ctx.request();

assertThat(req).isNotNull();

assert req != null;

assertThat(req.isComplete()).isTrue();

if (ctx.rpcRequest() != null) {

final HttpRequest lastHttpReq =

ctx.log().children()

.get(ctx.log().children().size() - 1).context().request();

if (lastHttpReq != null) {

assertThat(lastHttpReq).isSameAs(ctx.log().context().request());

}

}

},

childLogVerifiers[childLogVerifiers.length - 1]

), childLogVerifiers

);

assertValidRequestContextWithVerifier(ctx,VERIFY_NOTHING, childLogVerifiers);

There are situations where the caller wants to override the default parent log verifier so that is why it was only at that place and not integrated further dowstream.

I reflected on this and I think it is best to extract those checks into separate verifiers as you said and just always demand from the caller to provide a parent log verifier. The advantage of that is that the test are then more explicit on what to expect from the parent log which is less given with a hidden default parent log verifier.

jrhee17 · 2025-07-02T08:56:09Z

testing-internal/src/main/java/com/linecorp/armeria/internal/testing/RequestContextUtils.java

+
+    @FunctionalInterface
+    public interface RequestLogVerifier {
+        void verifyChildLog(RequestLog childLog) throws Exception;


nit; I wonder if it makes sense to drop the child prefix since the validation can also be applied to the parent log.

Suggested change

void verifyChildLog(RequestLog childLog) throws Exception;

void verifyLog(RequestLog requestLog) throws Exception;

jrhee17 · 2025-07-02T09:13:24Z

testing-internal/src/main/java/com/linecorp/armeria/internal/testing/RequestContextUtils.java

+        };
+    }
+
+    public static RequestLogVerifier verifyExactlyOneValid(RequestLogVerifier... childLogVerifiers) {


Maybe just me, but I was imagining exactly one child is valid for all of the verifiers when looking at the name

Suggested change

public static RequestLogVerifier verifyExactlyOneValid(RequestLogVerifier... childLogVerifiers) {

public static RequestLogVerifier verifyExactlyOneVerifierValid(RequestLogVerifier... childLogVerifiers) {

jrhee17 · 2025-07-02T09:15:25Z

testing-internal/src/main/java/com/linecorp/armeria/internal/testing/RequestContextUtils.java

+        if (expectedNumRequests == 0) {
+            return;
+        }


Question) What do you think of removing this restriction? I still think this method can be useful in validating parentLogVerifier

jrhee17 · 2025-07-02T09:19:34Z

core/src/test/java/com/linecorp/armeria/client/retry/RetryingClientTest.java

+        final WebClient client = client(rule,
+                                        10000, 0, 100);


nit;

Suggested change

final WebClient client = client(rule,

10000, 0, 100);

final WebClient client = client(rule, 10000, 0, 100);

…Valid`

schiemon · 2025-07-02T11:35:42Z

@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

…ient-test

test: verify valid client request context after retrying test

42bcec6

schiemon mentioned this pull request Jun 25, 2025

feat: support request hedging #6252

Draft

6 tasks

schiemon added 2 commits June 25, 2025 22:47

test: add more detailed verification of child logs

a6b77a4

test: add client request context verifications to `RetryingRpcClientT…

fd2f768

…est`

schiemon changed the title ~~Add verifications of ClientRequestContexts to RetryClientTest~~ Add verifications of ClientRequestContexts to RetryClientTest and RetryingRpcClientTest Jun 26, 2025

schiemon added 2 commits June 26, 2025 12:17

refactor: remove unused logQueue from RetryingRpcClientTest

7c17e7b

refactor: remove unused verifyRequestCause(Throwable) from `RequestCo…

2834c62

…ntextUtils`

schiemon marked this pull request as ready for review June 26, 2025 10:22

schiemon requested review from ikhoon, jrhee17, minwoox and trustin as code owners June 26, 2025 10:22

jrhee17 reviewed Jul 2, 2025

View reviewed changes

schiemon added 7 commits July 2, 2025 12:39

refactor: rename verifyChildLog to verifyLog

243f0d8

refactor: rename verifyExactlyOneValid to `verifyExactlyOneVerifier…

d9a400a

…Valid`

refactor: remove skipping of verifiers when there are no child logs

fe47175

style: remove unnecessary line break

0841164

refactor: rename verifyAllValid to verifyAllVerifierValid

98b8aa6

refactor: enforce that users provide a parent log verifier

57b11ab

refactor: improve internal naming

5904886

schiemon requested a review from jrhee17 July 3, 2025 11:26

Merge branch 'main' into verify-client-request-context-in-retrying-cl…

b4be00e

…ient-test

jrhee17 added this to the 1.33.0 milestone Jul 31, 2025

jrhee17 added the improvement label Jul 31, 2025

ikhoon modified the milestones: 1.33.0, 1.34.0 Aug 1, 2025

github-actions bot added the Stale label Sep 1, 2025

github-actions bot removed the Stale label Sep 2, 2025

github-actions bot added the Stale label Oct 3, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

schiemon commented Jun 25, 2025 •

edited

Loading

Uh oh!

codecov bot commented Jun 25, 2025 •

edited

Loading

Uh oh!

github-actions bot commented Jun 25, 2025 •

edited

Loading

Uh oh!

jrhee17 left a comment

Uh oh!

jrhee17 Jul 2, 2025

Uh oh!

schiemon Jul 2, 2025 •

edited

Loading

Uh oh!

jrhee17 Jul 2, 2025

Uh oh!

jrhee17 Jul 2, 2025

Uh oh!

jrhee17 Jul 2, 2025

Uh oh!

jrhee17 Jul 2, 2025

Uh oh!

schiemon commented Jul 2, 2025 •

edited

Loading

Uh oh!

Uh oh!

	void verifyChildLog(RequestLog childLog) throws Exception;
	void verifyLog(RequestLog requestLog) throws Exception;

	public static RequestLogVerifier verifyExactlyOneValid(RequestLogVerifier... childLogVerifiers) {
	public static RequestLogVerifier verifyExactlyOneVerifierValid(RequestLogVerifier... childLogVerifiers) {

	final WebClient client = client(rule,
	10000, 0, 100);
	final WebClient client = client(rule, 10000, 0, 100);

Add verifications of ClientRequestContexts to RetryClientTest and RetryingRpcClientTest #6296

Are you sure you want to change the base?

Add verifications of ClientRequestContexts to RetryClientTest and RetryingRpcClientTest #6296

Uh oh!

Conversation

schiemon commented Jun 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation:

Modifications:

Result:

Uh oh!

codecov bot commented Jun 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions bot commented Jun 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔍 Build Scan® (commit: b4be00e)

Uh oh!

jrhee17 left a comment

Choose a reason for hiding this comment

Uh oh!

jrhee17 Jul 2, 2025

Choose a reason for hiding this comment

Uh oh!

schiemon Jul 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrhee17 Jul 2, 2025

Choose a reason for hiding this comment

Uh oh!

jrhee17 Jul 2, 2025

Choose a reason for hiding this comment

Uh oh!

jrhee17 Jul 2, 2025

Choose a reason for hiding this comment

Uh oh!

jrhee17 Jul 2, 2025

Choose a reason for hiding this comment

Uh oh!

schiemon commented Jul 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

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

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

schiemon commented Jun 25, 2025 •

edited

Loading

codecov bot commented Jun 25, 2025 •

edited

Loading

github-actions bot commented Jun 25, 2025 •

edited

Loading

🔍 Build Scan® (commit: `b4be00e`)

schiemon Jul 2, 2025 •

edited

Loading

schiemon commented Jul 2, 2025 •

edited

Loading