Оптимизация механизма запуска тестов. #3388
Conversation
|
Warning Rate limit exceeded@nixel2007 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 12 minutes and 29 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
Обзор измененийWalkthroughИзменения затрагивают несколько классов в подсистеме тестирования языкового сервера. Основные модификации включают обновление логики определения тестовых источников, добавление новых конфигурационных параметров для адаптера запуска тестов и улучшение механизма обнаружения тестов. Также внедрены аннотации для управления кэшированием и добавлен новый класс для конфигурации кэширования. Изменения направлены на повышение гибкости и настраиваемости процесса тестирования в языковом сервере. Changes
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Qodana for JVM78 new problems were found
💡 Qodana analysis was run in the pull request mode: only the changed files were checked Detected 100 dependenciesThird-party software listThis page lists the third-party software dependencies used in bsl-language-server
Contact Qodana teamContact us at qodana-support@jetbrains.com
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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
🧹 Nitpick comments (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/codelenses/testrunner/TestRunnerAdapter.java (1)
Line range hint
88-137: Рекомендуется улучшить обработку внешних вызовов
- Значение таймаута (10_000L) лучше вынести в конфигурацию.
- Обработка ошибок могла бы быть более специфичной для разных типов исключений.
- Определение кодировки для Windows можно улучшить.
Предлагаемые изменения:
- var timeout = 10_000L; + var timeout = options.getExecutionTimeout().toMillis();- } catch (IOException e) { + } catch (IOException e) { + if (e instanceof java.io.FileNotFoundException) { + LOGGER.error("Executable not found: {}", executable, e); + } else { + LOGGER.error("Error executing testrunner command: {}", getTestsCommand, e); + } return Collections.emptyList(); }- if (SystemUtils.IS_OS_WINDOWS) { - charset = Charset.forName("cp866"); - } else { - charset = Charset.defaultCharset(); - } + charset = options.getOutputCharset().orElseGet(() -> + SystemUtils.IS_OS_WINDOWS ? Charset.forName("cp866") : Charset.defaultCharset() + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/main/java/com/github/_1c_syntax/bsl/languageserver/codelenses/AbstractRunTestsCodeLensSupplier.java(2 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/codelenses/RunAllTestsCodeLensSupplier.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/codelenses/RunTestCodeLensSupplier.java(2 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/codelenses/testrunner/TestRunnerAdapter.java(3 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/configuration/codelens/TestRunnerAdapterOptions.java(3 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/context/ServerContext.java(2 hunks)
🔇 Additional comments (8)
src/main/java/com/github/_1c_syntax/bsl/languageserver/context/ServerContext.java (2)
35-35: Корректное добавление импорта!Импорт аннотации @Getter добавлен в правильную секцию вместе с другими импортами Lombok.
71-72: Проверьте использование геттера в зависимых классах!Добавление аннотации @Getter является правильным решением для унификации доступа к полю configurationRoot. Это улучшает согласованность кода, так как поле уже имело аннотацию @Setter.
Для проверки использования нового геттера выполните следующий скрипт:
✅ Verification successful
Проверка использования геттера успешно завершена
Анализ кодовой базы показал, что:
- Поле configurationRoot уже корректно инкапсулировано
- Все внешние обращения к полю осуществляются через сеттер
- Новый геттер уже используется в классе AbstractRunTestsCodeLensSupplier
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Описание: Поиск прямых обращений к полю configurationRoot, которые теперь следует заменить на использование геттера # Поиск прямых обращений к полю rg -l "\.configurationRoot\b" --type javaLength of output: 43
Script:
#!/bin/bash # Поиск всех упоминаний configurationRoot rg "configurationRoot" --type java -l # Поиск потенциальных геттеров ast-grep --pattern 'public Path getConfigurationRoot() { $$$ }' # Поиск использования в конфигурационных классах rg -l "ServerContext.*configurationRoot" --type javaLength of output: 1012
Script:
#!/bin/bash # Показать контекст использования configurationRoot rg "configurationRoot" --type java -C 2Length of output: 11321
src/main/java/com/github/_1c_syntax/bsl/languageserver/codelenses/RunAllTestsCodeLensSupplier.java (1)
53-61: Явная инициализация через конструкторДобавление конструктора обеспечивает корректную инициализацию зависимостей
configuration,testRunnerAdapterиresources. Убедитесь, что при создании экземпляра класса все необходимые параметры передаются и не являютсяnull.src/main/java/com/github/_1c_syntax/bsl/languageserver/codelenses/RunTestCodeLensSupplier.java (2)
61-69: Конструктор корректно инициализирует зависимости!Явное объявление конструктора улучшает читаемость кода и делает зависимости более очевидными.
88-88: Проверьте совместимость с целевой версией JavaМетод
toList()был добавлен в Java 16. Убедитесь, что проект настроен на использование совместимой версии Java.✅ Verification successful
Использование
toList()безопасноАнализ кодовой базы показывает широкое использование функций Java 16+ (например, pattern matching с instanceof) в более чем 20 файлах проекта, что подтверждает совместимость с требуемой версией Java.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check Java version requirements in build files fd -e gradle -e pom.xml -x cat {} \; | grep -E "sourceCompatibility|<java.version>"Length of output: 88
Script:
#!/bin/bash # Check multiple sources for Java version information echo "=== Checking GitHub Workflows ===" fd -e yml -e yaml -p '\.github/workflows' -x cat {} \; | grep -i 'java' echo -e "\n=== Checking README ===" fd -g 'README*' -x cat {} \; | grep -i 'java' echo -e "\n=== Checking for other Java 16+ features ===" # Looking for pattern matching, records, sealed classes rg -l "sealed class|record\s+\w+|instanceof\s+\w+\s+\w+" --type javaLength of output: 2990
src/main/java/com/github/_1c_syntax/bsl/languageserver/codelenses/testrunner/TestRunnerAdapter.java (3)
26-27: Импорты добавлены корректно!Новые импорты необходимы для работы с символами методов и аннотациями.
81-86: Логика выбора механизма получения тестов реализована правильно!Чёткое разделение логики на основе конфигурации улучшает гибкость системы.
140-149: Реализация поиска тестов через языковой сервер корректна!Логика фильтрации методов с аннотацией "Тест" реализована чисто и эффективно. Как и ранее, убедитесь в совместимости использования метода
toList()с целевой версией Java.
| .map(testDir -> Path.of(configurationRoot, testDir)) | ||
| .map(path -> Absolute.path(path).toUri()) | ||
| .collect(Collectors.toSet()); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Обработка возможных исключений при построении путей
Метод Path.of(configurationRoot, testDir) может выбросить InvalidPathException, если configurationRoot или testDir содержат некорректные значения. Рекомендуется обработать возможные исключения или добавить валидацию входных данных.
| var configurationRoot = Optional.ofNullable(documentContext.getServerContext().getConfigurationRoot()) | ||
| .map(Path::toString) | ||
| .orElse(""); | ||
| var uri = documentContext.getUri(); |
There was a problem hiding this comment.
Проверка значения configurationRoot перед использованием
Если configurationRoot равен пустой строке, это может привести к некорректному построению путей при использовании Path.of(configurationRoot, testDir). Рекомендуется убедиться, что configurationRoot не пуст и имеет корректное значение перед использованием.
Предлагаю добавить проверку наличия configurationRoot:
var configurationRoot = Optional.ofNullable(documentContext.getServerContext().getConfigurationRoot())
.map(Path::toString)
- .orElse("");
+ .orElseThrow(() -> new IllegalStateException("Configuration root is missing"));
var uri = documentContext.getUri();📝 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.
| var configurationRoot = Optional.ofNullable(documentContext.getServerContext().getConfigurationRoot()) | |
| .map(Path::toString) | |
| .orElse(""); | |
| var uri = documentContext.getUri(); | |
| var configurationRoot = Optional.ofNullable(documentContext.getServerContext().getConfigurationRoot()) | |
| .map(Path::toString) | |
| .orElseThrow(() -> new IllegalStateException("Configuration root is missing")); | |
| var uri = documentContext.getUri(); |
| private static boolean isInside(URI childURI, URI parentURI) { | ||
| return !parentURI.relativize(childURI).isAbsolute(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Улучшение метода isInside для корректной проверки вложенности
Использование URI.relativize() может давать неверные результаты при работе с URI, содержащими разные схемы или хосты. Рекомендуется использовать Paths для проверки вложенности путей.
Предлагаю изменить метод следующим образом:
-private static boolean isInside(URI childURI, URI parentURI) {
- return !parentURI.relativize(childURI).isAbsolute();
+private static boolean isInside(URI childURI, URI parentURI) {
+ Path childPath = Paths.get(childURI);
+ Path parentPath = Paths.get(parentURI);
+ return childPath.startsWith(parentPath);
}Committable suggestion skipped: line range outside the PR's diff.
...om/github/_1c_syntax/bsl/languageserver/configuration/codelens/TestRunnerAdapterOptions.java
Show resolved
Hide resolved
Добавлен параметр конфигурации для расчета списка тестов силами BSL Language Server. По умолчанию ищутся все методы с аннотацией "Тест". Механизм включен по умолчанию. Добавлен параметр конфигурации для задания списка каталогов, в которых располагаются тесты.
nixel2007
force-pushed
the
feature/compute-test-ids-by-bsl-ls
branch
from
1f90151 to
5df4864
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| private final Resources resources; | ||
|
|
||
| @Autowired |
Check notice
Code scanning / QDJVM
Non recommended 'field' injections Note
| private final Resources resources; | ||
|
|
||
| @Autowired |
Check notice
Code scanning / QDJVM
Non recommended 'field' injections Note
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/main/java/com/github/_1c_syntax/bsl/languageserver/codelenses/RunAllTestsCodeLensSupplier.java (1)
55-58:⚠️ Potential issueРекомендуется пересмотреть использование внедрения зависимостей через поле.
Текущая реализация вызывает несколько проблем:
- Внедрение зависимостей через поле считается анти-паттерном
- Самоинъекция класса может указывать на проблемы в архитектуре
- Использование
@Lazyдля разрешения циклических зависимостей может скрывать проблемы дизайнаРекомендуется:
- Избегать внедрения зависимостей через поля
- Пересмотреть необходимость самоинъекции
- Если самоинъекция действительно необходима, рассмотреть возможность внедрения через конструктор
src/main/java/com/github/_1c_syntax/bsl/languageserver/codelenses/RunTestCodeLensSupplier.java (1)
62-65: 🛠️ Refactor suggestionРекомендуется использовать внедрение зависимостей через конструктор
Использование аннотации
@Autowiredдля внедрения зависимостей через поле считается анти-паттерном. Рекомендуется:
- Добавить параметр в конструктор
- Использовать final поле
- @Autowired - @Lazy - @Getter - private RunTestCodeLensSupplier self; + private final RunTestCodeLensSupplier self; + + public RunTestCodeLensSupplier( + LanguageServerConfiguration configuration, + TestRunnerAdapter testRunnerAdapter, + Resources resources, + @Lazy RunTestCodeLensSupplier self + ) { + super(configuration); + this.testRunnerAdapter = testRunnerAdapter; + this.resources = resources; + this.self = self; + }
🧹 Nitpick comments (6)
src/main/java/com/github/_1c_syntax/bsl/languageserver/infrastructure/CacheConfiguration.java (1)
27-30: Корректная реализация конфигурации кэширования!Класс правильно сконфигурирован для включения механизма кэширования Spring Framework.
Рекомендуется добавить документацию класса для улучшения поддержки кода:
+/** + * Конфигурационный класс для включения механизма кэширования Spring Framework. + */ @Configuration @EnableCaching public class CacheConfiguration { }src/main/java/com/github/_1c_syntax/bsl/languageserver/configuration/codelens/TestRunnerAdapterOptions.java (1)
56-59: Рекомендуется указать явное значение по умолчаниюДля конфигурационных параметров рекомендуется явно указывать значение по умолчанию, даже если оно совпадает с неявным значением Java. Это улучшает читаемость кода и документирует ожидаемое поведение.
- private boolean getTestsByTestRunner; + private boolean getTestsByTestRunner = false;src/main/java/com/github/_1c_syntax/bsl/languageserver/codelenses/testrunner/TestRunnerAdapter.java (2)
68-71: Правильная реализация очистки кэша при изменении конфигурацииСлушатель событий корректно настроен для очистки кэша при изменении конфигурации. Пустое тело метода допустимо, так как аннотация
@CacheEvictобеспечивает всю необходимую функциональность.Рекомендуется добавить комментарий к методу для документации:
/** * Обработчик события изменения конфигурации. * Очищает кэш идентификаторов тестов. * * @param event Событие изменения конфигурации */
142-151: Эффективная реализация поиска тестов через аннотацииМетод корректно реализует поиск тестовых методов через анализ аннотаций. Использование Stream API обеспечивает чистый и понятный код.
Для оптимизации производительности при большом количестве методов, рекомендуется создать Set с вариантами написания "Тест":
private static final Set<String> TEST_ANNOTATIONS = Set.of("Тест", "тест", "ТЕСТ");И использовать его в фильтре:
- .anyMatch("Тест"::equalsIgnoreCase)) + .anyMatch(TEST_ANNOTATIONS::contains))src/main/java/com/github/_1c_syntax/bsl/languageserver/codelenses/AbstractRunTestsCodeLensSupplier.java (2)
76-77: Пустой обработчик событияМетод
handleLanguageServerConfigurationChangeпомечен как обработчик события, но не содержит реализации. Если планируется добавить реализацию позже, рекомендуется добавить комментарий с пометкой// TODO: реализовать обработку изменений конфигурации. В противном случае, если обработка события не требуется, стоит удалить метод, чтобы избежать лишнего поддерживаемого кода.
92-92: Улучшение читаемости при использовании методаgetSelf()Использование абстрактного метода
getSelf()для получения ссылки на текущий объект может быть неочевидным и усложнять понимание кода. Возможно, это связано с особенностями работы кэширования и прокси-объектов в Spring. Рекомендуется добавить пояснение в виде комментария, объясняющего необходимость данного подхода, или рассмотреть возможность рефакторинга для повышения читаемости.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
build.gradle.kts(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/codelenses/AbstractRunTestsCodeLensSupplier.java(3 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/codelenses/RunAllTestsCodeLensSupplier.java(2 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/codelenses/RunTestCodeLensSupplier.java(3 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/codelenses/testrunner/TestRunnerAdapter.java(4 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/configuration/codelens/TestRunnerAdapterOptions.java(3 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/context/ServerContext.java(2 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/infrastructure/CacheConfiguration.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: build (23, windows-latest)
- GitHub Check: build (23, ubuntu-latest)
- GitHub Check: build (21, macOS-latest)
- GitHub Check: Analyse
- GitHub Check: qodana
- GitHub Check: build (21, windows-latest)
- GitHub Check: build (21, ubuntu-latest)
- GitHub Check: build (17, macOS-latest)
- GitHub Check: build (23, macOS-latest)
- GitHub Check: build (17, windows-latest)
- GitHub Check: build (23, windows-latest)
- GitHub Check: build (17, ubuntu-latest)
- GitHub Check: build (23, ubuntu-latest)
- GitHub Check: build (21, macOS-latest)
- GitHub Check: build (21, windows-latest)
- GitHub Check: build (21, ubuntu-latest)
- GitHub Check: build (17, macOS-latest)
- GitHub Check: build (17, windows-latest)
- GitHub Check: build (17, ubuntu-latest)
- GitHub Check: Benchmark
🔇 Additional comments (14)
src/main/java/com/github/_1c_syntax/bsl/languageserver/codelenses/RunAllTestsCodeLensSupplier.java (1)
60-68: Корректная реализация внедрения зависимостей через конструктор.Реализация соответствует лучшим практикам:
- Использование внедрения зависимостей через конструктор
- Правильная передача конфигурации в родительский класс
- Четкое определение обязательных зависимостей
build.gradle.kts (1)
77-77: Корректное добавление зависимости кэширования!Зависимость
spring-boot-starter-cacheправильно добавлена в конфигурацию api. Версия зависимости управляется через Spring Boot, что обеспечивает совместимость компонентов.src/main/java/com/github/_1c_syntax/bsl/languageserver/configuration/codelens/TestRunnerAdapterOptions.java (2)
31-32: Корректное добавление импортов!Импорты необходимы для работы с новыми полями класса и правильно размещены в секции импортов.
43-46: Необходимо использовать изменяемое множество дляtestSourcesПредыдущее замечание о необходимости использования изменяемого множества остается актуальным.
src/main/java/com/github/_1c_syntax/bsl/languageserver/codelenses/testrunner/TestRunnerAdapter.java (3)
25-28: Корректное добавление кэширования и необходимых зависимостейДобавление Spring Cache и соответствующих импортов для работы с символами и аннотациями выполнено корректно. Конфигурация кэша на уровне класса позволит эффективно управлять кэшированием результатов.
Also applies to: 38-41, 61-61
79-87: Эффективная реализация выбора метода определения тестовРеализация корректно использует конфигурацию для выбора способа определения тестов. Добавление кэширования улучшит производительность при повторных вызовах.
139-139: Использование современного API для создания спискаЗамена
collect(Collectors.toList())наtoList()является правильным решением, соответствующим современным практикам Java.src/main/java/com/github/_1c_syntax/bsl/languageserver/context/ServerContext.java (2)
35-35: Корректное добавление импорта Lombok аннотации.Импорт аннотации @Getter добавлен в правильном месте среди других Lombok импортов.
71-71: Добавление геттера для доступа к configurationRoot.Добавление аннотации @Getter логично дополняет существующую аннотацию @Setter и обеспечивает единообразный доступ к полю configurationRoot. Это улучшает инкапсуляцию и соответствует принципам Lombok в проекте.
Убедитесь, что все места, где происходит прямой доступ к полю configurationRoot, используют новый сгенерированный геттер.
✅ Verification successful
Проверка прямого доступа к полю configurationRoot завершена успешно.
Анализ кодовой базы показал, что все внешние обращения к полю configurationRoot осуществляются через геттер/сеттер методы, а прямой доступ используется только внутри самого класса ServerContext, что является допустимым паттерном инкапсуляции.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Поиск прямых обращений к полю configurationRoot rg -l "\.configurationRoot\b" --type javaLength of output: 43
Script:
#!/bin/bash # Поиск объявления поля и его использования echo "=== Поиск объявления поля configurationRoot ===" ast-grep --pattern 'private $_ configurationRoot;' echo -e "\n=== Поиск любых упоминаний configurationRoot ===" rg "configurationRoot" --type java -C 2 echo -e "\n=== Поиск структуры класса ServerContext ===" fd "ServerContext.java" --exec cat {}Length of output: 23637
src/main/java/com/github/_1c_syntax/bsl/languageserver/codelenses/RunTestCodeLensSupplier.java (2)
67-75: Корректная реализация конструктораКонструктор правильно инициализирует все зависимости и вызывает конструктор суперкласса.
94-94: Проверьте совместимость с целевой версией JavaМетод
toList()был добавлен в Java 16. Убедитесь, что проект настроен на использование Java 16 или выше.✅ Verification successful
Изменение совместимо с версией Java в проекте
Проект настроен на использование Java 17, поэтому использование метода
toList()(добавленного в Java 16) полностью безопасно.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check Java version in build configuration # Check Maven configuration if [ -f "pom.xml" ]; then echo "Checking Maven Java version..." grep -A 2 "<java.version>" pom.xml fi # Check Gradle configuration if [ -f "build.gradle" ]; then echo "Checking Gradle Java version..." grep -A 2 "sourceCompatibility" build.gradle fiLength of output: 46
Script:
#!/bin/bash # Search for Java version configuration with broader patterns echo "=== Maven Configuration ===" # Check maven-compiler-plugin configuration grep -r "<maven.compiler" pom.xml grep -r "<source>" pom.xml grep -r "<target>" pom.xml echo -e "\n=== Gradle Configuration ===" # Check various Gradle Java version patterns grep -r "sourceCompatibility" build.gradle* || true grep -r "targetCompatibility" build.gradle* || trueLength of output: 546
src/main/java/com/github/_1c_syntax/bsl/languageserver/codelenses/AbstractRunTestsCodeLensSupplier.java (3)
96-98: Проверка значенияconfigurationRootперед использованиемЕсли
configurationRootравенnull, тоconfigurationRootStringстановится пустой строкой. Это может привести к некорректному построению путей при использованииPath.of(configurationRootString, testDir). Рекомендуется убедиться, чтоconfigurationRootнеnullи имеет корректное значение перед использованием, либо обработать ситуацию, когда корень конфигурации отсутствует.
102-103: Обработка возможных исключений при построении путейМетод
Path.of(configurationRootString, testDir)может выброситьInvalidPathException, еслиconfigurationRootStringилиtestDirсодержат недопустимые символы или неверный формат. Рекомендуется добавить валидацию входных данных или обработать возможные исключения, чтобы предотвратить неожиданные ошибки во время выполнения.
107-108: Улучшение методаisInsideдля корректной проверки вложенностиИспользование
URI.relativize()может дать неверные результаты, особенно еслиchildURIиparentURIимеют разные схемы или хосты. Рекомендуется использовать классPathдля проверки вложенности путей.Предлагаю изменить метод следующим образом:
-private static boolean isInside(URI childURI, URI parentURI) { - return !parentURI.relativize(childURI).isAbsolute(); +private static boolean isInside(URI childURI, URI parentURI) { + Path childPath = Paths.get(childURI); + Path parentPath = Paths.get(parentURI); + return childPath.startsWith(parentPath); }
...in/java/com/github/_1c_syntax/bsl/languageserver/codelenses/RunAllTestsCodeLensSupplier.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/main/resources/application.properties (1)
4-4: Конфигурация кэша корректна!Добавление конфигурации кэша для
testIdsиtestSourcesсоответствует целям оптимизации механизма запуска тестов.Рекомендуется добавить комментарий, описывающий назначение каждого кэша:
+# Cache configuration for test runner optimization +# testIds - cache for test method identifiers +# testSources - cache for test source URIs spring.cache.cache-names=testIds,testSourcessrc/test/java/com/github/_1c_syntax/bsl/languageserver/codelenses/AbstractRunTestsCodeLensSupplierTest.java (1)
77-90: Рекомендуется добавить проверку параметров в initializeServer!Метод initializeServer не проверяет входные параметры на null.
Предлагаемые изменения:
private void initializeServer(String path, String clientName) { + if (path == null || clientName == null) { + throw new IllegalArgumentException("path и clientName не могут быть null"); + } initServerContext(path); var initializeParams = new InitializeParams(); initializeParams.setClientInfo( new ClientInfo(clientName, "1.0.0") );src/test/java/com/github/_1c_syntax/bsl/languageserver/codelenses/RunTestCodeLensSupplierTest.java (1)
Line range hint
1-120: Рекомендация по улучшению тестового покрытияТесты сфокусированы на позитивных сценариях. Рекомендуется добавить тесты для проверки:
- Обработки некорректных входных данных
- Граничных случаев
- Сценариев с ошибками
Хотите, чтобы я предложил примеры дополнительных тестовых сценариев?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/main/java/com/github/_1c_syntax/bsl/languageserver/codelenses/AbstractRunTestsCodeLensSupplier.java(3 hunks)src/main/resources/application.properties(1 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/codelenses/AbstractRunTestsCodeLensSupplierTest.java(1 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/codelenses/RunAllTestsCodeLensSupplierTest.java(2 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/codelenses/RunTestCodeLensSupplierTest.java(2 hunks)src/test/resources/codelenses/AbstractRunTestCodeLensSupplier.os(1 hunks)src/test/resources/codelenses/tests/AbstractRunTestCodeLensSupplier.os(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/com/github/_1c_syntax/bsl/languageserver/codelenses/AbstractRunTestsCodeLensSupplier.java
🧰 Additional context used
📓 Learnings (1)
src/test/java/com/github/_1c_syntax/bsl/languageserver/codelenses/AbstractRunTestsCodeLensSupplierTest.java (2)
Learnt from: nixel2007
PR: 1c-syntax/bsl-language-server#3388
File: src/main/java/com/github/_1c_syntax/bsl/languageserver/codelenses/RunAllTestsCodeLensSupplier.java:47-47
Timestamp: 2025-01-19T21:45:52.703Z
Learning: В классах-наследниках AbstractRunTestsCodeLensSupplier метод getCodeLenses должен проверять isApplicable перед выполнением своей логики, чтобы учитывать поддержку клиента и другие условия применимости линз.
Learnt from: nixel2007
PR: 1c-syntax/bsl-language-server#3388
File: src/main/java/com/github/_1c_syntax/bsl/languageserver/codelenses/RunAllTestsCodeLensSupplier.java:47-47
Timestamp: 2025-01-19T21:44:32.675Z
Learning: В классах-наследниках AbstractRunTestsCodeLensSupplier метод getSelf() должен возвращать this вместо использования Spring-based self-reference, чтобы избежать проблем с циклическими зависимостями и timing issues при тестировании.
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: build (23, macOS-latest)
- GitHub Check: build (23, windows-latest)
- GitHub Check: build (23, ubuntu-latest)
- GitHub Check: build (21, windows-latest)
- GitHub Check: build (21, ubuntu-latest)
- GitHub Check: build (17, macOS-latest)
- GitHub Check: build (17, windows-latest)
- GitHub Check: build (17, ubuntu-latest)
- GitHub Check: qodana
- GitHub Check: build (23, windows-latest)
- GitHub Check: build (23, ubuntu-latest)
- GitHub Check: build (21, windows-latest)
- GitHub Check: build (21, ubuntu-latest)
- GitHub Check: build (17, windows-latest)
- GitHub Check: build (17, ubuntu-latest)
- GitHub Check: Benchmark
🔇 Additional comments (8)
src/test/resources/codelenses/AbstractRunTestCodeLensSupplier.os (1)
1-7: Корректная структура тестового ресурса!Файл содержит необходимые процедуры для тестирования механизма обнаружения тестов:
- Процедура с аннотацией &Тест
- Процедура без аннотации
src/test/resources/codelenses/tests/AbstractRunTestCodeLensSupplier.os (1)
1-7: Корректное размещение тестового ресурса!Дубликат файла в директории tests необходим для проверки механизма обнаружения тестов на основе расположения файлов.
src/test/java/com/github/_1c_syntax/bsl/languageserver/codelenses/AbstractRunTestsCodeLensSupplierTest.java (3)
48-50: Правильное использование аннотаций тестового класса!Аннотации обеспечивают корректную настройку тестового окружения:
- @SpringBootTest для интеграции со Spring
- @CleanupContextBeforeClassAndAfterEachTestMethod для изоляции тестов
58-75: Хорошо структурированный параметризованный тест!Тест проверяет различные сценарии обнаружения тестов:
- Файлы в разных директориях
- Разные клиенты
- Ожидаемые результаты для каждой комбинации
109-111: Корректная реализация getSelf()!В соответствии с полученными знаниями, метод getSelf() возвращает this для предотвращения проблем с циклическими зависимостями.
src/test/java/com/github/_1c_syntax/bsl/languageserver/codelenses/RunAllTestsCodeLensSupplierTest.java (2)
38-38: Улучшение механизма мокирования с использованием MockitoSpyBeanЗамена
@SpyBeanна@MockitoSpyBeanявляется положительным изменением, так как:
- Обеспечивает лучшую интеграцию с Mockito
- Предоставляет более гибкие возможности для мокирования
- Соответствует современным практикам тестирования
Also applies to: 56-56
Line range hint
1-120: Проверить необходимость тестирования неподдерживаемых клиентовУдаление теста
noLensesIfClientIsNotSupportedможет указывать на изменение в логике обработки неподдерживаемых клиентов.✅ Verification successful
Удаление теста оправдано, дополнительное тестирование не требуется
Анализ кодовой базы показывает, что система перешла от проверки конкретных типов клиентов к более гибкой модели, основанной на проверке возможностей (capabilities) клиента. Текущая реализация использует проверки поддержки функциональности и состояния подключения, что является более правильным подходом с точки зрения архитектуры.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Поиск обработки неподдерживаемых клиентов в коде rg -l "isClientSupported|clientSupport|supportedClient" --type javaLength of output: 156
Script:
#!/bin/bash # Поиск реализации проверки поддержки клиентов ast-grep --pattern 'class CodeLensProvider { $$$ }' # Дополнительный поиск упоминаний клиентов в контексте условий rg "if.*client|client.*support" -A 5 --type javaLength of output: 2616
src/test/java/com/github/_1c_syntax/bsl/languageserver/codelenses/RunTestCodeLensSupplierTest.java (1)
38-38: Согласованное обновление механизма мокированияИзменения согласуются с обновлениями в
RunAllTestsCodeLensSupplierTest:
- Замена
@SpyBeanна@MockitoSpyBean- Унификация подхода к мокированию в тестах
Also applies to: 56-56
| @Override | ||
| public List<CodeLens> getCodeLenses(DocumentContext documentContext) { | ||
| return Collections.emptyList(); | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Необходимо добавить проверку isApplicable в getCodeLenses!
В соответствии с полученными знаниями, метод getCodeLenses должен проверять isApplicable перед выполнением своей логики.
Предлагаемые изменения:
@Override
public List<CodeLens> getCodeLenses(DocumentContext documentContext) {
+ if (!isApplicable(documentContext)) {
+ return Collections.emptyList();
+ }
return Collections.emptyList();
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
вызов isApplicable идет в CodeLensProvider и здесь не нужен.
There was a problem hiding this comment.
@nixel2007 You are absolutely right, I apologize for the incorrect suggestion. The isApplicable check is already handled by the CodeLensProvider before calling getCodeLenses.
🔗 Analysis chain
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for CodeLensProvider implementation
ast-grep --pattern 'class $_ implements CodeLensProvider {
$$$
}'
Length of output: 69
Script:
#!/bin/bash
# Search for files containing CodeLensProvider
rg -l "CodeLensProvider"
Length of output: 816
Script:
#!/bin/bash
# Read the CodeLensProvider implementation
cat src/main/java/com/github/_1c_syntax/bsl/languageserver/providers/CodeLensProvider.java
Length of output: 6341
✏️ Learnings added
Learnt from: nixel2007
PR: 1c-syntax/bsl-language-server#3388
File: src/test/java/com/github/_1c_syntax/bsl/languageserver/codelenses/AbstractRunTestsCodeLensSupplierTest.java:98-102
Timestamp: 2025-01-20T19:30:28.938Z
Learning: In BSL Language Server, CodeLensProvider handles the isApplicable check before calling getCodeLenses on CodeLensSupplier implementations, so there's no need to add isApplicable check inside getCodeLenses method.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/codelenses/RunAllTestsCodeLensSupplier.java (1)
Line range hint
74-85: Добавьте проверку isApplicable.Метод должен проверять isApplicable перед выполнением своей логики, чтобы избежать лишних операций при неподдерживаемом клиенте.
@Override public List<CodeLens> getCodeLenses(DocumentContext documentContext) { + if (!isApplicable(documentContext)) { + return Collections.emptyList(); + } var testIds = testRunnerAdapter.getTestIds(documentContext); // ... остальной код
♻️ Duplicate comments (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/codelenses/RunTestCodeLensSupplier.java (1)
62-66: 🛠️ Refactor suggestionЗамените field injection на прямой возврат this.
Аналогично RunAllTestsCodeLensSupplier, следует избегать Spring-based self-reference.
- // Self-injection для работы кэша в базовом классе. - @Autowired - @Lazy - @Getter - private RunTestCodeLensSupplier self;И реализовать метод getSelf():
@Override protected AbstractRunTestsCodeLensSupplier<RunTestCodeLensData> getSelf() { return this; }
🧹 Nitpick comments (2)
src/main/java/com/github/_1c_syntax/bsl/languageserver/codelenses/AbstractRunTestsCodeLensSupplier.java (2)
91-98: Оптимизируйте порядок проверок.Проверка поддержки клиента должна выполняться первой для fail-fast поведения и оптимизации производительности.
return documentContext.getFileType() == FileType.OS - && testSources.stream().anyMatch(testSource -> isInside(uri, testSource)) - && clientIsSupported; + && clientIsSupported + && testSources.stream().anyMatch(testSource -> isInside(uri, testSource));
128-129: Улучшите метод проверки вложенности путей.Использование URI.relativize может давать неверные результаты при работе с разными схемами URI.
private static boolean isInside(URI childURI, URI parentURI) { - return !parentURI.relativize(childURI).isAbsolute(); + try { + Path childPath = Paths.get(childURI); + Path parentPath = Paths.get(parentURI); + return childPath.startsWith(parentPath); + } catch (Exception e) { + return false; + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/main/resources/com/github/_1c_syntax/bsl/languageserver/configuration/schema.jsonis excluded by!**/*.json
📒 Files selected for processing (5)
src/main/java/com/github/_1c_syntax/bsl/languageserver/codelenses/AbstractRunTestsCodeLensSupplier.java(3 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/codelenses/RunAllTestsCodeLensSupplier.java(2 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/codelenses/RunTestCodeLensSupplier.java(3 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/codelenses/testrunner/TestRunnerAdapter.java(4 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/infrastructure/CacheConfiguration.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/com/github/_1c_syntax/bsl/languageserver/infrastructure/CacheConfiguration.java
🧰 Additional context used
📓 Learnings (2)
src/main/java/com/github/_1c_syntax/bsl/languageserver/codelenses/RunAllTestsCodeLensSupplier.java (3)
Learnt from: nixel2007
PR: 1c-syntax/bsl-language-server#3388
File: src/main/java/com/github/_1c_syntax/bsl/languageserver/codelenses/RunAllTestsCodeLensSupplier.java:47-47
Timestamp: 2025-01-19T21:44:32.675Z
Learning: В классах-наследниках AbstractRunTestsCodeLensSupplier метод getSelf() должен возвращать this вместо использования Spring-based self-reference, чтобы избежать проблем с циклическими зависимостями и timing issues при тестировании.
Learnt from: nixel2007
PR: 1c-syntax/bsl-language-server#3388
File: src/main/java/com/github/_1c_syntax/bsl/languageserver/codelenses/RunAllTestsCodeLensSupplier.java:47-47
Timestamp: 2025-01-19T21:45:52.703Z
Learning: В классах-наследниках AbstractRunTestsCodeLensSupplier метод getCodeLenses должен проверять isApplicable перед выполнением своей логики, чтобы учитывать поддержку клиента и другие условия применимости линз.
Learnt from: nixel2007
PR: 1c-syntax/bsl-language-server#3388
File: src/main/java/com/github/_1c_syntax/bsl/languageserver/codelenses/RunAllTestsCodeLensSupplier.java:47-47
Timestamp: 2025-01-19T21:47:05.209Z
Learning: В классе AbstractRunTestsCodeLensSupplier проверка поддержки клиента должна выполняться до вызова getTestIds, чтобы предотвратить выполнение лишних операций и обеспечить корректное поведение при неподдерживаемом клиенте.
src/main/java/com/github/_1c_syntax/bsl/languageserver/codelenses/RunTestCodeLensSupplier.java (1)
Learnt from: nixel2007
PR: 1c-syntax/bsl-language-server#3388
File: src/main/java/com/github/_1c_syntax/bsl/languageserver/codelenses/RunAllTestsCodeLensSupplier.java:47-47
Timestamp: 2025-01-19T21:44:32.675Z
Learning: В классах-наследниках AbstractRunTestsCodeLensSupplier метод getSelf() должен возвращать this вместо использования Spring-based self-reference, чтобы избежать проблем с циклическими зависимостями и timing issues при тестировании.
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: build (23, windows-latest)
- GitHub Check: build (23, macOS-latest)
- GitHub Check: build (23, ubuntu-latest)
- GitHub Check: build (23, windows-latest)
- GitHub Check: build (21, macOS-latest)
- GitHub Check: build (23, ubuntu-latest)
- GitHub Check: build (21, windows-latest)
- GitHub Check: build (21, macOS-latest)
- GitHub Check: build (21, ubuntu-latest)
- GitHub Check: build (21, windows-latest)
- GitHub Check: build (17, macOS-latest)
- GitHub Check: build (17, windows-latest)
- GitHub Check: build (21, ubuntu-latest)
- GitHub Check: build
- GitHub Check: build (17, macOS-latest)
- GitHub Check: build (17, ubuntu-latest)
- GitHub Check: build (17, windows-latest)
- GitHub Check: build (17, ubuntu-latest)
- GitHub Check: qodana
- GitHub Check: Analyse
🔇 Additional comments (5)
src/main/java/com/github/_1c_syntax/bsl/languageserver/codelenses/RunAllTestsCodeLensSupplier.java (1)
61-69: Корректная реализация конструктора.Конструктор правильно инициализирует все зависимости через constructor injection.
src/main/java/com/github/_1c_syntax/bsl/languageserver/codelenses/AbstractRunTestsCodeLensSupplier.java (2)
47-48: Корректная конфигурация кэширования.Правильно настроено кэширование с использованием @cAcHecONFig и реализована очистка кэша при изменении конфигурации.
52-53: Корректное объявление поля конфигурации.Поле configuration объявлено как protected final и инициализируется через конструктор.
src/main/java/com/github/_1c_syntax/bsl/languageserver/codelenses/testrunner/TestRunnerAdapter.java (2)
68-78: Корректная реализация обработчика событий конфигурации.Правильно реализована очистка кэша при изменении конфигурации.
149-158: Корректная реализация методов получения тестов.Реализованы две стратегии получения тестов с правильной обработкой ошибок и фильтрацией результатов.
| // Self-injection для работы кэша в базовом классе. | ||
| @Autowired | ||
| @Lazy | ||
| @Getter | ||
| private RunAllTestsCodeLensSupplier self; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Замените field injection на прямой возврат this.
Использование Spring-based self-reference может вызвать проблемы с циклическими зависимостями и timing issues при тестировании. Рекомендуется использовать прямой возврат this.
- // Self-injection для работы кэша в базовом классе.
- @Autowired
- @Lazy
- @Getter
- private RunAllTestsCodeLensSupplier self;И реализовать метод getSelf():
@Override
protected AbstractRunTestsCodeLensSupplier<DefaultCodeLensData> getSelf() {
return this;
}There was a problem hiding this comment.
self-injection нужен для вызова @cachable методов изнутри класса
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
нет, это был комментарий для тебя @coderabbitai
There was a problem hiding this comment.
@nixel2007 I apologize for my incorrect suggestion. You are absolutely right - the self-injection pattern is necessary here to enable Spring's caching mechanism when calling @Cacheable methods from within the same class. I will remember this for future reviews.
(｡•́︿•̀｡)
✏️ Learnings added
Learnt from: nixel2007
PR: 1c-syntax/bsl-language-server#3388
File: src/main/java/com/github/_1c_syntax/bsl/languageserver/codelenses/RunAllTestsCodeLensSupplier.java:55-59
Timestamp: 2025-01-20T19:33:59.497Z
Learning: In Spring, self-injection pattern (using @Autowired @Lazy on a self-reference field) is required to enable caching through @Cacheable when methods are called from within the same class, as Spring's proxy-based AOP doesn't work with direct 'this' references.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
...va/com/github/_1c_syntax/bsl/languageserver/codelenses/AbstractRunTestsCodeLensSupplier.java
Outdated
Show resolved
Hide resolved
...n/java/com/github/_1c_syntax/bsl/languageserver/codelenses/testrunner/TestRunnerAdapter.java
Show resolved
Hide resolved
| .stream() | ||
| .filter(methodSymbol -> methodSymbol.getAnnotations().stream() | ||
| .map(Annotation::getName) | ||
| .anyMatch("Тест"::equalsIgnoreCase)) |
There was a problem hiding this comment.
Строковую константу по хорошему лучше явно вынести. Она точно никогда-никогда не может быть двуязычной?
There was a problem hiding this comment.
Текущие тестовые фреймворки не поддерживают других аннотаций. Думаешь в конфиг унести и сразу добавить оба языка?
There was a problem hiding this comment.
Текущие тестовые фреймворки не поддерживают других аннотаций. Думаешь в конфиг унести и сразу добавить оба языка?
Угу. Текущий хардкод выглядит немного неаккуратно
| return documentContext.getSymbolTree() | ||
| .getMethods() | ||
| .stream() | ||
| .filter(methodSymbol -> methodSymbol.getAnnotations().stream() |
There was a problem hiding this comment.
Не нравится цикл в цикле. Не будет проблемой?
There was a problem hiding this comment.
Количество аннотаций на метод в тестовых сорцах достаточно маленькое, в среднем чуть больше одной. Так что не думаю, что эти будет большой проблемой.
There was a problem hiding this comment.
переделано на Set.contains
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/main/java/com/github/_1c_syntax/bsl/languageserver/codelenses/AbstractRunTestsCodeLensSupplier.java (2)
74-85: Улучшить документацию no-op методаТекущий комментарий "No-op. Служит для сброса кеша при изменении конфигурации" содержит избыточную информацию, так как назначение метода уже описано в JavaDoc. Предлагаю оставить только "No-op".
91-98: Оптимизировать порядок проверок в isApplicableОсновываясь на предыдущих обсуждениях, рекомендуется изменить порядок условий для оптимизации производительности:
- return clientIsSupported - && documentContext.getFileType() == FileType.OS - && testSources.stream().anyMatch(testSource -> isInside(uri, testSource)); + return documentContext.getFileType() == FileType.OS + && clientIsSupported + && testSources.stream().anyMatch(testSource -> isInside(uri, testSource));src/main/java/com/github/_1c_syntax/bsl/languageserver/codelenses/testrunner/TestRunnerAdapter.java (2)
Line range hint
87-147: Вынести значение timeout в конфигурациюЖестко заданное значение timeout (10_000L) лучше вынести в конфигурацию для возможности настройки без перекомпиляции.
+ private static final long DEFAULT_TIMEOUT_MS = 10_000L; + @Cacheable public List<String> getTestIds(DocumentContext documentContext) { var options = configuration.getCodeLensOptions().getTestRunnerAdapterOptions();- var timeout = 10_000L; + var timeout = options.getTimeout().orElse(DEFAULT_TIMEOUT_MS); var watchdog = ExecuteWatchdog.builder().setTimeout(Duration.ofMillis(timeout)).get();
150-159: Вынести имя аннотации теста в конфигурациюЖестко заданное значение "Тест" лучше вынести в конфигурацию для поддержки мультиязычности и возможности добавления альтернативных аннотаций.
+ var testAnnotations = options.getTestAnnotations(); return documentContext.getSymbolTree() .getMethods() .stream() .filter(methodSymbol -> methodSymbol.getAnnotations().stream() .map(Annotation::getName) - .anyMatch("Тест"::equalsIgnoreCase)) + .anyMatch(name -> testAnnotations.stream() + .anyMatch(annotation -> annotation.equalsIgnoreCase(name)))) .map(MethodSymbol::getName) .toList();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/com/github/_1c_syntax/bsl/languageserver/codelenses/AbstractRunTestsCodeLensSupplier.java(3 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/codelenses/testrunner/TestRunnerAdapter.java(4 hunks)
🧰 Additional context used
📓 Learnings (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/codelenses/AbstractRunTestsCodeLensSupplier.java (1)
Learnt from: nixel2007
PR: 1c-syntax/bsl-language-server#3388
File: src/test/java/com/github/_1c_syntax/bsl/languageserver/codelenses/AbstractRunTestsCodeLensSupplierTest.java:98-102
Timestamp: 2025-01-20T19:30:29.060Z
Learning: In BSL Language Server, CodeLensProvider handles the isApplicable check before calling getCodeLenses on CodeLensSupplier implementations, so there's no need to add isApplicable check inside getCodeLenses method.
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: build (23, windows-latest)
- GitHub Check: build (23, ubuntu-latest)
- GitHub Check: build (21, macOS-latest)
- GitHub Check: build (21, windows-latest)
- GitHub Check: build (21, ubuntu-latest)
- GitHub Check: build (17, macOS-latest)
- GitHub Check: build (17, windows-latest)
- GitHub Check: build (17, ubuntu-latest)
- GitHub Check: build (23, macOS-latest)
- GitHub Check: qodana
- GitHub Check: build (23, windows-latest)
- GitHub Check: Analyse
- GitHub Check: build (23, ubuntu-latest)
- GitHub Check: build (21, macOS-latest)
- GitHub Check: build (21, windows-latest)
- GitHub Check: build (21, ubuntu-latest)
- GitHub Check: build (17, macOS-latest)
- GitHub Check: build (17, windows-latest)
- GitHub Check: build (17, ubuntu-latest)
- GitHub Check: Benchmark
🔇 Additional comments (5)
src/main/java/com/github/_1c_syntax/bsl/languageserver/codelenses/AbstractRunTestsCodeLensSupplier.java (3)
24-48: Корректное добавление импортов и аннотаций!Добавленные импорты и аннотации класса хорошо соответствуют новой функциональности кэширования и конфигурации.
128-129: Улучшить метод isInside для корректной проверки вложенности
115-126:⚠️ Potential issueДобавить обработку исключений при работе с путями
Метод
Path.ofможет выброситьInvalidPathException, если пути содержат недопустимые символы. Необходимо добавить обработку исключений:return configuration.getCodeLensOptions().getTestRunnerAdapterOptions().getTestSources() .stream() - .map(testDir -> Path.of(configurationRootString, testDir)) + .map(testDir -> { + try { + return Path.of(configurationRootString, testDir); + } catch (InvalidPathException e) { + LOGGER.error("Некорректный путь к тестам: {}", testDir, e); + return null; + } + }) + .filter(Objects::nonNull) .map(path -> Absolute.path(path).toUri()) .collect(Collectors.toSet());Likely invalid or redundant comment.
src/main/java/com/github/_1c_syntax/bsl/languageserver/codelenses/testrunner/TestRunnerAdapter.java (2)
Line range hint
38-61: Корректная конфигурация кэширования!Добавленные аннотации кэширования и выбранное имя кэша хорошо соответствуют назначению класса.
68-79: Улучшить документацию no-op метода
src/test/resources/codelenses/AbstractRunTestCodeLensSupplier.os
Outdated
Show resolved
Hide resolved
src/test/resources/codelenses/tests/AbstractRunTestCodeLensSupplier.os
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/main/java/com/github/_1c_syntax/bsl/languageserver/configuration/databind/AnnotationsDeserializer.java (1)
43-60: LGTM! Рассмотрите добавление логирования.Реализация корректна и хорошо обрабатывает регистронезависимый ввод.
Рекомендуется добавить отладочное логирование для помощи в диагностике проблем десериализации:
@Override public Set<String> deserialize( JsonParser p, DeserializationContext context ) throws IOException { + log.debug("Десериализация аннотаций"); JsonNode annotations = p.getCodec().readTree(p); if (annotations == null) { + log.debug("Аннотации не указаны, используются значения по умолчанию"); return DEFAULT_ANNOTATIONS; } + log.debug("Создание регистронезависимого множества аннотаций"); Set<String> annotationsSet = new TreeSet<>(String.CASE_INSENSITIVE_ORDER); var objectMapper = (ObjectMapper) p.getCodec(); objectMapper.readerForUpdating(annotationsSet).readValue(p); return annotationsSet; }src/main/java/com/github/_1c_syntax/bsl/languageserver/configuration/codelens/TestRunnerAdapterOptions.java (1)
69-72: Добавьте информацию о значении по умолчанию в документацию.Рекомендуется дополнить JavaDoc информацией о значении параметра по умолчанию.
/** * Флаг, указывающий на необходимость получения списка тестов через исполняемый файл тестового фреймворка. + * <p> + * Значение по умолчанию: {@code false} */ private boolean getTestsByTestRunner;src/main/java/com/github/_1c_syntax/bsl/languageserver/codelenses/testrunner/TestRunnerAdapter.java (2)
75-79: Улучшите документацию no-op метода.Рекомендуется дополнить комментарий информацией о том, что метод намеренно пустой и используется только для обработки аннотаций.
- // No-op. Служит для сброса кеша при изменении конфигурации + // No-op метод. Аннотации @EventListener и @CacheEvict обеспечивают + // автоматический сброс кэша при изменении конфигурации
150-160: Рассмотрите оптимизацию производительности фильтрации.Текущая реализация использует вложенные потоки, что может повлиять на производительность при большом количестве методов или аннотаций.
Рекомендуется переработать логику для уменьшения вложенности:
private List<String> computeTestIdsByLanguageServer(DocumentContext documentContext) { var annotations = configuration.getCodeLensOptions().getTestRunnerAdapterOptions().getAnnotations(); return documentContext.getSymbolTree() .getMethods() .stream() - .filter(methodSymbol -> methodSymbol.getAnnotations().stream() - .map(Annotation::getName) - .anyMatch(annotations::contains)) + .filter(methodSymbol -> hasMatchingAnnotation(methodSymbol, annotations)) .map(MethodSymbol::getName) .toList(); } +private boolean hasMatchingAnnotation(MethodSymbol methodSymbol, Set<String> annotations) { + return methodSymbol.getAnnotations().stream() + .map(Annotation::getName) + .anyMatch(annotations::contains); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/main/resources/com/github/_1c_syntax/bsl/languageserver/configuration/schema.jsonis excluded by!**/*.json
📒 Files selected for processing (3)
src/main/java/com/github/_1c_syntax/bsl/languageserver/codelenses/testrunner/TestRunnerAdapter.java(4 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/configuration/codelens/TestRunnerAdapterOptions.java(4 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/configuration/databind/AnnotationsDeserializer.java(1 hunks)
🧰 Additional context used
📓 Learnings (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/configuration/codelens/TestRunnerAdapterOptions.java (2)
Learnt from: nixel2007
PR: 1c-syntax/bsl-language-server#3388
File: src/main/java/com/github/_1c_syntax/bsl/languageserver/configuration/codelens/TestRunnerAdapterOptions.java:46-46
Timestamp: 2025-01-19T21:34:39.797Z
Learning: In BSL Language Server configuration classes, immutable collections (Set.of, List.of) should be used for default values, while mutability is achieved through setters generated by @Data annotation.
Learnt from: nixel2007
PR: 1c-syntax/bsl-language-server#3388
File: src/main/java/com/github/_1c_syntax/bsl/languageserver/configuration/codelens/TestRunnerAdapterOptions.java:46-46
Timestamp: 2025-01-19T20:47:40.061Z
Learning: Configuration classes in the BSL Language Server project use mutable collections (HashMap, ArrayList) and @Data annotation from Lombok, allowing for modification of configuration properties after initialization.
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: build (23, windows-latest)
- GitHub Check: build (23, ubuntu-latest)
- GitHub Check: build (21, macOS-latest)
- GitHub Check: build (21, windows-latest)
- GitHub Check: build (21, ubuntu-latest)
- GitHub Check: build (17, macOS-latest)
- GitHub Check: build (17, windows-latest)
- GitHub Check: build (17, ubuntu-latest)
- GitHub Check: qodana
- GitHub Check: build (23, macOS-latest)
- GitHub Check: build (23, windows-latest)
- GitHub Check: build (23, ubuntu-latest)
- GitHub Check: build (21, macOS-latest)
- GitHub Check: build (21, windows-latest)
- GitHub Check: Analyse
- GitHub Check: build (21, ubuntu-latest)
- GitHub Check: build (17, macOS-latest)
- GitHub Check: build (17, windows-latest)
- GitHub Check: build (17, ubuntu-latest)
- GitHub Check: Benchmark
🔇 Additional comments (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/configuration/codelens/TestRunnerAdapterOptions.java (1)
46-60: LGTM! Корректное использование неизменяемых коллекций.Правильное использование
Set.of()иCollections.unmodifiableSet()для значений по умолчанию, что соответствует принятым практикам проекта.
|
Добавлен параметр конфигурации для расчета списка тестов силами BSL Language Server. По умолчанию ищутся все методы с аннотацией "Тест". Механизм включен по умолчанию.
Добавлен параметр конфигурации для задания списка каталогов, в которых располагаются тесты.
Подключение Spring Boot Cache
Описание
Связанные задачи
Closes
Чеклист
Общие
gradlew precommit)Для диагностик
Дополнительно