Инкрементальный парсер + инкремент копилота #3617
Conversation
WalkthroughIntroduces incremental text change support to the BSL language server, replacing full document synchronization with incremental updates. Adds per-document async execution, benchmarking infrastructure, and comprehensive test coverage. Optimizes tokenizer reuse in document context rebuild. Changes
Sequence DiagramsequenceDiagram
actor Client
participant LS as Language Server
participant Executor as Per-Doc Executor
participant DocService as BSLTextDocumentService
participant DocCtx as DocumentContext
Note over LS: Old: Full sync on didChange
Client->>LS: didChange (Full)
LS->>DocService: rebuild full document
DocService->>DocCtx: update content
Note over LS: New: Incremental sync
Client->>LS: didChange (Incremental)
LS->>Executor: queue async task
Executor->>DocService: applyIncrementalChange
DocService->>DocService: apply change (offset, delete, insert)
DocService->>DocCtx: rebuild tokenizer
DocCtx->>DocCtx: reuse tokenizer if exists
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45-60 minutes
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
- Changed TextDocumentSyncKind from Full to Incremental in server capabilities - Updated didChange to handle incremental text changes with ranges - Added utility methods to apply incremental changes to document content - Added tests for incremental changes including insert, delete, and multiple changes Co-authored-by: nixel2007 <[email protected]>
- Created IncrementalTextChangeTest with 9 test cases - Tests cover insert, delete, replace, multi-line, and full document scenarios - All tests pass successfully Co-authored-by: nixel2007 <[email protected]>
Changed regex pattern from "\r?\n" to "\r?\n|\r" to match DocumentContext.CONTENT_SPLIT_PATTERN, which correctly handles all line ending types including old Mac format (\r). Co-authored-by: nixel2007 <[email protected]>
- Replaced line-by-line reconstruction with direct string replacement using character offsets - Added getOffset() helper method to convert line/char positions to absolute offsets - Preserves original line endings (Windows \r\n, Unix \n, old Mac \r) - More efficient for large documents (O(n) single pass vs O(n) split + rebuild) - Added 3 new tests to verify line ending preservation Co-authored-by: nixel2007 <[email protected]>
- Replaced character-by-character iteration with indexOf() for finding line breaks - Uses String.indexOf() which is optimized in JVM for much better performance - Handles large documents (millions of characters) efficiently - Early return for line 0 to avoid unnecessary searching - Maintains correct handling of all line ending types (\r\n, \n, \r) Co-authored-by: nixel2007 <[email protected]>
- Created IncrementalTextChangeBenchmark with tests for 100, 1K, and 10K line documents - Tests change operations at start, middle, end, and multiple changes - Added comprehensive benchmark results documentation - Results show linear scaling: 0.16ms for 100 lines, 12.5ms for 1K lines - Performance is excellent for production use Co-authored-by: nixel2007 <[email protected]>
- Changed applyIncrementalChange and getOffset from private to protected - Updated JMH benchmark to call methods directly without reflection - Updated unit tests to call method directly - Removes reflection overhead from performance measurements Co-authored-by: nixel2007 <[email protected]>
Co-authored-by: nixel2007 <[email protected]>
…iple edits - Clarified that benchmarkChangeAt* tests measure ONE incremental edit - Clarified that benchmarkMultipleChanges measures THREE sequential edits - Updated notes to emphasize no reflection overhead in current version - Added detailed explanation of test scenarios - Results now clearly labeled as "per single edit" vs "3 sequential edits" Co-authored-by: nixel2007 <[email protected]>
Updated documentation to confirm that benchmarks now use direct method calls without reflection. While reflection overhead is minimal compared to the actual string operations (indexOf, substring), the current measurements are technically more accurate. Performance characteristics remain consistent with previous results as the core algorithm hasn't changed. Co-authored-by: nixel2007 <[email protected]>
- Added Map<String, ExecutorService> to track single-threaded executors per document - Initialize executor in didOpen using normalized URI from documentContext - Submit didChange operations to document's executor for serialization - Properly shutdown executors in didClose with 30-second timeout for pending changes - Use normalized URI from documentContext instead of params for consistency - Prevents race conditions when multiple didChange requests arrive rapidly Co-authored-by: nixel2007 <[email protected]>
theshadowco
force-pushed
the
feature/incParser
branch
from
71e8c12 to
2fce6ca
Compare
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
build.gradle.kts (1)
84-90: Dependency resolution will fail—version 0.27.0 does not existThe version string has two problems:
- "aplha" is misspelled (should be "alpha")
- Version 0.27.0 does not exist on Maven Central (latest available is 0.26.2)
Even correcting the typo would still fail to resolve. Update to an actual published version:
- api("io.github.1c-syntax", "bsl-parser", "0.27.0-aplha.1") { + api("io.github.1c-syntax", "bsl-parser", "0.26.2") {
🧹 Nitpick comments (5)
src/main/java/com/github/_1c_syntax/bsl/languageserver/context/DocumentContext.java (1)
308-320: Tokenizer reuse only active when secondary data is not clearedThe new logic:
this.content = content; if (tokenizer != null) { tokenizer.rebuild(content); } else { tokenizer = new BSLTokenizer(content); }is fine, but note that
rebuild()callsclearSecondaryData()when!isComputedDataFrozen, andclearSecondaryData()setstokenizer = null. So for normal rebuilds (no freeze) you still recreate the tokenizer every time; reuse only happens when data is frozen andclearSecondaryData()is skipped.If the intent was to reuse the tokenizer for all incremental rebuilds, consider not nulling
tokenizerinclearSecondaryData()and relying solely onrebuild(content)to refresh it.JMH_BENCHMARK_RESULTS.md (1)
49-72: Specify a language for fenced code blocks to satisfy markdownlintMarkdown lints (MD040) are complaining about code fences without a language:
Result: 157.129 ±4.841 µs/op [Average]
...To fix, you can tag them as plain text, for example:
-``` +```text Result: 157.129 ±4.841 µs/op [Average] ... -``` +```Apply similarly to the other result snippets.
Also applies to: 100-109
src/test/java/com/github/_1c_syntax/bsl/languageserver/IncrementalTextChangeTest.java (1)
24-36: Tighten exception handling and imports in incremental-change testsBehavioral checks look good; a couple of small cleanups will address Sonar warnings:
- Remove unused import
Positionis not used:-import org.eclipse.lsp4j.Position;
- Avoid
throws Exceptionon tests that can't throw itCurrently most
@Testmethods declarethrows Exceptioneven though they only callapplyIncrementalChange, which doesn’t throw. You can keep tests simpler (and satisfy Sonar) by:
- Making the reflection helper handle checked exceptions internally:
- private String applyTextDocumentChanges(String content, List<TextDocumentContentChangeEvent> changes) - throws NoSuchMethodException, InvocationTargetException, IllegalAccessException { - Method method = BSLTextDocumentService.class.getDeclaredMethod( - "applyTextDocumentChanges", String.class, List.class); - method.setAccessible(true); - return (String) method.invoke(null, content, changes); - } + private String applyTextDocumentChanges(String content, List<TextDocumentContentChangeEvent> changes) { + try { + Method method = BSLTextDocumentService.class.getDeclaredMethod( + "applyTextDocumentChanges", String.class, List.class + ); + method.setAccessible(true); + return (String) method.invoke(null, content, changes); + } catch (NoSuchMethodException | IllegalAccessException | InvocationTargetException e) { + throw new RuntimeException(e); + } + }
- Then removing
throws Exceptionfrom the test methods (@Testmethods fromtestInsertAtBeginningthroughtestPreserveMixedLineEndings), since none of them call APIs that throw checked exceptions anymore.This keeps the tests focused on assertions and removes noisy exception declarations.
Also applies to: 112-126, 127-143, 145-213, 215-226
src/main/java/com/github/_1c_syntax/bsl/languageserver/BSLTextDocumentService.java (1)
401-417: Per-document executors: lifecycle looks sound, consider clearing onreset()as wellThe per-document executor setup for
didOpen/didChange/didCloselooks reasonable and should serializedidChangesafely per URI.One small lifecycle gap:
reset()currently only callscontext.clear();and does not touchdocumentExecutors. In normal server shutdown this is fine (you callshutdown()thenexit()), but in tests or any reuse of the service within the same JVM, per-document executors may linger.Consider enhancing
reset()along the lines of:public void reset() { - context.clear(); + context.clear(); + documentExecutors.values().forEach(ExecutorService::shutdownNow); + documentExecutors.clear(); }so that state and thread pools are reset together.
Also applies to: 419-451, 453-481, 569-572
src/jmh/java/com/github/_1c_syntax/bsl/languageserver/IncrementalTextChangeBenchmark.java (1)
49-50: Consider increasing warmup and measurement iterations for more reliable results.The current configuration uses very light warmup (1 iteration) and measurement (2 iterations), which may produce results with high variance. JMH best practices typically recommend at least 5 warmup iterations and 5-10 measurement iterations to achieve stable, representative performance measurements.
Apply this diff to improve benchmark stability:
-@Warmup(iterations = 1, time = 1) -@Measurement(iterations = 2, time = 1) +@Warmup(iterations = 5, time = 1) +@Measurement(iterations = 10, time = 1)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
JMH_BENCHMARK_RESULTS.md(1 hunks)build.gradle.kts(1 hunks)src/jmh/java/com/github/_1c_syntax/bsl/languageserver/IncrementalTextChangeBenchmark.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/BSLLanguageServer.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/BSLTextDocumentService.java(7 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/context/DocumentContext.java(1 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/BSLTextDocumentServiceTest.java(1 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/IncrementalTextChangeTest.java(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-02-10T17:12:56.150Z
Learnt from: nixel2007
Repo: 1c-syntax/bsl-language-server PR: 3408
File: src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/ProtectedModuleDiagnostic.java:63-66
Timestamp: 2025-02-10T17:12:56.150Z
Learning: In BSL Language Server, `documentContext.getServerContext().getConfiguration()` is guaranteed to return a non-null value, making null checks unnecessary.
Applied to files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/context/DocumentContext.java
📚 Learning: 2025-11-19T09:02:03.154Z
Learnt from: theshadowco
Repo: 1c-syntax/bsl-language-server PR: 3610
File: src/main/java/com/github/_1c_syntax/bsl/languageserver/folding/QueryPackageFoldingRangeSupplier.java:26-26
Timestamp: 2025-11-19T09:02:03.154Z
Learning: В проекте bsl-language-server используется кастомная версия ANTLR4 от 1c-syntax (io.github.1c-syntax:antlr4), которая включает базовый класс org.antlr.v4.runtime.Tokenizer с методами getAst() и getTokens(). SDBLTokenizer и BSLTokenizer наследуются от этого базового класса.
Applied to files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/context/DocumentContext.java
🪛 GitHub Check: SonarCloud Code Analysis
src/test/java/com/github/_1c_syntax/bsl/languageserver/IncrementalTextChangeTest.java
[warning] 71-71: Remove the declaration of thrown exception 'java.lang.Exception', as it cannot be thrown from method's body.
[warning] 202-202: Remove the declaration of thrown exception 'java.lang.Exception', as it cannot be thrown from method's body.
[warning] 99-99: Remove the declaration of thrown exception 'java.lang.Exception', as it cannot be thrown from method's body.
[warning] 57-57: Remove the declaration of thrown exception 'java.lang.Exception', as it cannot be thrown from method's body.
[warning] 188-188: Remove the declaration of thrown exception 'java.lang.Exception', as it cannot be thrown from method's body.
[warning] 146-146: Remove the declaration of thrown exception 'java.lang.Exception', as it cannot be thrown from method's body.
[warning] 25-25: Remove this unused import 'org.eclipse.lsp4j.Position'.
[warning] 43-43: Remove the declaration of thrown exception 'java.lang.Exception', as it cannot be thrown from method's body.
[warning] 85-85: Remove the declaration of thrown exception 'java.lang.Exception', as it cannot be thrown from method's body.
[warning] 160-160: Remove the declaration of thrown exception 'java.lang.Exception', as it cannot be thrown from method's body.
[warning] 174-174: Remove the declaration of thrown exception 'java.lang.Exception', as it cannot be thrown from method's body.
src/main/java/com/github/_1c_syntax/bsl/languageserver/BSLTextDocumentService.java
[warning] 428-428: Move the declaration of "version" closer to the code that uses it.
[warning] 469-469: Assign this magic number 30 to a well-named constant, and use the constant instead.
[warning] 658-658: Declare this local variable with "var" instead.
[warning] 685-685: Assign this magic number 2 to a well-named constant, and use the constant instead.
[warning] 657-657: Declare this local variable with "var" instead.
[warning] 684-684: Assign this magic number 2 to a well-named constant, and use the constant instead.
[warning] 656-656: Declare this local variable with "var" instead.
src/test/java/com/github/_1c_syntax/bsl/languageserver/BSLTextDocumentServiceTest.java
[failure] 85-85: Add at least one assertion to this test case.
[failure] 107-107: Add at least one assertion to this test case.
[failure] 135-135: Add at least one assertion to this test case.
🪛 LanguageTool
JMH_BENCHMARK_RESULTS.md
[grammar] ~130-~130: Ensure spelling is correct
Context: ...Acceptable latency** for large files (< 100ms for 10K lines, single edit) ✅ **Product...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
JMH_BENCHMARK_RESULTS.md
50-50: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
63-63: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (3)
src/main/java/com/github/_1c_syntax/bsl/languageserver/BSLLanguageServer.java (1)
196-210: Incremental sync setting matches new text-change pipelineSwitching
textDocumentSync.setChange(TextDocumentSyncKind.Incremental);is consistent with the new incremental change handling in
BSLTextDocumentServiceand the added tests/benchmarks. No issues spotted here.src/main/java/com/github/_1c_syntax/bsl/languageserver/BSLTextDocumentService.java (1)
595-694: Incremental change application and offset computation look correct and well-coveredThe new helpers:
applyTextDocumentChanges(...)handling full vs incremental updates,applyIncrementalChange(...)doing substring-based replacement, andgetOffset(...)scanning line breaks (handling\n,\r, and\r\nas a single line ending)match LSP expectations (0-based lines/characters, end-exclusive ranges) and preserve original line endings. The dedicated
IncrementalTextChangeTestsuite exercises a good matrix of scenarios (insert, delete, replace, multi-line, CRLF/CR/mixed), so the implementation looks solid.src/jmh/java/com/github/_1c_syntax/bsl/languageserver/IncrementalTextChangeBenchmark.java (1)
95-117: Benchmark method structure is sound.The four benchmark methods appropriately test individual changes and sequential changes. The sequential application in
benchmarkMultipleChangesis valid because each change targets a different line and only replaces text without adding or removing lines, keeping subsequent line numbers valid.However, the usefulness of these benchmarks depends on fixing the
middleLinecalculation issue in the setup method.
| // Изменение в середине документа | ||
| int middleLine = lineCount * 2; | ||
| changeInMiddle = new TextDocumentContentChangeEvent( | ||
| Ranges.create(middleLine, 2, middleLine, 15), | ||
| "Новый комментарий" | ||
| ); |
There was a problem hiding this comment.
Fix incorrect middle line calculation.
The calculation middleLine = lineCount * 2 does not point to a comment line as intended. Each procedure block consists of 5 lines (declaration, comment, return, end, blank). For lineCount = 100, the total document has 500 lines, and middleLine = 200 points to line 200, which is the procedure declaration of the 40th procedure block (line 200 / 5 = procedure 40, line 0), not a comment line.
The change event expects to modify columns 2-15 on a comment line (which starts with " // Комментарий"), but line 200 contains "Процедура Тест40()\n", making the benchmark test incorrect content.
Apply this diff to correctly target a comment line in the middle of the document:
- int middleLine = lineCount * 2;
+ int middleLine = (lineCount / 2) * 5 + 1;
changeInMiddle = new TextDocumentContentChangeEvent(
Ranges.create(middleLine, 2, middleLine, 15),
"Новый комментарий"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Изменение в середине документа | |
| int middleLine = lineCount * 2; | |
| changeInMiddle = new TextDocumentContentChangeEvent( | |
| Ranges.create(middleLine, 2, middleLine, 15), | |
| "Новый комментарий" | |
| ); | |
| // Изменение в середине документа | |
| int middleLine = (lineCount / 2) * 5 + 1; | |
| changeInMiddle = new TextDocumentContentChangeEvent( | |
| Ranges.create(middleLine, 2, middleLine, 15), | |
| "Новый комментарий" | |
| ); |
🤖 Prompt for AI Agents
In
src/jmh/java/com/github/_1c_syntax/bsl/languageserver/IncrementalTextChangeBenchmark.java
around lines 80 to 85, the middleLine calculation incorrectly uses lineCount * 2
and therefore points at a procedure declaration; replace it with an expression
that computes the middle procedure and selects its comment line, e.g. middleLine
= (lineCount / 2) * 5 + 1 (integer division) so that for N procedures you target
the comment line at offset 1 inside the middle 5-line block.
| @Test | ||
| void didChangeIncremental() throws IOException { | ||
| // given | ||
| var textDocumentItem = getTextDocumentItem(); | ||
| var didOpenParams = new DidOpenTextDocumentParams(textDocumentItem); | ||
| textDocumentService.didOpen(didOpenParams); | ||
|
|
||
| // when - incremental change: insert text at position | ||
| var params = new DidChangeTextDocumentParams(); | ||
| var uri = textDocumentItem.getUri(); | ||
| params.setTextDocument(new VersionedTextDocumentIdentifier(uri, 2)); | ||
|
|
||
| var range = Ranges.create(0, 0, 0, 0); | ||
| var changeEvent = new TextDocumentContentChangeEvent(range, "// Комментарий\n"); | ||
| List<TextDocumentContentChangeEvent> contentChanges = new ArrayList<>(); | ||
| contentChanges.add(changeEvent); | ||
| params.setContentChanges(contentChanges); | ||
|
|
||
| // then - should not throw exception | ||
| textDocumentService.didChange(params); | ||
| } | ||
|
|
There was a problem hiding this comment.
Add an explicit assertion to didChangeIncremental to satisfy Sonar
This test currently relies on the absence of an exception but has no explicit assertion, which Sonar flags.
You can make the intent explicit:
@@
@Test
void didChangeIncremental() throws IOException {
@@
- // then - should not throw exception
- textDocumentService.didChange(params);
+ // then - should not throw exception
+ assertDoesNotThrow(() -> textDocumentService.didChange(params));and add:
-import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;at the top.
Repeat the assertDoesNotThrow pattern for the other new incremental tests below.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Test | |
| void didChangeIncremental() throws IOException { | |
| // given | |
| var textDocumentItem = getTextDocumentItem(); | |
| var didOpenParams = new DidOpenTextDocumentParams(textDocumentItem); | |
| textDocumentService.didOpen(didOpenParams); | |
| // when - incremental change: insert text at position | |
| var params = new DidChangeTextDocumentParams(); | |
| var uri = textDocumentItem.getUri(); | |
| params.setTextDocument(new VersionedTextDocumentIdentifier(uri, 2)); | |
| var range = Ranges.create(0, 0, 0, 0); | |
| var changeEvent = new TextDocumentContentChangeEvent(range, "// Комментарий\n"); | |
| List<TextDocumentContentChangeEvent> contentChanges = new ArrayList<>(); | |
| contentChanges.add(changeEvent); | |
| params.setContentChanges(contentChanges); | |
| // then - should not throw exception | |
| textDocumentService.didChange(params); | |
| } | |
| @Test | |
| void didChangeIncremental() throws IOException { | |
| // given | |
| var textDocumentItem = getTextDocumentItem(); | |
| var didOpenParams = new DidOpenTextDocumentParams(textDocumentItem); | |
| textDocumentService.didOpen(didOpenParams); | |
| // when - incremental change: insert text at position | |
| var params = new DidChangeTextDocumentParams(); | |
| var uri = textDocumentItem.getUri(); | |
| params.setTextDocument(new VersionedTextDocumentIdentifier(uri, 2)); | |
| var range = Ranges.create(0, 0, 0, 0); | |
| var changeEvent = new TextDocumentContentChangeEvent(range, "// Комментарий\n"); | |
| List<TextDocumentContentChangeEvent> contentChanges = new ArrayList<>(); | |
| contentChanges.add(changeEvent); | |
| params.setContentChanges(contentChanges); | |
| // then - should not throw exception | |
| assertDoesNotThrow(() -> textDocumentService.didChange(params)); | |
| } |
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[failure] 85-85: Add at least one assertion to this test case.
🤖 Prompt for AI Agents
In
src/test/java/com/github/_1c_syntax/bsl/languageserver/BSLTextDocumentServiceTest.java
around lines 84 to 105 the test didChangeIncremental lacks an explicit assertion
(Sonar requires it); wrap the invocation of
textDocumentService.didChange(params) in an Assertions.assertDoesNotThrow(...)
(or static import assertDoesNotThrow) so the test explicitly asserts no
exception is thrown, add the corresponding import at the top of the file (e.g.
import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; or import
org.junit.jupiter.api.Assertions;) and apply the same assertDoesNotThrow pattern
to the other incremental didChange tests below.
| @Test | ||
| void didChangeIncrementalMultipleChanges() throws IOException { | ||
| // given | ||
| var textDocumentItem = getTextDocumentItem(); | ||
| var didOpenParams = new DidOpenTextDocumentParams(textDocumentItem); | ||
| textDocumentService.didOpen(didOpenParams); | ||
|
|
||
| // when - multiple incremental changes | ||
| var params = new DidChangeTextDocumentParams(); | ||
| var uri = textDocumentItem.getUri(); | ||
| params.setTextDocument(new VersionedTextDocumentIdentifier(uri, 2)); | ||
|
|
||
| List<TextDocumentContentChangeEvent> contentChanges = new ArrayList<>(); | ||
|
|
||
| // First change: insert at beginning | ||
| var range1 = Ranges.create(0, 0, 0, 0); | ||
| contentChanges.add(new TextDocumentContentChangeEvent(range1, "// Comment 1\n")); | ||
|
|
||
| // Second change: replace some text | ||
| var range2 = Ranges.create(1, 0, 1, 10); | ||
| contentChanges.add(new TextDocumentContentChangeEvent(range2, "Replaced")); | ||
|
|
||
| params.setContentChanges(contentChanges); | ||
|
|
||
| // then - should not throw exception | ||
| textDocumentService.didChange(params); | ||
| } |
There was a problem hiding this comment.
Also assert in didChangeIncrementalMultipleChanges
Same as above, wrap the call in an assertion:
- // then - should not throw exception
- textDocumentService.didChange(params);
+ // then - should not throw exception
+ assertDoesNotThrow(() -> textDocumentService.didChange(params));This makes the test intention explicit and silences the Sonar issue.
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[failure] 107-107: Add at least one assertion to this test case.
🤖 Prompt for AI Agents
In
src/test/java/com/github/_1c_syntax/bsl/languageserver/BSLTextDocumentServiceTest.java
around lines 106-132, the test calls textDocumentService.didChange(params)
without an explicit assertion; wrap that call in a JUnit assertion that verifies
no exception is thrown (e.g., assertDoesNotThrow(() ->
textDocumentService.didChange(params))) so the test intent is explicit and Sonar
warning is silenced; add the necessary static import or fully qualified
reference for the assertion if not already present.
| @Test | ||
| void didChangeIncrementalDelete() throws IOException { | ||
| // given | ||
| var textDocumentItem = getTextDocumentItem(); | ||
| var didOpenParams = new DidOpenTextDocumentParams(textDocumentItem); | ||
| textDocumentService.didOpen(didOpenParams); | ||
|
|
||
| // when - incremental change: delete text | ||
| var params = new DidChangeTextDocumentParams(); | ||
| var uri = textDocumentItem.getUri(); | ||
| params.setTextDocument(new VersionedTextDocumentIdentifier(uri, 2)); | ||
|
|
||
| var range = Ranges.create(0, 0, 0, 5); | ||
| var changeEvent = new TextDocumentContentChangeEvent(range, ""); | ||
| List<TextDocumentContentChangeEvent> contentChanges = new ArrayList<>(); | ||
| contentChanges.add(changeEvent); | ||
| params.setContentChanges(contentChanges); | ||
|
|
||
| // then - should not throw exception | ||
| textDocumentService.didChange(params); | ||
| } |
There was a problem hiding this comment.
Also assert in didChangeIncrementalDelete
Likewise, make the "no exception" expectation explicit:
- // then - should not throw exception
- textDocumentService.didChange(params);
+ // then - should not throw exception
+ assertDoesNotThrow(() -> textDocumentService.didChange(params));This keeps the test as a smoke test while satisfying the “at least one assertion” rule.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Test | |
| void didChangeIncrementalDelete() throws IOException { | |
| // given | |
| var textDocumentItem = getTextDocumentItem(); | |
| var didOpenParams = new DidOpenTextDocumentParams(textDocumentItem); | |
| textDocumentService.didOpen(didOpenParams); | |
| // when - incremental change: delete text | |
| var params = new DidChangeTextDocumentParams(); | |
| var uri = textDocumentItem.getUri(); | |
| params.setTextDocument(new VersionedTextDocumentIdentifier(uri, 2)); | |
| var range = Ranges.create(0, 0, 0, 5); | |
| var changeEvent = new TextDocumentContentChangeEvent(range, ""); | |
| List<TextDocumentContentChangeEvent> contentChanges = new ArrayList<>(); | |
| contentChanges.add(changeEvent); | |
| params.setContentChanges(contentChanges); | |
| // then - should not throw exception | |
| textDocumentService.didChange(params); | |
| } | |
| @Test | |
| void didChangeIncrementalDelete() throws IOException { | |
| // given | |
| var textDocumentItem = getTextDocumentItem(); | |
| var didOpenParams = new DidOpenTextDocumentParams(textDocumentItem); | |
| textDocumentService.didOpen(didOpenParams); | |
| // when - incremental change: delete text | |
| var params = new DidChangeTextDocumentParams(); | |
| var uri = textDocumentItem.getUri(); | |
| params.setTextDocument(new VersionedTextDocumentIdentifier(uri, 2)); | |
| var range = Ranges.create(0, 0, 0, 5); | |
| var changeEvent = new TextDocumentContentChangeEvent(range, ""); | |
| List<TextDocumentContentChangeEvent> contentChanges = new ArrayList<>(); | |
| contentChanges.add(changeEvent); | |
| params.setContentChanges(contentChanges); | |
| // then - should not throw exception | |
| assertDoesNotThrow(() -> textDocumentService.didChange(params)); | |
| } |
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[failure] 135-135: Add at least one assertion to this test case.
🤖 Prompt for AI Agents
In
src/test/java/com/github/_1c_syntax/bsl/languageserver/BSLTextDocumentServiceTest.java
around lines 134 to 154, the test only relies on not throwing but has no
explicit assertion; wrap the call to textDocumentService.didChange(params) with
a JUnit assertion that explicitly asserts no exception (e.g. use
Assertions.assertDoesNotThrow(() -> textDocumentService.didChange(params))) and
add the required static import or reference to Assertions so the test contains
at least one assertion while preserving the original behavior.
Описание
Связанные задачи
Closes
Чеклист
Общие
gradlew precommit)Для диагностик
Дополнительно
Summary by CodeRabbit
New Features
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.