Skip to content

Improve runtime, reduce log output, and stabilize SSL failure assertions for SSLCertificateTest#1215

Draft
Copilot wants to merge 19 commits intomasterfrom
copilot/improve-ssl-certificatetest-runtime
Draft

Improve runtime, reduce log output, and stabilize SSL failure assertions for SSLCertificateTest#1215
Copilot wants to merge 19 commits intomasterfrom
copilot/improve-ssl-certificatetest-runtime

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented May 6, 2026

SSLCertificateTest was consuming ~20 of the CI job's ~45 minutes, logging ~45k of ~120k log lines. This PR now includes six targeted fixes:

Timeout and heartbeat tuning

  • TIMEOUT_SECONDS: 15s → 7s
  • HeartBtInt: 30s → 5s across all session settings builders — a heartbeat exchange within the window is sufficient to confirm a healthy session

Reconnect interval cleanup

  • Removed all explicit Initiator.SETTING_RECONNECT_INTERVAL overrides in this test class so the default 30 second reconnect interval is used
  • This avoids repeated expected SSL exceptions during the 7 second assertion window

Reduce log noise for expected failures

  • volatile boolean exceptionExpected flag on TestConnector, starts false
  • Added expectException() method to TestConnector — called on each connector immediately after construction and before start() in all shouldFailWhen* methods
  • The MINA filter in prepareConnector checks the flag in exceptionCaught: logs message-only when true, full stack trace when false
  • Connectors used in non-failure tests never have the flag set, preserving full traces for unexpected failures
  • The exception log line now also includes connector context (initiator/acceptor and connectPort/acceptPort) so remaining SSL noise can be traced back to the exact connector

This cuts the bulk of log output generated by shouldFailWhen* tests while making the remaining output easier to attribute.

Parallel no-exception checks

Added assertNoSslExceptionsThrown(TestConnector... connectors) — runs each connector's latch-wait concurrently instead of sequentially. For a test with N connectors this replaces N×7s with a single 7s wait.

// before: sequential, each blocks for up to 7s
initiator.assertNoSslExceptionThrown();
// ... initiator assertions ...
acceptor.assertNoSslExceptionThrown();

// after: single concurrent wait
assertNoSslExceptionsThrown(initiator, acceptor);
// ... initiator and acceptor assertions ...

Threads are named (ssl-check<Type><index>) for debuggability. On failure, a composite error message summarises all failures upfront with individual errors as suppressed exceptions. Applied across 15 test methods.

Test name logging

Added a class-level LOGGER and a @Rule TestName field to SSLCertificateTest. The existing @Before setUp() now logs >>> Running test: <methodName> before every test, making it easy to correlate log output with the active test method in CI logs.

Targeted flakiness stabilization for shouldFailWhenUsingEmptyServerKeyStore()

  • Kept strict SSL-exception assertion on the acceptor.
  • Updated initiator assertion to accept failure as an outcome set:
    • SSL exception observed, or
    • no SSL exception observed but the initiator did not send FIX Logon before disconnect/reset.
  • Implemented minimal Logon tracking on TestInitiator via toAdmin(...) (MsgType.LOGON) to support this check.

This stabilizes the test across JSSE/provider/timing differences while preserving intended behavior.

Copilot AI and others added 2 commits May 6, 2026 11:15
…rallelize no-exception checks

Agent-Logs-Url: https://github.com/quickfix-j/quickfixj/sessions/f464dac4-ab03-4de1-a750-fbcb1d6bd19c

Co-authored-by: chrjohn <6644028+chrjohn@users.noreply.github.com>
Copilot AI changed the title [WIP] Improve runtime for SSLCertificateTest Improve runtime for SSLCertificateTest May 6, 2026
Copilot AI requested a review from chrjohn May 6, 2026 11:21
Copilot finished work on behalf of chrjohn May 6, 2026 11:21
Comment thread quickfixj-core/src/test/java/quickfix/mina/ssl/SSLCertificateTest.java Outdated
Copilot finished work on behalf of chrjohn May 6, 2026 13:51
Copilot AI requested a review from chrjohn May 6, 2026 13:51
Copilot finished work on behalf of chrjohn May 6, 2026 15:07
@chrjohn chrjohn changed the title Improve runtime for SSLCertificateTest Improve runtime for SSLCertificateTest May 6, 2026
@chrjohn chrjohn changed the title Improve runtime for SSLCertificateTest Improve runtime and log output for SSLCertificateTest May 6, 2026
Copilot finished work on behalf of chrjohn May 6, 2026 15:48
Refactor createDefaults method to initialize defaults directly.
Copilot stopped work on behalf of chrjohn due to an error May 6, 2026 17:05
@chrjohn chrjohn changed the title Improve runtime and log output for SSLCertificateTest Improve runtime and reduce log output for SSLCertificateTest May 6, 2026
Copy link
Copy Markdown
Contributor Author

