-
Notifications
You must be signed in to change notification settings - Fork 109
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Оптимизация механизма запуска тестов. #3388
base: develop
Are you sure you want to change the base?
Changes from all commits
5bfc959
5df4864
db12e51
c05d94c
e1d0cb6
0c51300
393ae5f
11377b2
30ff5fe
35a9dd5
45bd658
c0cdba9
c515628
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,10 +26,12 @@ | |
import com.github._1c_syntax.bsl.languageserver.context.DocumentContext; | ||
import com.github._1c_syntax.bsl.languageserver.context.symbol.MethodSymbol; | ||
import com.github._1c_syntax.bsl.languageserver.utils.Resources; | ||
import lombok.RequiredArgsConstructor; | ||
import lombok.Getter; | ||
import lombok.extern.slf4j.Slf4j; | ||
import org.eclipse.lsp4j.CodeLens; | ||
import org.eclipse.lsp4j.Command; | ||
import org.springframework.beans.factory.annotation.Autowired; | ||
import org.springframework.context.annotation.Lazy; | ||
import org.springframework.stereotype.Component; | ||
|
||
import java.nio.file.Paths; | ||
|
@@ -41,17 +43,31 @@ | |
* Поставщик линзы для запуска всех тестов в текущем файле. | ||
*/ | ||
@Component | ||
@RequiredArgsConstructor | ||
@Slf4j | ||
public class RunAllTestsCodeLensSupplier | ||
nixel2007 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
extends AbstractRunTestsCodeLensSupplier<DefaultCodeLensData> { | ||
|
||
private static final String COMMAND_ID = "language-1c-bsl.languageServer.runAllTests"; | ||
|
||
private final TestRunnerAdapter testRunnerAdapter; | ||
private final LanguageServerConfiguration configuration; | ||
private final Resources resources; | ||
|
||
// Self-injection для работы кэша в базовом классе. | ||
@Autowired | ||
Check notice Code scanning / QDJVM Non recommended 'field' injections Note
Field injection is not recommended
|
||
@Lazy | ||
@Getter | ||
private RunAllTestsCodeLensSupplier self; | ||
Comment on lines
+55
to
+59
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. self-injection нужен для вызова @cachable методов изнутри класса There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. нет, это был комментарий для тебя @coderabbitai There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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
|
||
|
||
public RunAllTestsCodeLensSupplier( | ||
LanguageServerConfiguration configuration, | ||
TestRunnerAdapter testRunnerAdapter, | ||
Resources resources | ||
) { | ||
super(configuration); | ||
this.testRunnerAdapter = testRunnerAdapter; | ||
this.resources = resources; | ||
} | ||
|
||
/** | ||
* {@inheritDoc} | ||
*/ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,7 +22,10 @@ | |
package com.github._1c_syntax.bsl.languageserver.codelenses.testrunner; | ||
|
||
import com.github._1c_syntax.bsl.languageserver.configuration.LanguageServerConfiguration; | ||
import com.github._1c_syntax.bsl.languageserver.configuration.events.LanguageServerConfigurationChangedEvent; | ||
import com.github._1c_syntax.bsl.languageserver.context.DocumentContext; | ||
import com.github._1c_syntax.bsl.languageserver.context.symbol.MethodSymbol; | ||
import com.github._1c_syntax.bsl.languageserver.context.symbol.annotations.Annotation; | ||
import lombok.RequiredArgsConstructor; | ||
import lombok.extern.slf4j.Slf4j; | ||
import org.apache.commons.exec.CommandLine; | ||
|
@@ -32,7 +35,10 @@ | |
import org.apache.commons.exec.PumpStreamHandler; | ||
import org.apache.commons.io.output.ByteArrayOutputStream; | ||
import org.apache.commons.lang3.SystemUtils; | ||
import org.apache.commons.lang3.tuple.Pair; | ||
import org.springframework.cache.annotation.CacheConfig; | ||
import org.springframework.cache.annotation.CacheEvict; | ||
import org.springframework.cache.annotation.Cacheable; | ||
import org.springframework.context.event.EventListener; | ||
import org.springframework.stereotype.Component; | ||
|
||
import java.io.IOException; | ||
|
@@ -42,11 +48,8 @@ | |
import java.util.Arrays; | ||
import java.util.Collections; | ||
import java.util.List; | ||
import java.util.Map; | ||
import java.util.WeakHashMap; | ||
import java.util.regex.Matcher; | ||
import java.util.regex.Pattern; | ||
import java.util.stream.Collectors; | ||
|
||
/** | ||
* Расчетчик списка тестов в документе. | ||
|
@@ -55,26 +58,44 @@ | |
@Component | ||
@RequiredArgsConstructor | ||
@Slf4j | ||
@CacheConfig(cacheNames = "testIds") | ||
public class TestRunnerAdapter { | ||
|
||
private static final Pattern NEW_LINE_PATTERN = Pattern.compile("\r?\n"); | ||
private static final Map<Pair<DocumentContext, Integer>, List<String>> CACHE = new WeakHashMap<>(); | ||
|
||
private final LanguageServerConfiguration configuration; | ||
|
||
/** | ||
* Обработчик события {@link LanguageServerConfigurationChangedEvent}. | ||
* <p> | ||
* Очищает кэш при изменении конфигурации. | ||
* | ||
* @param event Событие | ||
*/ | ||
@EventListener | ||
@CacheEvict(allEntries = true) | ||
public void handleEvent(LanguageServerConfigurationChangedEvent event) { | ||
// No-op. Служит для сброса кеша при изменении конфигурации | ||
} | ||
nixel2007 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
/** | ||
* Получить идентификаторы тестов, содержащихся в файле. | ||
* | ||
* @param documentContext Контекст документа с тестами. | ||
* @return Список идентификаторов тестов. | ||
*/ | ||
@Cacheable | ||
public List<String> getTestIds(DocumentContext documentContext) { | ||
var cacheKey = Pair.of(documentContext, documentContext.getVersion()); | ||
var options = configuration.getCodeLensOptions().getTestRunnerAdapterOptions(); | ||
|
||
if (options.isGetTestsByTestRunner()) { | ||
return computeTestIdsByTestRunner(documentContext); | ||
} | ||
|
||
return CACHE.computeIfAbsent(cacheKey, pair -> computeTestIds(documentContext)); | ||
return computeTestIdsByLanguageServer(documentContext); | ||
} | ||
|
||
private List<String> computeTestIds(DocumentContext documentContext) { | ||
private List<String> computeTestIdsByTestRunner(DocumentContext documentContext) { | ||
var options = configuration.getCodeLensOptions().getTestRunnerAdapterOptions(); | ||
|
||
var executable = SystemUtils.IS_OS_WINDOWS ? options.getExecutableWin() : options.getExecutable(); | ||
|
@@ -123,7 +144,18 @@ private List<String> computeTestIds(DocumentContext documentContext) { | |
.map(getTestsRegex::matcher) | ||
.filter(Matcher::matches) | ||
.map(matcher -> matcher.group(1)) | ||
.collect(Collectors.toList()); | ||
.toList(); | ||
} | ||
|
||
private List<String> computeTestIdsByLanguageServer(DocumentContext documentContext) { | ||
var annotations = configuration.getCodeLensOptions().getTestRunnerAdapterOptions().getAnnotations(); | ||
return documentContext.getSymbolTree() | ||
.getMethods() | ||
.stream() | ||
.filter(methodSymbol -> methodSymbol.getAnnotations().stream() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Не нравится цикл в цикле. Не будет проблемой? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Количество аннотаций на метод в тестовых сорцах достаточно маленькое, в среднем чуть больше одной. Так что не думаю, что эти будет большой проблемой. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. переделано на Set.contains |
||
.map(Annotation::getName) | ||
.anyMatch(annotations::contains)) | ||
.map(MethodSymbol::getName) | ||
.toList(); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Улучшение метода
isInside
для корректной проверки вложенностиИспользование
URI.relativize()
может давать неверные результаты при работе с URI, содержащими разные схемы или хосты. Рекомендуется использоватьPaths
для проверки вложенности путей.Предлагаю изменить метод следующим образом: