Skip to content

Tests: close protocol/auth/concurrency coverage gaps + fix testNoAttach race + harden error semantics - #4

Merged
Tagar merged 8 commits into
masterfrom
tests-and-race-fixes
May 21, 2026
Merged

Tests: close protocol/auth/concurrency coverage gaps + fix testNoAttach race + harden error semantics#4
Tagar merged 8 commits into
masterfrom
tests-and-race-fixes

Conversation

@Tagar

@Tagar Tagar commented May 21, 2026

Copy link
Copy Markdown
Member

This PR closes several concrete test-coverage gaps and fixes the testNoAttach flake. Along the way the new tests surfaced three small semantic gaps in protocol.py / java_gateway.py; those are folded in as part of the same coherent change.

Test coverage added

  • Protocol containers + malformed wire format (protocol_containers_test.py, new) — round-trip on list/dict/set get_command_part encoding (pinning that plain Python containers correctly raise AttributeError and need JVM-side conversion via java_collections.ListConverter); Py4JError semantics for unknown type codes / truncated answer / error marker.
  • Decimal edge cases (protocol_test.py::DecimalEncodingTest, 4 tests) — high-precision survival; Decimal("Infinity") and Decimal("NaN") encode verbatim into the wire output, NOT silently corrupted.
  • String escaping boundaries (protocol_test.py::StringEscapingEdgeCasesTest, 7 tests) — UTF-8 (CJK + emoji), CRLF, consecutive backslashes, null bytes, empty string, only-special-chars. Pins behavior before any future unescape_new_line rewrite.
  • Auth failure paths (client_server_test.py::AuthFailureTest, 3 tests) — wrong token / missing token / oversized token (4096 chars) → Py4JAuthenticationError on first JVM access.
  • Concurrent gateway usage (concurrent_gateway_test.py, new) — tiered with @pytest.mark.slow: 16-thread × 50-call quick variant on every CI cell, 100-thread × 1000-call stress variant gated for workflow_dispatch only. Uses threading.Barrier(timeout=60) for deterministic lockstep + safe_join for bounded teardown.
  • ThreadSafeFinalizer race (finalizer_test.py::ThreadSafeFinalizerRaceTest) — same tiered approach; concurrent add + clear pressure.
  • JVM crash mid-call (client_server_test.py::JVMCrashRecoveryTest) — Popen.kill() the JVM, next call must raise (Py4JNetworkError, Py4JError, ConnectionError, OSError) within a bounded read_timeout=5.0 (no hang). Tight exception list — no Exception catch-all.

Race / semantic fixes folded in

  • testNoAttach race fix — replaces the prior assertRaises that flaked on macOS Python 3.13 + Java 17 (1-in-N) with a deterministic socket-close probe (poll until connect() is refused, bounded 5s). The async listening-socket close after shutdown() is the race; the probe makes the precondition explicit.
  • get_return_value Py4JError semantics (production fix in protocol.py) — wraps OUTPUT_CONVERTER[type_char] lookups in try/except KeyError → Py4JError and adds a len(answer) < 2 guard so truncated responses surface as Py4JError, not bare KeyError / IndexError. Surfaced by MalformedWireFormatTest.
  • Missing-token → Py4JAuthenticationError (production fix in java_gateway.py) — GatewayConnection.send_command detects Java-side AuthCommand error responses ("Authentication error" substring on an ERROR + STRING_TYPE prefix) and raises Py4JAuthenticationError instead of letting the generic Py4JError path catch it. Required for the missing-token negative test; closes a latent bug where auth_token=None produced the wrong exception class.

Pytest config

Adds setup.cfg with the slow marker registered for @pytest.mark.slow gating.

Out of scope (deferred)

  • SSL/TLS cert validation tests (needs test fixtures).
  • Callback-server accept-timeout under load (needs harness).
  • Signals concurrent-dispatch tests (lower priority).
  • unescape_new_line state-machine rewrite (gated on this coverage landing — separate perf PR).
  • Extracting the "Authentication error" magic string to a named constant (follow-up; current behavior is correct, just couples to the Java-side string).