Copilot AI commented May 7, 2026

@copilot

  1. Please add logging to each test method so that I can see in the logs which test is currently active. That way I could identify where still too much is logged.
  2. Please update PR descri...

Done in dd263e0:

  1. Added a class-level LOGGER and @Rule public final TestName testName to SSLCertificateTest. The @Before setUp() now logs >>> Running test: <methodName> before every test — covers all 24 methods with a single change. Since the test is parameterized, the logged name also includes the parameter set (e.g. shouldAuthenticateServerCertificate[0]).

  2. Updated the PR description to accurately reflect all changes, including the expectException() / before-start() pattern replacing the old description of setting the flag in assertSslExceptionThrown.

Comment thread quickfixj-core/src/test/java/quickfix/mina/ssl/SSLCertificateTest.java Outdated
Comment thread quickfixj-core/src/test/java/quickfix/mina/ssl/SSLCertificateTest.java Outdated
@chrjohn
Copy link
Copy Markdown
Member

chrjohn commented May 9, 2026

@copilot
shouldFailWhenUsingEmptyServerKeyStore sometimes does not see the expected exception for the initiator. Only the acceptor gets the exception reliably.

Treat initiator failure as an outcome set, not a single event: SSL exception observed or connection closed/reset before logon, so please check if initiator could send a Logon. If no, fail the test.

This makes the test stable across JSSE/provider/timing differences while still verifying the intended behavior.

Only change for shouldFailWhenUsingEmptyServerKeyStore() for now

Please update PR description to document this.

Copy link
Copy Markdown
Contributor Author

Copilot AI commented May 9, 2026

@copilot
shouldFailWhenUsingEmptyServerKeyStore sometimes does not see the expected exception for the initiator. Only the acceptor gets the exception reliably.

...

Implemented for shouldFailWhenUsingEmptyServerKeyStore() in 34a7835. The initiator now accepts outcome-set behavior: it passes if an SSL exception is observed, or (if not) asserts that no Logon was sent before disconnect. Acceptor remains strict on SSL exception. I also updated the PR description to document this targeted change.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • localhost-quickfixj
    • Triggering command: /usr/lib/jvm/temurin-17-jdk-amd64/bin/java /usr/lib/jvm/temurin-17-jdk-amd64/bin/java -Xmx2g -Djava.net.preferIPv4Stack=true --add-opens java.base/java.lang=ALL-UNNAMED -jar /home/REDACTED/work/quickfixj/quickfixj/quickfixj-core/target/surefire/surefirebooter-20260509112429257_3.jar /home/REDACTED/work/quickfixj/quickfixj/quickfixj-core/target/surefire 2026-05-09T11-24-28_446-jvmRun1 surefire-20260509112429257_1tmp surefire_0-20260509112429257_2tmp (dns block)
    • Triggering command: /usr/lib/jvm/temurin-17-jdk-amd64/bin/java /usr/lib/jvm/temurin-17-jdk-amd64/bin/java -Xmx2g -Djava.net.preferIPv4Stack=true --add-opens java.base/java.lang=ALL-UNNAMED -jar /home/REDACTED/work/quickfixj/quickfixj/quickfixj-core/target/surefire/surefirebooter-20260509112544266_3.jar /home/REDACTED/work/quickfixj/quickfixj/quickfixj-core/target/surefire 2026-05-09T11-25-44_194-jvmRun1 surefire-20260509112544266_1tmp surefire_0-20260509112544266_2tmp (dns block)
    • Triggering command: /usr/lib/jvm/temurin-17-jdk-amd64/bin/java /usr/lib/jvm/temurin-17-jdk-amd64/bin/java -Xmx2g -Djava.net.preferIPv4Stack=true --add-opens java.base/java.lang=ALL-UNNAMED -jar /home/REDACTED/work/quickfixj/quickfixj/quickfixj-core/target/surefire/surefirebooter-20260509112645065_3.jar /home/REDACTED/work/quickfixj/quickfixj/quickfixj-core/target/surefire 2026-05-09T11-26-45_005-jvmRun1 surefire-20260509112645065_1tmp surefire_0-20260509112645065_2tmp (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

Copilot AI changed the title Improve runtime and reduce log output for SSLCertificateTest Improve runtime, reduce log output, and stabilize SSL failure assertions for SSLCertificateTest May 9, 2026
Copilot finished work on behalf of chrjohn May 9, 2026 11:29
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.

Runtime and log output for SSLCertificateTest needs to be improved

2 participants