fix: use lsprotocol-aware converter for jdtls + comprehensive e2e test suite#43
Merged
aviadshiber merged 14 commits intomainfrom Apr 10, 2026
Merged
fix: use lsprotocol-aware converter for jdtls + comprehensive e2e test suite#43aviadshiber merged 14 commits intomainfrom
aviadshiber merged 14 commits intomainfrom
Conversation
The server used a vanilla cattrs.Converter() to serialize LSP request
parameters before forwarding them to jdtls. This emits snake_case field
names verbatim from the pygls/lsprotocol models:
DefinitionParams(text_document=..., position=...)
→ {"text_document": {...}, "position": {...}}
But the LSP JSON wire format requires camelCase:
→ {"textDocument": {...}, "position": {...}}
jdtls's NavigateToDefinitionHandler (and every other position-based
handler) then called TextDocumentPositionParams.getTextDocument(),
which returned null because the JSON field was "text_document" not
"textDocument", and blew up with:
java.lang.NullPointerException: Cannot invoke
"org.eclipse.lsp4j.TextDocumentIdentifier.getUri()" because the
return value of "TextDocumentPositionParams.getTextDocument()"
is null
at NavigateToDefinitionHandler.definition(...:70)
This broke every go-to-definition, find-references, hover, document
symbol, and completion request forwarded to jdtls since the proxy
feature was introduced. Users saw "loading..." / no results and the
LSP log filled with stack traces.
The fix is a one-line change: use lsprotocol.converters.get_converter()
instead of cattrs.Converter(). The lsprotocol-aware converter:
- Converts snake_case attributes to camelCase JSON fields
- Handles lsprotocol's discriminated unions correctly
- Drops None-valued optional fields (matching LSP convention)
Also adds 7 regression tests in TestLspConverterCamelCase pinning the
JSON shape of DefinitionParams, ReferenceParams (with nested
context.includeDeclaration), HoverParams, DocumentSymbolParams,
CompletionParams, DidOpenTextDocumentParams (with nested languageId),
and the None-field-pruning behavior. A future regression to a vanilla
cattrs converter would fail every one of these.
Bumps version 0.7.1 -> 0.7.2.
Verification: 299 passed (was 292), 77% coverage, pyright+ruff clean.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Adds a new e2e test module (tests/test_e2e_jdtls.py) that spawns a real
jdtls subprocess and exercises every forwarded LSP request through the
actual wire. Unit tests with mocked subprocesses cannot catch JSON-shape
regressions because the mocks never parse the bytes — we learned this
the hard way twice in the last two releases:
- v0.7.1: jdtls silently failed to start because inherited JAVA_HOME
pointed at Java 8. Every unit test of find_jdtls_java_home passed
while users had zero working jdtls features.
- v0.7.2: every forwarded request NPE'd because a vanilla cattrs
Converter emitted snake_case field names (text_document) instead of
camelCase (textDocument). Every unit test passed while go-to-def,
references, hover, completion, and document symbol were all broken.
The new module has 7 tests covering every forwarded LSP method:
- test_initialize_handshake_succeeds — proxy fixture asserts start() ok
- test_document_symbol_round_trip — canonical camelCase regression
- test_definition_request_does_not_npe — the exact v0.7.2 scenario
- test_references_request_does_not_npe — exercises nested ReferenceContext
with include_declaration → includeDeclaration
- test_hover_request_does_not_npe
- test_completion_request_does_not_npe — distinct content-assist path
- test_did_open_notification_does_not_npe — notification path, catches
TextDocumentItem.language_id → languageId regressions
Each test:
1. Builds pygls-typed params (e.g. lsp.DefinitionParams(text_document=...))
2. Passes them through _serialize_params — the real production path
3. Asserts the serialized dict has camelCase keys (wire-format sanity)
4. Sends via JdtlsProxy.send_request/send_notification
5. Captures jdtls stderr via caplog and asserts no NullPointerException
appeared (catches regressions where send_request returns None from
an error response, or where jdtls logs but recovers)
Verified the tests actually catch the camelCase bug by temporarily
reverting _converter to cattrs.Converter() — the first test failed
immediately with:
AssertionError: _serialize_params emitted wrong field names:
dict_keys(['text_document', 'work_done_token', 'partial_result_token'])
CI additions (.github/workflows/test.yml):
- The unit matrix now passes `-m "not e2e"` to skip the new e2e tests.
- New `e2e-test` job runs on both ubuntu-latest and macos-latest with
a single Python version (3.12). Installs Java 21 via setup-java and
jdtls via `brew install jdtls` on macOS or a direct tarball download
from download.eclipse.org on Linux (wrapped in a shell launcher on
$PATH).
pyproject.toml registers the `e2e` pytest marker.
Local verification: 306 tests pass (299 unit + 7 e2e), 80% coverage,
68s for e2e runs end-to-end. pyright + ruff clean.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The previous commit pinned 1.47.0 arbitrarily — that's 10 minor versions behind the current milestone. Switch to 1.57.0 (build 202602261110) to match what Homebrew ships on macOS, so the Linux and macOS e2e jobs exercise the same jdtls version. Also switch to the canonical Eclipse mirror download URL used by the Homebrew formula, which redirects through a mirror selector and gives more reliable downloads than hitting download.eclipse.org directly. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Without a hash check, a compromised Eclipse mirror could ship a tampered tarball and our CI would execute arbitrary code inside the e2e jdtls subprocess. Pin and verify the sha256 from the Homebrew formula — it's the canonical upstream hash for this milestone build. The bump instructions in the comment now list all three values that must be updated together (VERSION, BUILD, SHA256) to prevent future maintainers from introducing a hash/version skew. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The e2e job runs only 7 tests (``-m e2e``) which exercise the proxy + server layers but none of the analyzers, so total coverage lands at ~27% — below the 60% fail-under threshold enforced by pyproject.toml's default addopts. That made CI report "7 passed" and then fail with exit 1. Pass --no-cov to the e2e pytest invocation. The unit matrix already produces the authoritative coverage report, so the e2e job doesn't need its own. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ion CI Adds 4 new e2e tests in TestMajorFeatureSanity that verify major product features end-to-end through the real LanguageServer instance: - test_null_return_diagnostic_fires — analyzer pipeline produces null-return diagnostic for a Java file with return null - test_null_check_to_monadic_diagnostic_fires — if(x != null) pattern triggers the monadic-flow hint - test_code_action_returns_quickfix_with_edit — CodeActionParams with a null-return diagnostic returns a QuickFix with Option.none() edit + auto-import - test_code_action_ignores_non_java_functional_lsp_diagnostics — jdtls diagnostics are filtered out, not processed by our handler These tests bootstrap a real pygls workspace via server.protocol._workspace, inject documents via put_text_document, and call the on_code_action handler directly. They exercise the full pipeline: pygls workspace → analyzer chain → fix generator → lsp CodeAction → WorkspaceEdit. CI changes: - Renamed e2e-test job to "integration" and changed it to run the FULL test suite (uv run pytest -v) rather than just -m e2e. This means the integration job runs all 310 tests (unit + e2e) with jdtls installed, and coverage naturally reaches 82% without any --no-cov hack. - The unit matrix (test job) still excludes e2e tests via -m "not e2e" for fast cross-version feedback. Verification: 310 passed (was 306), 82% coverage, pyright + ruff clean. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The test suite now achieves 82% coverage consistently (310 tests: unit + e2e). Raise the fail-under bar so future changes that drop coverage below 80% are caught by CI. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Moves the major-feature sanity tests from test_e2e_jdtls.py (gated by jdtls skipif) to a new tests/test_server.py (no jdtls dependency). This lets them run in the regular unit matrix, boosting coverage from 77% to 80% without needing jdtls installed. New test file tests/test_server.py contains: TestServerHelpers (11 tests): - _load_config: no workspace, missing file, valid JSON, invalid JSON - _to_lsp_diagnostic: with/without DiagnosticData - _analyze_document: with excludes config, without excludes - _serialize_params: camelCase regression guard - _handle_exception: logs uncaught exceptions - _jdtls_raw_to_lsp_diagnostics: valid + malformed raw dicts - _on_jdtls_diagnostics callback: mocked publish verifies analyzer runs TestAnalyzerPipeline (4 tests): - null-return, null-check-to-monadic, try-catch-to-monadic diagnostics - Clean file produces no diagnostics TestCodeActionPipeline (4 tests): - null-return QuickFix → Option.none() + auto-import - try-catch-to-monadic QuickFix → Try.of() + auto-import - Non-java-functional-lsp diagnostics filtered out - Empty diagnostic context → no actions Coverage threshold raised to 80% in pyproject.toml. Unit matrix (without e2e) reaches 80.13%; full suite (with e2e) reaches 83.25%. Verification: 327 passed (320 unit + 7 e2e), pyright + ruff clean. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…port Rewrites test_server.py with two complementary test tiers: **TestServerInternals** (16 tests) — direct-call tests exercising server.py helpers in-process for coverage: _load_config, _to_lsp_diagnostic, _analyze_document (with/without excludes), _handle_exception, _jdtls_raw_to_lsp_diagnostics, _on_jdtls_diagnostics callback, _serialize_params camelCase guard, and on_code_action for null-return, try-catch-to-monadic, and foreign-diagnostic filtering. One mock remains for text_document_publish_diagnostics (transport boundary). **TestLspLifecycle** (9 tests) — ZERO mocks. Spawns the real java-functional-lsp server as a subprocess via pygls LanguageClient, connects over stdio pipes, and drives the full LSP lifecycle: initialize → didOpen → wait for publishDiagnostics → codeAction. Tests cover: - initialize handshake - null-return / null-check-to-monadic / try-catch-to-monadic diagnostic publishing - Clean file → zero diagnostics - null-return QuickFix → Option.none() + auto-import - try-catch-to-monadic QuickFix → Try.of() + auto-import - Foreign-diagnostic filtering (jdtls diagnostics → no actions) - didChange with clean source → diagnostics cleared The subprocess tests run in ~3.4s total (real server startup + 9 requests). Coverage is provided by the direct-call tier (subprocess coverage cannot be captured by pytest-cov in the parent process). Unit matrix: 324 passed, 80.01% coverage (meets 80% bar). Full suite with e2e: 331 passed, 83%+ coverage. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
aviadshiber
changed the title
…est architecture - README: add try-catch-to-monadic to rules table (15 → 16 rules) and code actions table (3 patterns documented). Update jdtls section to mention auto-detection of Java 21+ via JDTLS_JAVA_HOME / java_home. Update Spring-aware suppression list. - CONTRIBUTING: add Test Architecture section documenting the 4-tier test suite (unit, server internals, LSP lifecycle, jdtls e2e) and the 80% coverage threshold. Document _FIX_TITLES assertion guard. - SKILL: same rule table + code action updates as README, plus updated @bean suppression list. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…precations - Fix heredoc indentation in CI (SHELL_EOF was indented, bash requires col 0) Replaced with printf to avoid heredoc pitfalls entirely - Replace false-confidence test_initialize_reports_capabilities with real assertions on code_action_provider and text_document_sync - Fix deprecated asyncio.get_event_loop() → get_running_loop() (Python 3.10+) - Add explicit timeout failure instead of silent .get() fallback in test_diagnostics_update_on_file_change - Log proxy cleanup errors instead of silently swallowing in e2e fixture - Assert required fields survive None-field pruning in test_none_fields_are_omitted Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Promote proxy and workspace fixtures to scope="class" with loop_scope="class" so all TestJdtlsEndToEnd tests share a single jdtls subprocess and event loop. Open the document once in fixture setup instead of per-test. The loop_scope parameter is required because pytest-asyncio creates a new event loop per test by default — without it, the asyncio subprocess handles from proxy setup break in subsequent tests. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes broken jdtls request forwarding (every go-to-definition, find-references, hover, document symbol, and completion request NPE'd) and adds a comprehensive end-to-end test suite that would have caught this before release.
The Bug
server.pyused a vanillacattrs.Converter()to serialize LSP request parameters before forwarding them to jdtls. This emitted snake_case field names (text_document) instead of the LSP-required camelCase (textDocument). jdtls's handlers then sawnullforTextDocumentPositionParams.getTextDocument()and threwNullPointerExceptionon every forwarded request.This has been broken since the jdtls proxy feature was introduced. Custom tree-sitter rules worked fine (they don't touch jdtls), but completions, hover, go-to-def, references, and document symbol were all dead.
The Fix
One-line change:
cattrs.Converter()->lsprotocol.converters.get_converter(). The LSP-aware converter handles camelCase, discriminated unions, and None-field pruning correctly.Test Architecture (the real payload of this PR)
We had two bugs slip through in the same release cycle (Java 21 detection in #42 + this camelCase bug) because all tests used mocked subprocesses that never parsed the actual bytes. This PR adds a layered test architecture that makes such regressions impossible:
TestServerInternalsTestLspLifecycleLanguageClientover stdio: initialize -> didOpen -> publishDiagnostics -> codeAction -> didChange. Catches serialization bugs, transport framing, workspace wiring.Verified the tests catch the bug
Temporarily reverted
_convertertocattrs.Converter()-- the first LSP lifecycle test failed immediately:CI Changes
testjob): 8 combos (Ubuntu + macOS x 4 Python versions), runs-m "not e2e", enforces 80% coverage (up from 60%)brew install jdtlsNumbers
Files Changed
src/java_functional_lsp/server.pycattrs.Converter()->get_converter(), dropimport cattrstests/test_server.pytests/test_e2e_jdtls.pytests/test_proxy.pyTestLspConverterCamelCase.github/workflows/test.ymlintegrationjob (Ubuntu + macOS) with jdtls install + sha256 verificationpyproject.tomle2epytest marker0.7.1->0.7.2Post-merge
Cut
v0.7.2release -> PyPI -> Homebrew tap auto-updates ->brew upgrade.🤖 Generated with Claude Code