Cleanup HttpServerTransport.Dispatcher in Netty tests #20160
Conversation
Signed-off-by: Sergei Ustimenko <[email protected]>
WalkthroughAdds TestDispatcherBuilder utilities for Netty4 and Reactor-Netty4 tests; refactors many tests to use the builder instead of inline anonymous HttpServerTransport.Dispatcher implementations; removes Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
🔇 Additional comments (3)
Comment |
Signed-off-by: Sergei Ustimenko <[email protected]>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
CHANGELOG.md (1)
75-75: Consider adding PR reference for consistencyNearby changelog entries all link the corresponding PR/issue; adding
([#20160](...))here would keep the section consistent and traceable, but is optional.modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.java (1)
28-30: Optionally mark the builder classfinalSince this is a focused test utility not intended for extension, you could make the class
finalto prevent accidental subclassing in tests:-public class TestDispatcherBuilder { +public final class TestDispatcherBuilder {Purely optional, but it better communicates intent and avoids surprising inheritance chains in test code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
CHANGELOG.md(1 hunks)modules/transport-netty4/src/test/java/org/opensearch/http/netty4/Netty4BadRequestTests.java(1 hunks)modules/transport-netty4/src/test/java/org/opensearch/http/netty4/Netty4HttpServerTransportTests.java(6 hunks)modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.java(1 hunks)modules/transport-netty4/src/test/java/org/opensearch/http/netty4/ssl/SecureNetty4HttpServerTransportTests.java(11 hunks)plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4BadRequestTests.java(1 hunks)plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4HttpServerTransportStreamingTests.java(3 hunks)plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4HttpServerTransportTests.java(8 hunks)plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.java(1 hunks)plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
modules/transport-netty4/src/test/java/org/opensearch/http/netty4/ssl/SecureNetty4HttpServerTransportTests.java (2)
modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.java (1)
TestDispatcherBuilder(28-103)plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.java (1)
TestDispatcherBuilder(28-103)
modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.java (1)
plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.java (1)
TestDispatcherBuilder(28-103)
plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java (2)
modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.java (1)
TestDispatcherBuilder(28-103)plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.java (1)
TestDispatcherBuilder(28-103)
plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4BadRequestTests.java (1)
plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.java (1)
TestDispatcherBuilder(28-103)
modules/transport-netty4/src/test/java/org/opensearch/http/netty4/Netty4BadRequestTests.java (1)
modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.java (1)
TestDispatcherBuilder(28-103)
plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.java (1)
modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.java (1)
TestDispatcherBuilder(28-103)
plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4HttpServerTransportStreamingTests.java (1)
plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.java (1)
TestDispatcherBuilder(28-103)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (21)
- GitHub Check: gradle-check
- GitHub Check: precommit (21, windows-2025, true)
- GitHub Check: precommit (21, macos-15)
- GitHub Check: precommit (25, macos-15)
- GitHub Check: Analyze (java)
- GitHub Check: precommit (21, ubuntu-latest)
- GitHub Check: precommit (25, macos-15-intel)
- GitHub Check: precommit (21, ubuntu-24.04-arm)
- GitHub Check: precommit (25, ubuntu-24.04-arm)
- GitHub Check: precommit (25, windows-latest)
- GitHub Check: precommit (21, windows-latest)
- GitHub Check: detect-breaking-change
- GitHub Check: precommit (25, ubuntu-latest)
- GitHub Check: precommit (21, macos-15-intel)
- GitHub Check: assemble (25, ubuntu-24.04-arm)
- GitHub Check: assemble (25, windows-latest)
- GitHub Check: assemble (25, ubuntu-latest)
- GitHub Check: assemble (21, ubuntu-24.04-arm)
- GitHub Check: assemble (21, ubuntu-latest)
- GitHub Check: assemble (21, windows-latest)
- GitHub Check: Mend Security Check
🔇 Additional comments (45)
modules/transport-netty4/src/test/java/org/opensearch/http/netty4/Netty4BadRequestTests.java (3)
73-78: Non-throwingsetupis fine
setup()does not throw checked exceptions, so droppingthrows Exceptionmatches the implementation and keeps the signature clean.
80-83: Non-throwingshutdownis appropriate
shutdown()body cannot throw checked exceptions; removingthrows Exceptionis correct and consistent with other tests.
85-96: Dispatcher builder usage preserves bad‑request behaviorUsing
TestDispatcherBuilder.withDefaults()with a failingdispatchRequestand customdispatchBadRequestmirrors the old anonymous Dispatcher while centralizing the common wiring; looks good.plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4BadRequestTests.java (3)
50-55:setupsignature cleanup is correctNo checked exceptions are thrown here, so removing
throws Exceptionis safe and simplifies the test.
57-60:shutdownsignature cleanup is correctDropping
throws Exceptionmatches the implementation and other tests.
62-73: Refactored dispatcher matches previous bad‑request handlingThe TestDispatcherBuilder configuration (failing normal requests, custom
dispatchBadRequest) keeps the original test intent while reducing boilerplate.plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java (8)
32-32: ImportingTestDispatcherBuilderaligns secure tests with shared helperPulling in the builder here is consistent with other Netty/Reactor tests and centralizes Dispatcher setup.
174-183:shutdownsignature change is benignThe method only does null checks and shutdowns; removing
throws Exceptionis appropriate.
185-205: Javadoc formatting onlyOnly spacing/blank-line adjustments in Javadoc; behavior is unaffected.
221-224: Expect‑header dispatcher correctly uses builderUsing the builder with a simple OK
BytesRestResponse("done")preserves the original expect/continue behavior while leveraging shared defaults.
318-328: Bad‑request dispatcher mirrors non‑secure variantThe custom
withDispatchBadRequestthat recordscauseand sends a BAD_REQUEST response matches the non-secure Netty test and keeps the assertion that aTooLongFrameExceptionoccurred.
381-390: Large compressed response dispatcher preserved via builderThe lambda guarding on
urland returning a large OK body is unchanged in logic; using the builder just removes anonymous class noise.
425-425: CORS tests still rely on pre‑dispatch handlingUsing
TestDispatcherBuilder.withDefaults().build()here keeps the “no application handler should be hit” invariant similar toNullDispatcher; CORS logic remains in the transport layer.
479-481: Read‑timeout test now uses shared dispatcher helperUsing the default builder dispatcher is consistent with other timeout tests and keeps all actual behavior in the transport.
modules/transport-netty4/src/test/java/org/opensearch/http/netty4/Netty4HttpServerTransportTests.java (7)
115-121:setupno longer declaringthrows Exceptionis appropriateMethod body doesn’t throw checked exceptions; signature cleanup is correct.
123-132:shutdowncleanup is safeExplicit shutdown and nulling of fields don’t require a checked exception; updated signature is fine.
173-179: Builder‑based dispatcher for expect‑header tests looks goodThe dispatcher sending
"done"viaBytesRestResponsematches prior behavior while using the shared TestDispatcherBuilder.
269-280: Bad‑request dispatcher correctly focuses ondispatchBadRequestOverriding only
withDispatchBadRequestand using the builder defaults elsewhere clearly expresses the test’s intent and keeps the cause capture semantics.
332-341: Large compressed response dispatcher refactor is behavior‑preservingThe
urlguard and large OK body logic are unchanged; moving to the builder simply standardizes Dispatcher construction.
391-393: CORS tests leveraging default dispatcher is consistentUsing
withDefaults().build()here mirrors priorNullDispatcherbehavior: CORS handling occurs before any app dispatch.
445-447: Read‑timeout test now uses shared dispatcher helperDefault dispatcher from TestDispatcherBuilder is sufficient since the test only cares about connection closure; change is straightforward.
modules/transport-netty4/src/test/java/org/opensearch/http/netty4/ssl/SecureNetty4HttpServerTransportTests.java (9)
31-31: Shared dispatcher helper imported for secure Netty testsAdding
TestDispatcherBuilderhere brings the secure tests in line with the rest of the Netty test suite.
145-163:setupsignature simplification is correctInitialization does not throw checked exceptions; dropping
throws Exceptionmatches the implementation.
165-174:shutdowncleanup is appropriateThe teardown logic is unchanged; removing
throws Exceptionis safe.
176-207: Javadoc spacing onlyOnly blank-line formatting in the Javadocs changed; no behavioral impact.
218-224: Expect‑header dispatcher correctly refactored to builderThe dispatcher’s behavior (sending
"done"on success) stays the same while using the central TestDispatcherBuilder.
318-328: Bad‑request dispatcher logic preservedUsing
withDispatchBadRequestto send a BAD_REQUEST with a custom message and trackcauseReferencematches the previous anonymous implementation.
381-390: Large compressed response handler via builder is equivalentURL check plus OK response are unchanged; builder usage simply standardizes construction across tests.
425-425: CORS dispatcher viawithDefaults()is consistent with other testsRelying on transport-level CORS logic with a default dispatcher keeps app-level handlers out of the path as before.
479-481: Read‑timeout test uses shared dispatcher patternDefault dispatcher from the builder is sufficient here; behavior remains focused on timeout-induced close.
plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4HttpServerTransportTests.java (9)
109-115:setupsignature cleaned up correctlyNo checked exceptions thrown; removing
throws Exceptionis appropriate.
117-126:shutdownsignature cleaned up correctlyTeardown is unchanged functionally; non-throwing signature is fine.
170-176: Expect‑header dispatcher via builder mirrors Netty variantThe dispatcher responding with
"done"now uses TestDispatcherBuilder, matching the non-Reactor tests and keeping semantics the same.
257-258: Bad‑request test using default dispatcher is intentionalUsing
withDefaults().build()ensures any unexpected dispatch would fail the test, while URI length errors are handled at the transport layer as before.
301-305: Dispatch‑failed test refactor preserves 500 behaviorThe custom
withDispatchRequestthrowingRuntimeExceptionstill drives the INTERNAL_SERVER_ERROR expectations; builder just removes boilerplate.
341-350: Large compressed response dispatcher refactor is behavior‑preservingURL guard and body content checks remain identical; dispatcher is now built via the shared helper.
401-410: Connections‑closed test dispatcher refactor looks goodThe dispatcher still serves only the expected URL and asserts on others; the looped request/response and stats checks remain unchanged.
446-448: CORS test using default dispatcher keeps focus on transport CORS logic
withDefaults().build()continues to ensure no app handler is needed; behavior matches other CORS tests.
500-502: Connect‑timeout test uses shared dispatcher helper correctlyDefault dispatcher suffices; the test still validates timeout via channel closure.
plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4HttpServerTransportStreamingTests.java (4)
63-68: Lambda simplification is fineChanging
(str) ->tostr ->is a pure style tweak with no behavioral impact; XCONTENT_CONVERTER remains equivalent.
75-81:setupsignature cleanup is appropriateInitialization logic doesn’t throw checked exceptions; non-throwing signature is correct.
83-92:shutdownsignature cleanup is appropriateTeardown logic is unchanged; removing
throws Exceptionis safe and consistent with other tests.
177-216: Streaming dispatcher refactor preserves behavior and clarifies responsibilitiesUsing TestDispatcherBuilder to:
- advertise a streaming
RestHandlerviawithDispatchHandler(supportsStreaming=true,handleRequestguarded to fail if ever called), and- implement the actual streaming pipeline in
withDispatchRequest(assertingStreamingRestChannel, waiting for writability, emitting chunks on the WRITE executor, and closingbytesOutputifReleasable)keeps the original streaming semantics while centralizing Dispatcher construction. The URL guard and chunking logic remain unchanged.
plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.java (1)
24-86: Well-factored test dispatcher helper; defaults and delegation look correctThe builder defaults (logging +
AssertionError) and thebuild()implementation cleanly delegate to the configured lambdas and should make dispatcher setup in tests much clearer. No issues from a correctness or test-behavior perspective.modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.java (1)
24-86: Dispatcher builder implementation is correct and consistent with reactor-netty4 versionThe builder’s defaults and
build()delegation are sound and match the reactor-netty4 counterpart, giving consistent test behavior across modules. No functional issues spotted.
|
❌ Gradle check result for e980258: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
|
Seems like a flaky test failure |
Signed-off-by: Sergei Ustimenko <[email protected]>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #20160 +/- ##
============================================
- Coverage 73.30% 73.20% -0.10%
+ Complexity 71732 71689 -43
============================================
Files 5793 5793
Lines 328056 328076 +20
Branches 47245 47246 +1
============================================
- Hits 240476 240167 -309
- Misses 68264 68649 +385
+ Partials 19316 19260 -56 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1 participant
Description
This PR introduces helper builders
TestDispatcherBuilderin modules/Netty & plugins/Reactor Netty that simplify the way instances ofHttpTransport.Disaptcherget created.It also includes some minor cleanups like remove stale
throwsclause from some of setup/teardown method and such.Related Issues
This is a cleanup PR, no additional issue was created.
Check List
[ ] API changes companion pull request created, if applicable.[ ] Public documentation issue/PR created, if applicable.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.
Summary by CodeRabbit
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.