Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Jan 28, 2026

Plan for testRejectMessagesNotResentInCurrentCode

  • Explore repository structure and understand SessionTest.java patterns
  • Create unit test testRejectMessagesNotResentInCurrentCode in SessionTest.java
  • Test sets up session with ForceResendWhenCorruptedStore=false (default)
  • Store messages in sequence: Logon(1), AppMessage(2), Reject(3), AppMessage(4), Heartbeat(5)
  • Trigger ResendRequest for messages 1-5
  • Verify application messages (2,4) are resent with PossDupFlag
  • Verify Reject message (3) is NOT resent (demonstrating the bug)
  • Verify Logon and Heartbeat messages (1,5) are NOT resent
  • Verify SequenceReset GapFill messages include Reject in the gap
  • Run the test to verify it passes with current code
  • Validate the implementation follows existing test patterns
  • Address code review feedback: Replace FailingResponder with mock Responder

Changes Made

Latest Update (in response to code review)

  • Removed incorrectly added UnitTestResponder.java file
  • Replaced FailingResponder with Mockito mock responder and ArgumentCaptor
  • Now follows the same pattern as other resend tests in the codebase (e.g., correct_sequence_number_for_last_gap_fill_* tests)
  • Test still passes and verifies the same behavior

Test Results

The test successfully passes and demonstrates the current behavior where:

  • SequenceReset GapFill is sent for Logon (seq 1 → 2)
  • Application message (seq 2) is resent with PossDupFlag
  • SequenceReset GapFill is sent for Reject (seq 3 → 4) - this is the bug
  • Application message (seq 4) is resent with PossDupFlag
  • SequenceReset GapFill is sent for Heartbeat (seq 5 → 6)

This test documents issue #597 and provides a baseline for verifying the fix in PR #1124.

Original prompt

Problem Statement

Create a unit test that demonstrates the current behavior (before PR #1124's fix) where Reject messages are NOT being resent during a ResendRequest when ForceResendWhenCorruptedStore is disabled (the default behavior).

According to the FIX specification, Reject messages (MsgType=3) are the only session-level messages that SHOULD be resent. However, the current code treats Reject messages like all other session-level messages and skips them during resend operations.

Requirements

Create a new unit test in quickfixj-core/src/test/java/quickfix/SessionTest.java that:

  1. Sets up a session with ForceResendWhenCorruptedStore set to false (the default)
  2. Stores a sequence of messages in the message store:
    • A Logon message (seq 1)
    • An application message (seq 2)
    • A Reject message (seq 3) - This is the key message to test
    • Another application message (seq 4)
    • A Heartbeat message (seq 5)
  3. Triggers a ResendRequest for messages 1-5
  4. Verifies the current behavior that:
    • Application messages (seq 2 and 4) ARE resent with PossDupFlag
    • The Reject message (seq 3) is NOT resent (this is the bug)
    • Logon and Heartbeat messages (seq 1 and 5) are NOT resent
    • SequenceReset GapFill messages are sent to skip session-level messages including the Reject

Test Details

The test should be named something like testRejectMessagesNotResentInCurrentCode and should follow the pattern of existing resend tests in SessionTest.java such as:

  • testResendMessagesWithIncorrectChecksum (around line 1389)
  • testResendAbortsWhenSendReturnsFalse (around line 3211)
  • testResendSeqWithReject (around line 2469)

Use the existing helper methods:

  • createAdminMessage(int seqNum) to create session-level messages
  • createAppMessage(int seqNum) to create application messages
  • createResendRequest(int sequence, int from) to create resend requests
  • processMessage(session, message) to process messages

The test should use verification to check:

  • Which messages were actually sent (using the responder or application mock)
  • That SequenceReset GapFill messages were sent for the appropriate ranges
  • That the Reject message was included in the gap fill range (demonstrating the bug)

File Location

File: quickfixj-core/src/test/java/quickfix/SessionTest.java

Add the test method to the SessionTest class, following the existing test patterns and conventions.

Expected Outcome

This test should PASS with the current code (before PR #1124), documenting the current behavior where Reject messages are not resent. When PR #1124 is merged, this test will need to be updated or a complementary test added to verify that Reject messages ARE properly resent after the fix.

This test serves as documentation of the issue described in #597 and provides a baseline for verifying the fix in PR #1124.

This pull request was created from Copilot chat.


✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

Copilot AI changed the title [WIP] Add unit test for Reject messages resending behavior Add test documenting Reject messages not resent during ResendRequest Jan 28, 2026
Copilot AI requested a review from chrjohn January 28, 2026 12:28
// Create session with default settings (ForceResendWhenCorruptedStore=false)
try (Session session = SessionFactoryTestSupport.createSession(sessionID, application, false, false, true, true, null)) {
// Use a responder that captures all sent messages
FailingResponder responder = new FailingResponder(100); // Allow many successful sends
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot Please replace the FailingResponder by a normal Responder, since we do not need the functionality of the FailingResponder.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replaced FailingResponder with mock Responder and ArgumentCaptor (commit fad489e). Also removed the incorrectly added UnitTestResponder.java file.

Copilot AI requested a review from chrjohn January 29, 2026 16:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants