MCP FAT: Add negative test scenarios by TheoGkoumas · Pull Request #35733 · OpenLiberty/open-liberty

TheoGkoumas · 2026-09-09T16:22:56Z

I have considered the risk of behavior change or other zero migration impact (https://github.com/OpenLiberty/open-liberty/wiki/Behavior-Changes).
If this PR fixes an Issue, the description includes "Fixes #FILLMEIN" or "Resolves #FILLMEIN" (verify release bug label if applicable: https://github.com/OpenLiberty/open-liberty/wiki/Open-Liberty-Conventions).
If this PR resolves an external Known Issue (including APARS), the description includes "Fixes #FILLMEIN" or "Resolves #FILLMEIN".

Fixes #35543

IssacAX123

LGTM

TheoGkoumas · 2026-09-11T11:05:24Z

#build (view Open Liberty Personal Build - ✅ completed successfully!)

Note: Target locations of links might be accessible only to IBM employees.

LibbyBot · 2026-09-11T11:16:56Z

Code analysis and actions

DO NOT DELETE THIS COMMENT.

12 FAT files were changed, added, or removed.
Check that the build did not break the affected FAT suite(s).
Please describe in a separate comment how you tested your changes.

Azquelt

There are a lot of problems here and a lot of the tests appear to make sense in isolation, but would be better added to existing tests. In addition, a lot of the new tests ignore the patterns in the existing tests.

Some of this reads like AI code that hasn't been properly read and understood.

Azquelt · 2026-09-11T14:24:22Z

+        // Wait for at least one @PreDestroy to confirm both calls ran
+        assertNotNull("@PreDestroy must fire after async tool calls",
+                      server.waitForStringInLogUsingMark("\\[LIFECYCLE] @PreDestroy AsyncLifecycleTools"));


Don't you need to wait for two?

Correct, assertions have been fixed.

Azquelt · 2026-09-11T14:26:23Z

+        List<String> postConstructEvents = server.findStringsInLogsUsingMark(
+                                                                             ".*\\[LIFECYCLE] @PostConstruct AsyncLifecycleTools.*", server.getDefaultLogFile());
+        List<String> preDestroyEvents = server.findStringsInLogsUsingMark(
+                                                                          ".*\\[LIFECYCLE] @PreDestroy AsyncLifecycleTools.*", server.getDefaultLogFile());
+
+        assertTrue("Two separate @PostConstruct events must fire (Dependent scope, no instance reuse)",
+                   postConstructEvents.size() >= 2);
+        assertTrue("Two separate @PreDestroy events must fire",
+                   preDestroyEvents.size() >= 2);


These checks are very loose. Please do a similar check to the previous test.

Azquelt · 2026-09-11T14:27:15Z

+        // Wait for two PreDestroy events to confirm two separate bean instances were created
+        assertNotNull("First @PreDestroy should fire",
+                      server.waitForStringInLogUsingMark("\\[LIFECYCLE] @PreDestroy ClassTool"));


Do you not need to wait for two messages?

Correct, assertions have been fixed.

Azquelt · 2026-09-11T14:27:37Z

+        List<String> postConstructEvents = server.findStringsInLogsUsingMark(
+                                                                             ".*\\[LIFECYCLE] @PostConstruct ClassTool.*", server.getDefaultLogFile());
+        List<String> preDestroyEvents = server.findStringsInLogsUsingMark(
+                                                                          ".*\\[LIFECYCLE] @PreDestroy ClassTool.*", server.getDefaultLogFile());
+
+        assertTrue("Two @PostConstruct events must fire for two calls (Dependent scope creates a new instance per call)",
+                   postConstructEvents.size() >= 2);
+        assertTrue("Two @PreDestroy events must fire for two calls",
+                   preDestroyEvents.size() >= 2);


Again, loose checks, see previous test

Azquelt · 2026-09-11T14:28:38Z

+        // Regardless of success/error the @PreDestroy callback must fire
+        assertNotNull("@PreDestroy must fire even when the tool call completes (success or error)",
+                      server.waitForStringInLogUsingMark("\\[LIFECYCLE] @PreDestroy ClassTool"));
+
+        List<String> lifecycleMessages = server.findStringsInLogsUsingMark(
+                                                                           ".*\\[LIFECYCLE].*", server.getDefaultLogFile());
+        assertFalse("Lifecycle log must not be empty", lifecycleMessages.isEmpty());
+
+        // @PostConstruct must precede @PreDestroy in the log
+        int postConstructIdx = -1;
+        int preDestroyIdx = -1;
+        for (int i = 0; i < lifecycleMessages.size(); i++) {
+            if (lifecycleMessages.get(i).contains("@PostConstruct ClassTool") && postConstructIdx == -1) {
+                postConstructIdx = i;
+            }
+            if (lifecycleMessages.get(i).contains("@PreDestroy ClassTool") && preDestroyIdx == -1) {
+                preDestroyIdx = i;
+            }
+        }
+        assertTrue("@PostConstruct must appear before @PreDestroy in the lifecycle log",
+                   postConstructIdx >= 0 && preDestroyIdx > postConstructIdx);
+    }


Loose checks, see previous comments.

Test strategy has changed in all corresponding tests with similar loose checks.
Additionally - on a general note - javadoc also calibrated to reflect the actual scenario.
Fixed.

Azquelt · 2026-09-11T16:20:55Z

+    /**
+     * Negative test: supplying an unknown {@code action} value to {@code async-test-tool}
+     * must return an internal-error response; the same invariant applies to the async path.
+     */
+    @Test
+    public void testCallAsyncToolWithUnknownActionReturnsInternalError() throws Exception {


This is testing the behaviour of a test fixture. Not necessary.

True. Test will be removed.

Azquelt · 2026-09-11T16:21:48Z

+    /**
+     * Negative test: {@link io.openliberty.mcp.tools.ToolManager#getTool} must return
+     * {@code null} when the requested tool name has never been registered.
+     * Verified in-process via {@link ToolManagerTestServlet}.
+     */
+    @Test
+    public void testGetToolReturnsNullForNonExistentTool() throws Exception {
+        runTest(server, APP_NAME + "/toolManagerTestServlet", "testGetToolReturnsNullForNonExistentTool");
+    }


Tests added to ToolManagerTestServlet are run automatically via the TestServlet annotation, so you don't need this.

Azquelt · 2026-09-11T16:25:08Z

+    /**
+     * Negative test: a {@code tools/list} call must report that both {@code exception}
+     * and {@code failureMechanism} are required arguments in {@code asyncErrorTool}'s
+     * {@code inputSchema}; neither may be absent from the {@code required} array.
+     */
+    @Test
+    public void testAsyncErrorToolSchemaListsBothArgumentsAsRequired() throws Exception {


This is written as a test for a text fixture.

If we did want to test this, I think we would do it as part of testToolList.

True, test method should be removed.

Azquelt · 2026-09-11T16:25:48Z

+    /**
+     * Negative test: supplying an unknown {@code exception} value (not one of the
+     * recognised class names) must return a well-formed {@code isError:true} JSON-RPC
+     * result; the server must not crash or produce an HTTP 500.
+     * The tool's default branch throws a {@link io.openliberty.mcp.tools.ToolCallException},
+     * so the response content must be the user-visible exception message.
+     */
+    @Test
+    public void testUnknownExceptionTypeReturnsUserErrorNotServerCrash() throws Exception {


This is testing a test fixture.

True, test method should be removed.

Azquelt · 2026-09-11T16:26:30Z

+    /**
+     * Negative test: when the async tool fails via {@code FAILED} (returns a
+     * {@code CompletableFuture.failedStage}), the CDI container must not keep any
+     * stale bean state; a second call with a different failure mechanism must still
+     * succeed independently. This is verified by calling twice in succession and
+     * asserting both produce well-formed {@code isError:true} responses (no session
+     * contamination or cached bean failure).
+     */
+    @Test
+    public void testSuccessiveFailedStageCallsRemainIsolated() throws Exception {


I think this is also testing a test fixture.

True, test method should be removed.

TheoGkoumas · 2026-09-15T16:07:54Z

Test methods have been revisited all over again and refactored accordingly based on the accurate PR comments.
Bod went overjoyed in this case and should have been treated more carefully.

Azquelt · 2026-09-15T16:24:34Z

+        // Wait for both @PreDestroy events — each call must destroy its own instance
+        assertNotNull("First @PreDestroy must fire",
+                      server.waitForStringInLogUsingMark("\\[LIFECYCLE] @PreDestroy AsyncLifecycleTools"));
+        assertNotNull("Second @PreDestroy must fire",
+                      server.waitForStringInLogUsingMark("\\[LIFECYCLE] @PreDestroy AsyncLifecycleTools"));


Unfortunately, this doesn't quite work, the second wait will just find the same message as the first wait.

We probably need something like this (possibly factored out into its own utility method):

Suggested change

// Wait for both @PreDestroy events — each call must destroy its own instance

assertNotNull("First @PreDestroy must fire",

server.waitForStringInLogUsingMark("\\[LIFECYCLE] @PreDestroy AsyncLifecycleTools"));

assertNotNull("Second @PreDestroy must fire",

server.waitForStringInLogUsingMark("\\[LIFECYCLE] @PreDestroy AsyncLifecycleTools"));

// Wait for both @PreDestroy events — each call must destroy its own instance

long startTime = System.nanoTime();

boolean found = false;

while (System.nanoTime() - startTime < Duration.ofSeconds(30).toNanos()) {

List<String> predestroyMessages = server.findStringInLogsUsingMark("\\[LIFECYCLE] @PreDestroy AsyncLifecycleTools");

if (predestroyMessages.size() >= 2) {

found = true;

break;

}

}

if (!found) {

fail("Did not found two PreDestroy messages");

}

Or we could artificially cause something else to be logged after that and wait on that message.

Suggestion has been applied.

Azquelt · 2026-09-18T13:05:18Z

+        // Wait for both @PreDestroy events - each call must destroy its own instance
+        assertNotNull("First @PreDestroy must fire",
+                      server.waitForStringInLogUsingMark("\\[LIFECYCLE] @PreDestroy ClassTool"));
+        assertNotNull("Second @PreDestroy must fire",
+                      server.waitForStringInLogUsingMark("\\[LIFECYCLE] @PreDestroy ClassTool"));


This has the same problem as above, the second wait will just find the first message.

The previous suggestion has been applied here as well.

Azquelt · 2026-09-18T13:07:20Z

+    /**
+     * Verifies that an {@code initialize} request whose body omits {@code protocolVersion}
+     * is tolerated: the server falls back to its preferred version ({@code 2025-11-25}) and
+     * returns a normal, successful initialize result rather than an error.
+     */
+    @Test
+    public void testInitializeWithMissingProtocolVersionFallsBackToServerDefault() throws Exception {


I can't find any version of the spec that permits the client to omit the protocol version.

The MCP spec says protocolVersion is a required string in the initialize params.
This test validates non-spec-compliant behavior (silent fallback to server default when a required field is missing). This test should be removed because the spec does not permit omitting protocolVersion; it's required.
The "fallback to server default" behavior is also not defined by the spec for missing required fields.

Azquelt · 2026-09-18T13:15:02Z

+        String errorType = operationStats.getErrorType();
+        assertNotNull("ErrorType must not be null for a business-error tool call", errorType);


We're still not asserting the actual error type here.

Fixed. Error type is now asserted.

Azquelt · 2026-09-18T13:18:50Z

+    // Negative Tests
+
+    // --- Monitoring and Management ---
+
+    /**
+     * Verifies that an operation MBean for a tool that threw a {@code ToolCallException} exposes
+     * {@code RpcResponseStatusCode} as {@code "error"} and {@code ErrorType} as {@code "tool_error"}.
+     */
+    @Test
+    public void testErrorToolMBeanHasErrorStatusAndNonNullErrorType() throws Exception {
+        client.callMCP(BUSINESS_ERROR_REQUEST);
+        runTest(server, SERVLET, "testErrorToolMBeanHasErrorStatusAndNonNullErrorType");
+    }
+
+    // --- Bean Lifecycle ---
+
+    /**
+     * Verifies that the session MBean is registered with a positive count and duration
+     * after the session is explicitly ended via DELETE.
+     */
+    @Test
+    public void testSessionMBeanPresentAfterSessionDeleted() throws Exception {
+        client.callMCP(PING_REQUEST);
+        client.deleteSession();
+        runTest(server, SERVLET, "testSessionMBeanPresentAfterSessionDeleted");
+    }
+
+    // --- Tool Metadata and Schema Generation ---
+
+    /**
+     * Verifies that every registered operation MBean exposes a non-null, non-blank
+     * {@code McpMethodName} attribute.
+     */
+    @Test
+    public void testAllOperationMBeansHaveNonEmptyMethodName() throws Exception {
+        client.callMCP(ADD_REQUEST);
+        runTest(server, SERVLET, "testAllOperationMBeansHaveNonEmptyMethodName");
+    }


As noted previously, doing these tests from outside the server in McpMonitorTest is sufficient.

That is true. I confirm that the three negative test scenarios are fully and equivalently covered from outside the server in McpMonitorTest. Tests will be removed as redundant.

Azquelt

This is looking way better. There are just a handful of comments left above to fix.

I may not be available to re-review. Most of my requests have been addressed.

Azquelt · 2026-09-29T12:40:58Z

+        // Both must succeed with isError:false
+        assertFalse("Alpha response must not indicate an error",
+                    new JSONObject(alphaResponse).getJSONObject("result").getBoolean("isError"));
+        assertFalse("Beta response must not indicate an error",
+                    new JSONObject(betaResponse).getJSONObject("result").getBoolean("isError"));


I'm a little concerned that a JSON-RPC error response might pass this check?

At best you'd get a null pointer exception if getJSONObject("result") returns null if there's no result object.

Azquelt · 2026-09-29T12:43:50Z

+        while (System.nanoTime() - startTime < Duration.ofSeconds(30).toNanos()) {
+            List<String> predestroyMessages = server.findStringsInLogsUsingMark("\\[LIFECYCLE] @PreDestroy AsyncLifecycleTools", server.getDefaultLogFile());
+
+            if (predestroyMessages.size() >= 2) {
+                found = true;
+                break;
+            }
+        }


Add a short sleep in here rather than doing a tight loop.

Suggested change

while (System.nanoTime() - startTime < Duration.ofSeconds(30).toNanos()) {

List<String> predestroyMessages = server.findStringsInLogsUsingMark("\\[LIFECYCLE] @PreDestroy AsyncLifecycleTools", server.getDefaultLogFile());

if (predestroyMessages.size() >= 2) {

found = true;

break;

}

}

while (System.nanoTime() - startTime < Duration.ofSeconds(30).toNanos()) {

List<String> predestroyMessages = server.findStringsInLogsUsingMark("\\[LIFECYCLE] @PreDestroy AsyncLifecycleTools", server.getDefaultLogFile());

if (predestroyMessages.size() >= 2) {

found = true;

break;

}

Thread.sleep(100);

}

Azquelt · 2026-09-29T12:45:50Z

+        while (System.nanoTime() - startTime < Duration.ofSeconds(30).toNanos()) {
+            List<String> predestroyMessages = server.findStringsInLogsUsingMark("\\[LIFECYCLE] @PreDestroy ClassTool", server.getDefaultLogFile());
+
+            if (predestroyMessages.size() >= 2) {
+                found = true;
+                break;
+            }
+        }


Add a short sleep here too.

Azquelt · 2026-09-29T12:53:38Z

Looking much better, just a few comments noted above.

TheoGkoumas added 2 commits September 9, 2026 18:00

negative test scenarios added

dd5615f

additional candidates for negative test scenarios

92f43df

TheoGkoumas force-pushed the mcp-neg-tests branch from 0532d75 to 92f43df Compare September 9, 2026 17:00

remove redundant methods as the scenarios have already been covered

9194f88

TheoGkoumas marked this pull request as ready for review September 10, 2026 13:48

IssacAX123 reviewed Sep 11, 2026

View reviewed changes

LibbyBot added the CLA Signed label Sep 11, 2026

LibbyBot added the noproductcode label Sep 11, 2026

TheoGkoumas requested a review from Azquelt September 11, 2026 13:07

Azquelt previously requested changes Sep 11, 2026

View reviewed changes

TheoGkoumas added 5 commits September 14, 2026 12:51

fix loose checks and flanky logic - PR comments

db2fe88

clean up redundant logic and apply PR comments

3a8acce

clean up from redundancies and fix test logic

ab4d945

revert to previous version

118ea03

revert to previous version

f208bf3

TheoGkoumas requested a review from Azquelt September 15, 2026 16:04

Azquelt reviewed Sep 15, 2026

View reviewed changes

Azquelt reviewed Sep 18, 2026

View reviewed changes

Comment thread ....mcp.internal_fat/fat/src/io/openliberty/mcp/internal/fat/lifecycle/tests/LifecycleTest.java

Azquelt reviewed Sep 18, 2026

View reviewed changes

Azquelt self-requested a review September 18, 2026 13:21

Azquelt reviewed Sep 18, 2026

View reviewed changes

Azquelt mentioned this pull request Sep 18, 2026

Write negative testing scenarios #35543

Open

6 tasks

More fixes and cleanups on PR comments

4e63a62

TheoGkoumas requested a review from Azquelt September 28, 2026 13:08

Azquelt requested changes Sep 29, 2026

View reviewed changes

		String errorType = operationStats.getErrorType();
		assertNotNull("ErrorType must not be null for a business-error tool call", errorType);

Conversation

TheoGkoumas commented Sep 9, 2026

Uh oh!

IssacAX123 left a comment

Choose a reason for hiding this comment

Uh oh!

TheoGkoumas commented Sep 11, 2026 • edited by LibbyBot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LibbyBot commented Sep 11, 2026

Code analysis and actions

Uh oh!

Azquelt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheoGkoumas commented Sep 15, 2026

Uh oh!

Azquelt Sep 15, 2026 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheoGkoumas commented Sep 11, 2026 •

edited by LibbyBot

Loading

Azquelt Sep 15, 2026 •

edited

Loading

TheoGkoumas Sep 28, 2026 •

edited

Loading