Bind the shared connector mocks to the loopback address - #2090
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR stabilizes integration-test connector stubs by moving shared WireMock-based connector mocks from ephemeral ports (new WireMockServer(0)) to fixed, named ports defined in WireMockPorts, reducing the risk of hard-to-diagnose local port collisions.
Changes:
- Added dedicated fixed ports for connector-related WireMock stubs (including a secondary content-signing formatting port).
- Updated
BaseConnectorMockand its concrete connector mocks/factory to bind to these fixed ports. - Updated affected tests to use the shared fixed connector port and improved teardown null-safety where multiple mocks may be partially started.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/test/java/com/otilm/core/util/WireMockPorts.java | Adds fixed port constants for connector mocks and clarifies when new constants are warranted. |
| src/test/java/com/otilm/core/util/mocks/BaseConnectorMock.java | Changes connector mock base to start WireMock on a fixed port (and extensions variant too). |
| src/test/java/com/otilm/core/util/mocks/ConnectorMockFactory.java | Constructs connector mocks with the appropriate WireMockPorts and adds a secondary content-signing formatting starter. |
| src/test/java/com/otilm/core/util/mocks/ContentSigningFormattingMock.java | Makes the formatting mock accept an explicit port so multiple instances can be run when required. |
| src/test/java/com/otilm/core/util/mocks/CryptographyProviderConnectorMock.java | Binds the cryptography-provider connector mock to its dedicated fixed port. |
| src/test/java/com/otilm/core/util/mocks/TimestampingFormattingConnectorMock.java | Binds the timestamping formatting connector mock to its dedicated fixed port (with transformers). |
| src/test/java/com/otilm/core/util/mocks/SignerConnectorMock.java | Binds the signer connector mock to its dedicated fixed port. |
| src/test/java/com/otilm/core/service/signingprofile/SigningProfileTestBase.java | Uses the shared fixed connector port for its WireMock-backed formatting connector server. |
| src/test/java/com/otilm/core/service/compliance/BaseComplianceTest.java | Uses the shared fixed connector port for compliance provider stubbing. |
| src/test/java/com/otilm/core/service/cmp/CmpTestUtil.java | Uses the shared fixed connector port for CMP cryptography-provider stubs and documents the shared-port constraint. |
| src/test/java/com/otilm/core/integration/service/SigningProfileServiceImplITest.java | Adds null-safe teardown for mocks and uses the secondary formatting mock where two are needed concurrently. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
ivosh
force-pushed
the
fix/1958-wiremock-fixed-ports-bases
branch
from
August 24, 2026 09:30
1e76d86 to
c173cdd
Compare
`new WireMockServer(0)` binds the IPv4 wildcard. The OS picks an ephemeral port without considering ports already bound on the more specific loopback address, and Jetty sets SO_REUSEADDR, so the bind succeeds anyway — a later connect is then routed to the foreign process holding 127.0.0.1 on that port, and the stub never sees the request. The failure surfaces as a plausible-looking HTTP response from an unrelated local listener, so it reads as a product bug rather than an environment collision. Convert the mock servers reached through a shared base class to the fixed ports WireMockPorts already names, all below the 49152 ephemeral floor. BaseConnectorMock takes the port from its concrete flavour, so each connector mock owns one; the per-class stubs that no class holds two of at once share a single CONNECTOR constant. SigningProfileServiceImplITest keeps two content-signing formatting mocks alive at the same time while it moves a profile between them, so that one gets an explicit second port through startSecondContentSigningFormatting(). No @TestPropertySource or @DynamicPropertySource changes, so the context signature count is untouched and ContextSignatureGuardTest.BASELINE stays at 57. Refs #1958 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
ivosh
force-pushed
the
fix/1958-wiremock-fixed-ports-bases
branch
from
August 24, 2026 09:32
c173cdd to
0057c1c
Compare
`new WireMockServer(port)` is `wireMockConfig().port(port)` — the same call the extensions variant already made explicitly. Once the port is threaded through both constructors, the only difference left is the extensions array, which an empty varargs covers, so the pair collapses into one. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
vyskocilm
reviewed
Aug 24, 2026
The wildcard bind is what breaks, not the dynamic port. `new WireMockServer(0)` asks the OS for a port free on 0.0.0.0, which it grants without regard for ports already held on the more specific 127.0.0.1; Jetty sets SO_REUSEADDR, so the overlapping bind succeeds and the loopback holder keeps answering. Naming the loopback address makes the OS skip every port already taken there, which fixes the collision without giving up dynamic ports. Fixed ports bought that safety at the price of coupling every test class to every other one's port usage, and capped each stub at one live instance — hence a CONTENT_SIGNING_FORMATTING_SECONDARY constant and a startSecondContentSigningFormatting() factory to work around it. Both are gone; same-kind mocks now simply coexist. LoopbackWireMock centralizes the configuration and the rationale. Every mock converted here hands its URL to the code under test at runtime, so none of them needs a port known before the Spring context exists. WireMockPorts keeps the three infrastructure stubs that genuinely do, and its doc now says so. URLs use the literal 127.0.0.1 rather than localhost: that name resolves to both 127.0.0.1 and ::1, and a client picking the IPv6 form cannot reach an IPv4-bound server. Verified against a live collision: with IntelliJ holding 127.0.0.1:63342, a wildcard bind on that port succeeded and the probe was answered by IntelliJ, while the loopback bind was refused as intended. Refs #1958 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
ivosh
changed the title
ivosh
changed the title
Each doc comment states what the element is, in one or two simple sentences. The rationale that survives is the part a reader cannot get from the code: that SO_REUSEADDR lets a wildcard bind overlap a held loopback port, and that WireMock registers response transformers only at server creation. Drop the comments that restated a name (`url`, `start`), the `@see` pointing at a sibling method, and the framing that defined each choice by what it is not. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
vyskocilm
approved these changes
Aug 25, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
new WireMockServer(0)binds the IPv4 wildcard. The OS picks an ephemeral port without considering ports already bound on the more specific loopback address, and Jetty sets SO_REUSEADDR, so the bind succeeds anyway — a later connect is then routed to the foreign process holding 127.0.0.1 on that port, and the stub never sees the request. The failure surfaces as a plausible-looking HTTP response from an unrelated local listener, so it reads as a product bug rather than an environment collision.Naming the bind address fixes it.
options().bindAddress("127.0.0.1").dynamicPort()makes the OS skip every port already taken on loopback, so the port stays dynamic and the stub still gets the request.LoopbackWireMockcentralises that configuration and the rationale; the connector mocks and the three test bases that build their own stub reach it throughstart()andurl().Stub URLs use the literal
127.0.0.1rather thanlocalhost. That name resolves to both127.0.0.1and::1, and a client picking the IPv6 form cannot reach an IPv4-bound server.WireMockPortskeeps the three infrastructure stubs whose URL has to exist before the Spring context does — auth-service, scheduler and provisioning-api, each injected as a base URL — and its doc now states that as the test for adding a constant. Every stub converted here hands its URL to the code under test at runtime, so none of them needs one.SigningProfileServiceImplITestguards its@AfterEachstops against a null mock, so a setup that aborts part-way surfaces its own failure instead of an NPE.No
@TestPropertySourceor@DynamicPropertySourcechanges, so the context signature count is untouched andContextSignatureGuardTest.BASELINEstays at 57.Verified against a live collision: with another local process holding
127.0.0.1:63342, a wildcard bind on that port succeeded and the probe was answered by that process, while the loopback bind on it was refused. Twenty-five consecutive loopback-bound dynamic starts each landed on a free port and were reached.SigningProfileServiceImplITest,ContentSigningEngineITest, the TSA/TSP controllers, the CMP handlers, the compliance and signing-profile bases,WireMockPortsGuardTestandContextSignatureGuardTestall pass.Refs #1958