Co-authored-by: Isaac

Tagar added 8 commits May 20, 2026 22:15
shutdown() asks the JVM to tear down its GatewayServer but does not
wait for the listening socket to actually close. The follow-up call
through gateway2 could land before the close completed, leaving the
assertRaises with no exception to catch — a 1-in-N flake (observed
on Python 3.13 + Java 17 + macOS-latest cells).

Replace the racy second assertRaises with a deterministic probe:
poll the gateway port until connect() refuses, then assert. Bounded
5s timeout with a clear failure message.

Co-authored-by: Isaac
Adds tests that pin get_command_part encoding for primitives and
document that plain Python list/dict/set require JVM-side conversion
first (they are not encoded directly). Adds malformed-wire tests for
get_return_value (unknown type code, empty answer, error marker).

Production fix included: get_return_value previously let KeyError
escape to the caller on unknown OUTPUT_CONVERTER type codes. Both
error-path and success-path lookups are now wrapped to raise
Py4JError with a descriptive message.

Co-authored-by: Isaac
Decimal goes through smart_decode(repr(d)) — precision survives,
Inf/NaN must not silently corrupt the wire. No prior test pinned
either guarantee.

Co-authored-by: Isaac
Pins the existing string-encoding contract before any future
state-machine rewrite of unescape_new_line. Boundary cases:
CJK, emoji, consecutive backslashes, mixed CRLF, embedded null.

Co-authored-by: Isaac
Wrong token / missing token / oversized token all must raise
Py4JAuthenticationError on the first JVM access, not a generic
socket error.  No prior test exercised the negative paths.

The missing-token case exposed a gap: when auth_token=None the
Python client skipped the auth handshake, the Java server returned
an "Authentication error: unexpected command." error string, and
Python surfaced it as the parent Py4JError.  Fix
GatewayClientConnection.send_command() to detect the "Authentication
error" sentinel in error responses and raise Py4JAuthenticationError
before the caller's generic get_error_message() path.

Co-authored-by: Isaac
Quick (16t x 50 iter) runs on every CI cell; stress (100t x 1000 iter)
is @pytest.mark.slow-gated for workflow_dispatch only. Both use
threading.Barrier for deterministic lockstep — no time.sleep,
no flake from CI runner load.

Co-authored-by: Isaac
Quick (16t x 100 iter) on every CI cell; stress (100t x 1000 iter)
@pytest.mark.slow-gated. Threads alternately add finalizers and
trigger clears; asserts no exceptions, no leaked entries.

Co-authored-by: Isaac
Kill the example-app JVM process via terminate(), then exercise the
gateway: must raise a connection-class exception within a reasonable
time bound (not hang).

Co-authored-by: Isaac
@codspeed-hq

codspeed-hq Bot commented May 21, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 16 untouched benchmarks
⏩ 7 skipped benchmarks1


Comparing tests-and-race-fixes (0b9c46f) with master (899ee0d)

Open in CodSpeed

Footnotes

  1. 7 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@Tagar
Tagar merged commit c4dc138 into master May 21, 2026
58 checks passed
Tagar added a commit that referenced this pull request May 21, 2026
…03)"

This reverts commit 5026653 — removing FindBugs was overreach on
weak evidence:

* The 403 was on a SINGLE cell in PR #9's matrix (Python 3.9 /
  Java 8 / ubuntu-latest). Other cells in the SAME matrix run
  resolved `findbugs:3.0.+` successfully — proving the 403 is
  transient (likely Maven Central IP-throttling fresh runners),
  not a permanent policy change.
* Master CI on PRs #4 / #5 / #6 / #7 / #8 has been passing the
  same FindBugs resolution step reliably for months.
* Removing static analysis to "fix" a single flake degrades code
  quality on every future build.

The right defensive measures are already in this PR:
* shell-level retry around `./gradlew check && assemble`
* `shell: bash` for cross-platform consistency

If FindBugs ever does become permanently unavailable, that's the
moment to switch to SpotBugs — a real plugin migration, not a
panic delete.

Co-authored-by: Isaac
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant