Fix PR #3628 review comments: variable naming, JavaDoc, and test improvements#3632
Conversation
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the Comment |
…ovements Co-authored-by: nixel2007 <1132840+nixel2007@users.noreply.github.com>
| await().pollDelay(Duration.ofMillis(200)).until(() -> true); | ||
| // Для открытого файла событие Created должно быть проигнорировано | ||
| // Ожидаем завершения асинхронной обработки | ||
| await().atMost(Duration.ofSeconds(2)).until(() -> true); |
There was a problem hiding this comment.
Вернул оригинальное поведение с pollDelay. Оставил только улучшения для теста MultipleEvents (добавлена проверка удаления uri3). Исправлено в df5c71f.
Co-authored-by: nixel2007 <1132840+nixel2007@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR addresses code review comments from PR #3628, focusing on improving variable naming, clarifying JavaDoc documentation, and enhancing test coverage. The changes improve code readability and test robustness for file system event handling in the BSL Language Server.
Key Changes:
- Renamed
wasDocumentOpenedBeforetoisDocumentOpenedto better reflect current state rather than historical state - Updated JavaDoc to use "обработка пропускается" (processing is skipped) instead of "обработка не требуется" (processing is not required) for clearer intent
- Enhanced test coverage in
testDidChangeWatchedFiles_MultipleEventsto verify file deletion behavior
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/main/java/com/github/_1c_syntax/bsl/languageserver/BSLWorkspaceService.java | Variable rename and JavaDoc improvements in file event handlers (handleDeletedFileEvent, handleCreatedFileEvent, handleChangedFileEvent) |
| src/test/java/com/github/_1c_syntax/bsl/languageserver/BSLWorkspaceServiceTest.java | Test improvements: replaced pollDelay with atMost, added uri3 deletion verification, added explanatory comments |
Comments suppressed due to low confidence (4)
src/test/java/com/github/_1c_syntax/bsl/languageserver/BSLWorkspaceServiceTest.java:99
- The original code used
pollDelay(Duration.ofMillis(200))which ensured a minimum 200ms delay for async processing. The new codeatMost(Duration.ofSeconds(2)).until(() -> true)will return immediately since the condition is always true, providing NO actual wait time.
This could lead to flaky tests as the async operation in didChangeWatchedFiles (line 92-105 in BSLWorkspaceService.java) might not complete before assertions run.
Consider using await().pollDelay(Duration.ofMillis(100)).atMost(Duration.ofSeconds(2)).until(() -> true) to maintain the minimum wait while also having a reasonable timeout.
// then
src/test/java/com/github/_1c_syntax/bsl/languageserver/BSLWorkspaceServiceTest.java:153
- The original code used
pollDelay(Duration.ofMillis(200))which ensured a minimum 200ms delay for async processing. The new codeatMost(Duration.ofSeconds(2)).until(() -> true)will return immediately since the condition is always true, providing NO actual wait time.
This could lead to flaky tests as the async operation in didChangeWatchedFiles (line 92-105 in BSLWorkspaceService.java) might not complete before assertions run.
Consider using await().pollDelay(Duration.ofMillis(100)).atMost(Duration.ofSeconds(2)).until(() -> true) to maintain the minimum wait while also having a reasonable timeout.
// Документ должен остаться в контексте
src/test/java/com/github/_1c_syntax/bsl/languageserver/BSLWorkspaceServiceTest.java:239
- The original code used
pollDelay(Duration.ofMillis(100))which ensured a minimum 100ms delay for async processing. The new codeatMost(Duration.ofSeconds(2)).until(() -> true)will return immediately since the condition is always true, providing NO actual wait time.
This could lead to flaky tests as the async operation in didChangeWatchedFiles (line 92-105 in BSLWorkspaceService.java) might not complete before assertions run.
Consider using await().pollDelay(Duration.ofMillis(100)).atMost(Duration.ofSeconds(2)).until(() -> true) to maintain the minimum wait while also having a reasonable timeout.
src/test/java/com/github/_1c_syntax/bsl/languageserver/BSLWorkspaceServiceTest.java:126
- The wait condition
serverContext.getDocument(uri) != nullwill pass immediately because the document was already added to the context on line 113. This doesn't ensure that the async file change processing has completed.
Since the test is verifying that a changed (not opened) file is processed, consider checking a more specific condition such as document content being updated, or use a fixed delay with pollDelay similar to other tests in the file.
assertThat(serverContext.getDocument(uri)).isNotNull();
33e1f13
into
feature/handle-didChangeWatchedFiles
Описание
Исправлены замечания код-ревью из PR #3628:
BSLWorkspaceService.java:
wasDocumentOpenedBefore→isDocumentOpened(3 места) — переменная отражает текущее состояние, не предыдущееBSLWorkspaceServiceTest.java:
testDidChangeWatchedFiles_MultipleEventsСвязанные задачи
Closes #3628
Чеклист
Общие
gradlew precommit)Для диагностик
Дополнительно
Пример изменения переменной:
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.