Skip to content
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

Open
wants to merge 13 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ dependencies {
// spring
api("org.springframework.boot:spring-boot-starter")
api("org.springframework.boot:spring-boot-starter-websocket")
api("org.springframework.boot:spring-boot-starter-cache")
api("info.picocli:picocli-spring-boot-starter:4.7.6")

// lsp4j core
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,18 +21,36 @@
*/
package com.github._1c_syntax.bsl.languageserver.codelenses;

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.FileType;
import com.github._1c_syntax.bsl.languageserver.events.LanguageServerInitializeRequestReceivedEvent;
import com.github._1c_syntax.utils.Absolute;
import jakarta.annotation.Nullable;
import lombok.RequiredArgsConstructor;
import org.eclipse.lsp4j.ClientInfo;
import org.eclipse.lsp4j.InitializeParams;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
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 java.net.URI;
import java.nio.file.Path;
import java.util.Optional;
import java.util.Set;
import java.util.stream.Collectors;

@RequiredArgsConstructor
@CacheConfig(cacheNames = "testSource")
public abstract class AbstractRunTestsCodeLensSupplier<T extends CodeLensData>
implements CodeLensSupplier<T> {

protected final LanguageServerConfiguration configuration;

private boolean clientIsSupported;

/**
Expand All @@ -43,6 +61,7 @@ public abstract class AbstractRunTestsCodeLensSupplier<T extends CodeLensData>
* @param event Событие
*/
@EventListener
@CacheEvict(allEntries = true)
public void handleEvent(LanguageServerInitializeRequestReceivedEvent event) {
var clientName = Optional.of(event)
.map(LanguageServerInitializeRequestReceivedEvent::getParams)
Expand All @@ -52,11 +71,40 @@ public void handleEvent(LanguageServerInitializeRequestReceivedEvent event) {
clientIsSupported = "Visual Studio Code".equals(clientName);
}

@EventListener
@CacheEvict(allEntries = true)
public void handleLanguageServerConfigurationChange(LanguageServerConfigurationChangedEvent event) {
}

/**
* {@inheritDoc}
*/
@Override
public boolean isApplicable(DocumentContext documentContext) {
return documentContext.getFileType() == FileType.OS && clientIsSupported;
var uri = documentContext.getUri();
var testSources = getSelf().getTestSources(documentContext.getServerContext().getConfigurationRoot());

return documentContext.getFileType() == FileType.OS
&& testSources.stream().anyMatch(testSource -> isInside(uri, testSource))
nixel2007 marked this conversation as resolved.
Show resolved Hide resolved
&& clientIsSupported;
}

protected abstract AbstractRunTestsCodeLensSupplier<T> getSelf();

@Cacheable
public Set<URI> getTestSources(@Nullable Path configurationRoot) {
var configurationRootString = Optional.ofNullable(configurationRoot)
.map(Path::toString)
.orElse("");

return configuration.getCodeLensOptions().getTestRunnerAdapterOptions().getTestSources()
.stream()
.map(testDir -> Path.of(configurationRootString, testDir))
.map(path -> Absolute.path(path).toUri())
.collect(Collectors.toSet());
}

private static boolean isInside(URI childURI, URI parentURI) {
return !parentURI.relativize(childURI).isAbsolute();
Comment on lines +128 to +129
Copy link
Contributor

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 для проверки вложенности путей.

Предлагаю изменить метод следующим образом:

-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.

}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -41,17 +43,30 @@
* Поставщик линзы для запуска всех тестов в текущем файле.
*/
@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;

@Autowired

Check notice on line 55 in src/main/java/com/github/_1c_syntax/bsl/languageserver/codelenses/RunAllTestsCodeLensSupplier.java

View workflow job for this annotation

GitHub Actions / Qodana for JVM

Non recommended 'field' injections

Field injection is not recommended

Check notice

Code scanning / QDJVM

Non recommended 'field' injections Note

Field injection is not recommended
@Lazy
@Getter
private RunAllTestsCodeLensSupplier self;

public RunAllTestsCodeLensSupplier(
LanguageServerConfiguration configuration,
TestRunnerAdapter testRunnerAdapter,
Resources resources
) {
super(configuration);
this.testRunnerAdapter = testRunnerAdapter;
this.resources = resources;
}

/**
* {@inheritDoc}
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,12 +28,14 @@
import com.github._1c_syntax.bsl.languageserver.context.symbol.MethodSymbol;
import com.github._1c_syntax.bsl.languageserver.utils.Resources;
import lombok.EqualsAndHashCode;
import lombok.RequiredArgsConstructor;
import lombok.Getter;
import lombok.ToString;
import lombok.Value;
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.beans.ConstructorProperties;
Expand All @@ -43,23 +45,35 @@
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.stream.Collectors;

/**
* Поставщик линз для запуска теста по конкретному тестовому методу.
*/
@Component
@RequiredArgsConstructor
@Slf4j
public class RunTestCodeLensSupplier
extends AbstractRunTestsCodeLensSupplier<RunTestCodeLensSupplier.RunTestCodeLensData> {

private static final String COMMAND_ID = "language-1c-bsl.languageServer.runTest";

private final TestRunnerAdapter testRunnerAdapter;
private final LanguageServerConfiguration configuration;
private final Resources resources;

@Autowired

Check notice on line 62 in src/main/java/com/github/_1c_syntax/bsl/languageserver/codelenses/RunTestCodeLensSupplier.java

View workflow job for this annotation

GitHub Actions / Qodana for JVM

Non recommended 'field' injections

Field injection is not recommended

Check notice

Code scanning / QDJVM

Non recommended 'field' injections Note

Field injection is not recommended
@Lazy
@Getter
private RunTestCodeLensSupplier self;

public RunTestCodeLensSupplier(
LanguageServerConfiguration configuration,
TestRunnerAdapter testRunnerAdapter,
Resources resources
) {
super(configuration);
this.testRunnerAdapter = testRunnerAdapter;
this.resources = resources;
}

/**
* {@inheritDoc}
*/
Expand All @@ -77,7 +91,7 @@
.map(symbolTree::getMethodSymbol)
.flatMap(Optional::stream)
.map(methodSymbol -> toCodeLens(methodSymbol, documentContext))
.collect(Collectors.toList());
.toList();
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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;

/**
* Расчетчик списка тестов в документе.
Expand All @@ -55,26 +58,36 @@
@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;

@EventListener
@CacheEvict(allEntries = true)
public void handleEvent(LanguageServerConfigurationChangedEvent event) {
}
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();

return CACHE.computeIfAbsent(cacheKey, pair -> computeTestIds(documentContext));
if (options.isGetTestsByTestRunner()) {
return computeTestIdsByTestRunner(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();
Expand Down Expand Up @@ -123,7 +136,17 @@ private List<String> computeTestIds(DocumentContext documentContext) {
.map(getTestsRegex::matcher)
.filter(Matcher::matches)
.map(matcher -> matcher.group(1))
.collect(Collectors.toList());
.toList();
}

private static List<String> computeTestIdsByLanguageServer(DocumentContext documentContext) {
return documentContext.getSymbolTree()
.getMethods()
.stream()
.filter(methodSymbol -> methodSymbol.getAnnotations().stream()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Не нравится цикл в цикле. Не будет проблемой?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Количество аннотаций на метод в тестовых сорцах достаточно маленькое, в среднем чуть больше одной. Так что не думаю, что эти будет большой проблемой.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

переделано на Set.contains

.map(Annotation::getName)
.anyMatch("Тест"::equalsIgnoreCase))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Строковую константу по хорошему лучше явно вынести. Она точно никогда-никогда не может быть двуязычной?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Текущие тестовые фреймворки не поддерживают других аннотаций. Думаешь в конфиг унести и сразу добавить оба языка?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Текущие тестовые фреймворки не поддерживают других аннотаций. Думаешь в конфиг унести и сразу добавить оба языка?

Угу. Текущий хардкод выглядит немного неаккуратно

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

переделал на конфиг

.map(MethodSymbol::getName)
.toList();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,9 @@
import lombok.NoArgsConstructor;
import org.apache.commons.lang3.SystemUtils;

import java.util.List;
import java.util.Set;

/**
* Параметры запускателя тестового фреймворка.
*/
Expand All @@ -37,6 +40,11 @@
@JsonIgnoreProperties(ignoreUnknown = true)
public class TestRunnerAdapterOptions {

/**
* Каталоги с исходными файлами тестов.
*/
private Set<String> testSources = Set.of("tests");
nixel2007 marked this conversation as resolved.
Show resolved Hide resolved

/**
* Имя исполняемого файла тестового фреймворка (linux и macOS).
*/
Expand All @@ -45,6 +53,10 @@ public class TestRunnerAdapterOptions {
* Имя исполняемого файла тестового фреймворка (windows).
*/
private String executableWin = "1testrunner.bat";
/**
* Флаг, указывающий на необходимость получения списка тестов через исполняемый файл тестового фреймворка.
*/
private boolean getTestsByTestRunner;
/**
* Аргументы для получения списка тестов.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
import com.github._1c_syntax.utils.Absolute;
import com.github._1c_syntax.utils.Lazy;
import edu.umd.cs.findbugs.annotations.Nullable;
import lombok.Getter;
import lombok.RequiredArgsConstructor;
import lombok.Setter;
import lombok.extern.slf4j.Slf4j;
Expand Down Expand Up @@ -67,6 +68,7 @@ public class ServerContext {
private final Lazy<CF> configurationMetadata = new Lazy<>(this::computeConfigurationMetadata);
@Nullable
@Setter
@Getter
private Path configurationRoot;
private final Map<URI, String> mdoRefs = Collections.synchronizedMap(new HashMap<>());
private final Map<String, Map<ModuleType, DocumentContext>> documentsByMDORef
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
/*
* This file is a part of BSL Language Server.
*
* Copyright (c) 2018-2025
* Alexey Sosnoviy <labotamy@gmail.com>, Nikita Fedkin <nixel2007@gmail.com> and contributors
*
* SPDX-License-Identifier: LGPL-3.0-or-later
*
* BSL Language Server is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
* License as published by the Free Software Foundation; either
* version 3.0 of the License, or (at your option) any later version.
*
* BSL Language Server is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
* Lesser General Public License for more details.
*
* You should have received a copy of the GNU Lesser General Public
* License along with BSL Language Server.
*/
package com.github._1c_syntax.bsl.languageserver.infrastructure;

import org.springframework.cache.annotation.EnableCaching;
import org.springframework.context.annotation.Configuration;

@Configuration
@EnableCaching
public class CacheConfiguration {
}
Loading