fix: refining CLIResult asserts related to missing or repeated messages by shawkins · Pull Request #51238 · keycloak/keycloak

shawkins · 2026-07-28T13:08:16Z

These changes should keep the intention of the asserts, but avoid adding a baked in atLeast durations when checking for no messages.

closes: #51095

Copilot

Pull request overview

Note

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Refines CLI output assertions used in Quarkus distribution/integration tests to better handle “no message” checks without relying on implicit timing, especially for running-server scenarios (issue #51095).

Changes:

Extends CLIResult “no output” assertion helper to optionally allow assertions while the server is still running.
Adds new CLIResult assertion helpers (assertNoMessageAfter, assertNoStartupMessage) and updates existing assertions to guard against misuse on running servers.
Updates multiple distribution tests to use the new assertions for startup/running-server output.

Reviewed changes

Copilot reviewed 1 out of 6 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
quarkus/tests/junit5/src/main/java/org/keycloak/it/junit5/extension/CLIResult.java	Adds new assertion helpers and enforces safer usage when the server is still running.
quarkus/tests/integration/src/test/java/org/keycloak/it/cli/dist/MetricsDistTest.java	Switches to startup-safe “no message” assertion for running server.
quarkus/tests/integration/src/test/java/org/keycloak/it/cli/dist/ManagementOffDistTest.java	Switches to startup-safe “no message” assertion for running server.
quarkus/tests/integration/src/test/java/org/keycloak/it/cli/dist/ManagementDistTest.java	Switches to startup-safe “no message” assertion for running server.
quarkus/tests/integration/src/test/java/org/keycloak/it/cli/dist/LoggingDistTest.java	Updates logging assertions to use new helpers and adds a test provider.
quarkus/tests/integration/src/test/java/org/keycloak/it/cli/dist/FipsDistTest.java	Replaces “shown 0 times” with startup-safe “no message” assertion.

Copilot

Pull request overview

Copilot reviewed 1 out of 6 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

quarkus/tests/junit5/src/main/java/org/keycloak/it/junit5/extension/CLIResult.java:139

expected does not act as a completion fence unless it is emitted after every possible notExpected message. The new callers use the @PreMatching TestFilter request log as expected, but the HTTP access log is emitted after request processing, so this can still snapshot output before the forbidden access-log line arrives and false-pass. Use a marker that is emitted after access logging/output collection has completed.

        assertMessage(expected);
        noOutput("The standard output contains: " + notExpected, notExpected, this::getOutput, this, true);

quarkus/tests/junit5/src/main/java/org/keycloak/it/junit5/extension/CLIResult.java:147

This bypass is unsafe for failed manual starts. RawKeycloakDistribution.waitFor(true) returns immediately when the process has already stopped without joining the asynchronous output reader, and ManagementDistTest.testManagementNoHttps now invokes this helper on an expected startup failure; the assertion can therefore false-pass while stdout is still being drained. Drain completed-process output or require a reliable completion signal before checking absence.

    default void assertNoStartupMessage(String message) {
        noOutput("The standard output contains: " + message, message, this::getOutput, this, true);

Copilot

Pull request overview

Copilot reviewed 2 out of 7 changed files in this pull request and generated 1 comment.

Copilot

Pull request overview

Copilot reviewed 2 out of 7 changed files in this pull request and generated no new comments.

Copilot

Pull request overview

Copilot reviewed 2 out of 7 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

quarkus/tests/junit5/src/main/java/org/keycloak/it/junit5/extension/CLIResult.java:139

assertMessage(expected) is not an output-completion barrier: the expected line may precede a forbidden line that is still being produced or drained. In the new access-log usages, TestFilter logs from a ContainerRequestFilter before access logging completes, so this immediate absence check can still falsely pass; use a sentinel guaranteed to be emitted after the candidate output (and wait for its new occurrence), or stop and fully drain the process before checking.

    default void assertNoMessageGiven(String expected, String notExpected) {
        assertMessage(expected);
        noOutput("The standard output contains: " + notExpected, notExpected, this::getOutput, this, true);

Pepo48

I played with assertNoMessageGiven and managed to get it to false-pass. I broke the exclude pattern in Quarkus AccessLogHandler (the exact bug the test is meant to catch) and added a non-blocking 3s delay before the log write. The assertNoMessageGiven check passed silently - the access-log line just hadn't hit stdout yet. Only the file-based Awaitility check in httpAccessLogNotNamedPattern caught it.

I think it's because assertNoMessageGiven does a single snapshot right after finding the expected message, so anything arriving a bit later slips through.

Regardless, it's not a blocker for me, the real-world window is tiny and this is way better than the old approach. Just thought it was worth mentioning.

Therefore, I'm approving. Thanks, Steve.

shawkins · 2026-07-30T19:57:41Z

I played with assertNoMessageGiven and managed to get it to false-pass. I broke the exclude pattern in Quarkus AccessLogHandler (the exact bug the test is meant to catch) and added a non-blocking 3s delay before the log write. The assertNoMessageGiven check passed silently - the access-log line just hadn't hit stdout yet. Only the file-based Awaitility check in httpAccessLogNotNamedPattern caught it.

I think it's because assertNoMessageGiven does a single snapshot right after finding the expected message, so anything arriving a bit later slips through.

Regardless, it's not a blocker for me, the real-world window is tiny and this is way better than the old approach. Just thought it was worth mentioning.

Therefore, I'm approving. Thanks, Steve.

This is possible and relates to what copilot was calling out as well. Essentially I had started down the path of reworking the TestFilter for this purpose, but that started to seem like overkill, so I just let it be based upon looking for a subsequent request log. Let me take a look again at what doing this the right way would entail.

closes: keycloak#51095 Signed-off-by: Steve Hawkins <shawkins@redhat.com>

shawkins · 2026-07-30T23:41:24Z

@Pepo48 added a done handler to run after the access logging.

Copilot

Pull request overview

Copilot reviewed 3 out of 9 changed files in this pull request and generated no new comments.

Copilot

Pull request overview

Copilot reviewed 3 out of 9 changed files in this pull request and generated no new comments.

Copilot

Pull request overview

Copilot reviewed 3 out of 9 changed files in this pull request and generated no new comments.

vmuzikar

LGTM, leaving to @Pepo48 to merge.

Pepo48

CI failure is related - we just need to adjust the strings.

Apart from that LGTM as well.

Pepo48 · 2026-07-31T14:35:34Z

        when().get("http://127.0.0.1:8080/realms/master/clients/account/redirect").then()
                .statusCode(200);
-        cliResult.assertNoMessage("127.0.0.1 GET /realms/master/clients/account/redirect");
+        cliResult.assertNoMessageGiven("Request GET /realms/master/clients/account/redirect has context request", "127.0.0.1 GET /realms/master/clients/account/redirect");


Suggested change

cliResult.assertNoMessageGiven("Request GET /realms/master/clients/account/redirect has context request", "127.0.0.1 GET /realms/master/clients/account/redirect");

cliResult.assertNoMessageGiven("TestFilter Request GET /realms/master/clients/account/redirect is done", "127.0.0.1 GET /realms/master/clients/account/redirect");

Pepo48 · 2026-07-31T14:36:16Z

+        fileCliResult.assertNoMessageGiven("TestFilter GET Request /realms/master/clients/account/redirect is done", "[org.keycloak.http.access-log]");
+        fileCliResult.assertNoMessageGiven("TestFilter GET Request /realms/master/clients/account/redirect is done", "127.0.0.1 GET /realms/master/.well-known/openid-configuration");
+        fileCliResult.assertNoMessageGiven("TestFilter GET Request /realms/master/clients/account/redirect is done", "127.0.0.1 GET /realms/master/clients/account/redirect");


Suggested change

fileCliResult.assertNoMessageGiven("TestFilter GET Request /realms/master/clients/account/redirect is done", "[org.keycloak.http.access-log]");

fileCliResult.assertNoMessageGiven("TestFilter GET Request /realms/master/clients/account/redirect is done", "127.0.0.1 GET /realms/master/.well-known/openid-configuration");

fileCliResult.assertNoMessageGiven("TestFilter GET Request /realms/master/clients/account/redirect is done", "127.0.0.1 GET /realms/master/clients/account/redirect");

fileCliResult.assertNoMessageGiven("TestFilter Request GET /realms/master/clients/account/redirect is done", "[org.keycloak.http.access-log]");

fileCliResult.assertNoMessageGiven("TestFilter Request GET /realms/master/clients/account/redirect is done", "127.0.0.1 GET /realms/master/.well-known/openid-configuration");

fileCliResult.assertNoMessageGiven("TestFilter Request GET /realms/master/clients/account/redirect is done", "127.0.0.1 GET /realms/master/clients/account/redirect");

Signed-off-by: Steve Hawkins <shawkins@redhat.com>

shawkins · 2026-07-31T19:02:44Z

CI failure is related - we just need to adjust the strings.

Yes, sorry I copied those in wrong to that test. I see we also have a few more spots for false positives like line 356, but I'm not sure it's worth the complexity to keep addressing this.

Copilot

Pull request overview

Copilot reviewed 3 out of 9 changed files in this pull request and generated no new comments.

shawkins requested a review from Copilot July 28, 2026 13:08

Copilot AI reviewed Jul 28, 2026

View reviewed changes

shawkins force-pushed the iss51095 branch 2 times, most recently from 856dab2 to 4a57489 Compare July 28, 2026 13:47

Copilot started reviewing on behalf of shawkins July 28, 2026 14:44 View session

shawkins marked this pull request as ready for review July 28, 2026 19:45

shawkins requested a review from a team as a code owner July 28, 2026 19:45

Copilot AI review requested due to automatic review settings July 28, 2026 19:45

shawkins requested a review from a team as a code owner July 28, 2026 19:45

Copilot started reviewing on behalf of shawkins July 28, 2026 19:46 View session

Copilot AI reviewed Jul 28, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings July 28, 2026 20:49

shawkins force-pushed the iss51095 branch from 4a57489 to 1912861 Compare July 28, 2026 20:49

Copilot started reviewing on behalf of shawkins July 28, 2026 20:49 View session

Copilot AI reviewed Jul 28, 2026

View reviewed changes

Comment thread quarkus/tests/junit5/src/main/java/org/keycloak/it/utils/RawKeycloakDistribution.java

Copilot AI review requested due to automatic review settings July 28, 2026 21:11

shawkins force-pushed the iss51095 branch from 1912861 to 1d7d5e8 Compare July 28, 2026 21:11

Copilot started reviewing on behalf of shawkins July 28, 2026 21:12 View session

Copilot AI reviewed Jul 28, 2026

View reviewed changes

shawkins force-pushed the iss51095 branch from 1d7d5e8 to 050affe Compare July 28, 2026 21:27

Copilot AI review requested due to automatic review settings July 28, 2026 21:27

Copilot started reviewing on behalf of shawkins July 28, 2026 21:28 View session

Copilot AI reviewed Jul 28, 2026

View reviewed changes

Pepo48 previously approved these changes Jul 30, 2026

View reviewed changes

Pepo48 mentioned this pull request Jul 30, 2026

[26.6] moving waiting into the assertMessage method #51239

Open

fix: refining CLIResult asserts related to missing or repeated messages

50e8761

closes: keycloak#51095 Signed-off-by: Steve Hawkins <shawkins@redhat.com>

Copilot AI review requested due to automatic review settings July 30, 2026 23:40

shawkins dismissed Pepo48’s stale review via 50e8761 July 30, 2026 23:40

shawkins force-pushed the iss51095 branch from 050affe to 50e8761 Compare July 30, 2026 23:40

Copilot started reviewing on behalf of shawkins July 30, 2026 23:40 View session

Copilot AI reviewed Jul 30, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings July 30, 2026 23:43

Copilot started reviewing on behalf of shawkins July 30, 2026 23:44 View session

shawkins force-pushed the iss51095 branch from ac55e03 to d9c98c7 Compare July 30, 2026 23:46

Copilot AI reviewed Jul 30, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings July 30, 2026 23:46

Copilot started reviewing on behalf of shawkins July 30, 2026 23:46 View session

Copilot AI reviewed Jul 30, 2026

View reviewed changes

vmuzikar previously approved these changes Jul 31, 2026

View reviewed changes

Pepo48 reviewed Jul 31, 2026

View reviewed changes

removing additional await and process argument

b866d9f

Signed-off-by: Steve Hawkins <shawkins@redhat.com>

Copilot AI review requested due to automatic review settings July 31, 2026 19:00

shawkins dismissed vmuzikar’s stale review via b866d9f July 31, 2026 19:00

shawkins force-pushed the iss51095 branch from d9c98c7 to b866d9f Compare July 31, 2026 19:00

Copilot started reviewing on behalf of shawkins July 31, 2026 19:00 View session

Copilot AI reviewed Jul 31, 2026

View reviewed changes

	cliResult.assertNoMessageGiven("Request GET /realms/master/clients/account/redirect has context request", "127.0.0.1 GET /realms/master/clients/account/redirect");
	cliResult.assertNoMessageGiven("TestFilter Request GET /realms/master/clients/account/redirect is done", "127.0.0.1 GET /realms/master/clients/account/redirect");

Uh oh!

Conversation

shawkins commented Jul 28, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Pepo48 left a comment

Choose a reason for hiding this comment

Uh oh!

shawkins commented Jul 30, 2026

Uh oh!

shawkins commented Jul 30, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

vmuzikar left a comment

Choose a reason for hiding this comment

Uh oh!

Pepo48 left a comment

Choose a reason for hiding this comment

Uh oh!

Pepo48 Jul 31, 2026

Choose a reason for hiding this comment

Uh oh!

Pepo48 Jul 31, 2026

Choose a reason for hiding this comment

Uh oh!

shawkins commented Jul 31, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants