Исправление ошибки работы, если не используется БСП#205
Conversation
Detect BSP configurations by searching config.srcDir for ОбновлениеИнформационнойБазыБСП.xml or ОбновлениеИнформационнойБазыБСП.mdo. For BSP configurations, keep passing --exitCodePath and read build/migration-exit-status.log. For non-BSP configurations, do not pass --exitCodePath and rely on the vrunner process exit status instead. Tests cover both BSP and non-BSP migration command behavior. Issue: firstBitMarksistskaya#189
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughInitInfoBase теперь детектирует BSP через BspDetector и условно добавляет --exitCodePath; VRunner стал искать NoSuchFileException в цепочке причин при чтении файла статуса; добавлены/обновлены юнит‑тесты и небольшая правка plugins.groovy. ChangesПоддержка миграции с конфигурацией БСП и обработка исключений
Sequence DiagramsequenceDiagram
participant InitInfoBase
participant BspDetector
participant MigrationProcess
participant VRunner
InitInfoBase->>BspDetector: isBspConfiguration(config)
alt BSP
InitInfoBase->>MigrationProcess: exec(... --exitCodePath build/migration-exit-status.log)
MigrationProcess-->>InitInfoBase: exitStatus
InitInfoBase->>VRunner: readExitStatusFromFile("build/migration-exit-status.log", fallback)
VRunner-->>InitInfoBase: effectiveExitStatus
else non-BSP
InitInfoBase->>MigrationProcess: exec(...)
MigrationProcess-->>InitInfoBase: exitStatus (used directly)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/ru/pulsar/jenkins/library/steps/InitInfoBase.groovy`:
- Around line 133-146: In isBspConfiguration, trim config.srcDir before using it
to build sourceDir so trailing/leading whitespace doesn't cause findFiles to
miss matches; change the construction of sourceDir to use a trimmed value (e.g.,
assign String sourceDir = config.srcDir?.trim() and then replace('\\','/') on
that trimmed string) and keep the existing empty/null check using the trimmed
value before calling steps.findFiles for ОбновлениеИнформационнойБазыБСП.xml and
.mdo.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c327104e-7da9-4704-98ea-dc054ea987fe
⛔ Files ignored due to path filters (1)
test/unit/resources/initInfoBaseRunMigration.jsonis excluded by!**/*.json
📒 Files selected for processing (4)
src/ru/pulsar/jenkins/library/steps/InitInfoBase.groovysrc/ru/pulsar/jenkins/library/utils/VRunner.groovytest/unit/groovy/ru/pulsar/jenkins/library/steps/InitInfoBaseTest.javatest/unit/groovy/ru/pulsar/jenkins/library/utils/VRunnerTest.java
| boolean bspConfiguration = isBspConfiguration(steps) | ||
| if (bspConfiguration) { | ||
| command += " --exitCodePath \"${migrationStatusFile}\"" | ||
| } else { | ||
| Logger.println("Конфигурация не на БСП, запуск миграции ИБ без контроля ${migrationStatusFile}") | ||
| } | ||
| // Запуск миграции | ||
| steps.catchError { | ||
| Integer exitStatus = VRunner.exec(command, true) | ||
| exitStatuses.put(command, VRunner.readExitStatusFromFile(migrationStatusFile, exitStatus)) | ||
| if (bspConfiguration) { | ||
| exitStatuses.put(command, VRunner.readExitStatusFromFile(migrationStatusFile, exitStatus)) | ||
| } else { | ||
| exitStatuses.put(command, exitStatus) | ||
| } |
There was a problem hiding this comment.
Здесь два раздельных if (bspConfiguration) (строки 71 и 79) на одну и ту же булевую - логика построения команды и обработки её результата разнесена. Если кто-то завтра поменяет правило, легко забыть второе место.
Плюс сама идея «угадывать БСП эвристикой ради того, чтобы не передавать --exitCodePath» дублирует то, что уже делает VRunner.readExitStatusFromFile(file, fallback) - он умеет вернуть fallback, если файла нет. Если довести readExitStatusFromFile до устойчивого детекта NoSuchFileException (см. комментарий к VRunner.groovy), то весь этот блок можно вернуть к виду до PR - всегда передавать --exitCodePath, а fallback на exit-код процесса делать в одном месте, в VRunner.
Если детект БСП всё-таки нужен - собрать всё в одно решение и одну ветку:
boolean useExitCodeFile = BspDetector.isBspConfiguration(config)
if (useExitCodeFile) {
command += " --exitCodePath \"${migrationStatusFile}\""
} else {
Logger.println("Конфигурация не на БСП - миграция без файла статуса ${migrationStatusFile}")
}
steps.catchError {
Integer exitStatus = VRunner.exec(command, true)
Integer effective = useExitCodeFile
? VRunner.readExitStatusFromFile(migrationStatusFile, exitStatus)
: exitStatus
exitStatuses.put(command, effective)
}There was a problem hiding this comment.
Исправили: оставили единый флаг useExitCodeFile. Через него теперь и добавляется --exitCodePath, и выбирается итоговый effectiveExitStatus; дублирующий exitStatuses.put(...) убран.
| private boolean isBspConfiguration(IStepExecutor steps) { | ||
| if (config.srcDir == null || config.srcDir.trim().isEmpty()) { | ||
| Logger.println("Не указан srcDir, конфигурация считается не на БСП") | ||
| return false | ||
| } | ||
|
|
||
| String sourceDir = config.srcDir.replace('\\', '/') | ||
| FileWrapper[] xmlFiles = steps.findFiles("${sourceDir}/**/ОбновлениеИнформационнойБазыБСП.xml") ?: new FileWrapper[0] | ||
| FileWrapper[] mdoFiles = steps.findFiles("${sourceDir}/**/ОбновлениеИнформационнойБазыБСП.mdo") ?: new FileWrapper[0] | ||
| boolean bspConfiguration = xmlFiles.length > 0 || mdoFiles.length > 0 | ||
|
|
||
| Logger.println("Определение БСП по исходникам ${sourceDir}: ${bspConfiguration ? 'БСП' : 'не БСП'}") | ||
| return bspConfiguration |
There was a problem hiding this comment.
Несколько проблем:
-
Метод не на своём месте. «БСП или не БСП» - свойство всего проекта, а не одного шага инициализации. Эту же логику захотят переиспользовать в вдругих шагах. Стоит вынести в ru.pulsar.jenkins.library.utils.BspDetector (или сделать ленивым геттером на JobConfiguration).
-
findFiles("${sourceDir}/**/...") дорогой. Рекурсивный обход всего srcDir на каждом запуске миграции ради одного CLI-флага. Достаточно steps.fileExists(точныйПуть).
-
Игнорируется config.sourceFormat. Именно об этом намекал @nixel2007, кинув ссылку на SonarScanner.groovy#L113 - там уже есть формат-aware (EDT vs Designer) построение пути к общему модулю БСП:
if (config.sourceFormat == SourceFormat.EDT) {
rootFile = "$config.srcDir/src/CommonModules/$nameOfModule/Module.bsl"
} else {
rootFile = "$config.srcDir/CommonModules/$nameOfModule/Ext/Module.bsl"
}Логично переиспользовать этот же подход.
-
Магические строки. "ОбновлениеИнформационнойБазыБСП" зашит в groovy дважды без константы. У конфигураций на БСП имя бывает разное (см. описание sonarQubeOptions.infoBaseUpdateModuleName — ОбновлениеИнформационнойБазыXXX). Имя стоит вынести в константу и/или брать из config.sonarQubeOptions.infoBaseUpdateModuleName, если оно задано.
-
config.srcDir не trim-ится перед replace('\', '/') (строка 139). Если в конфиге пробелы по краям - findFiles тихо вернёт пусто и конфигурация молча пометится как не-БСП. CodeRabbit об этом уже писал.
There was a problem hiding this comment.
Исправили: детект БСП вынесен в BspDetector, рекурсивный findFiles заменен на точечный fileExists. Путь строится с учетом sourceFormat EDT/Designer, имя модуля берется из sonarQubeOptions.infoBaseUpdateModuleName либо из константы по умолчанию, srcDir trim-ится.
| private static boolean isNoSuchFileException(Throwable e) { | ||
| Throwable current = e | ||
| while (current != null) { | ||
| if (current instanceof NoSuchFileException || current.message?.contains(NoSuchFileException.name)) { | ||
| return true | ||
| } | ||
| current = current.cause | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
if (current instanceof NoSuchFileException || current.message?.contains(NoSuchFileException.name)) {
Поиск "java.nio.file.NoSuchFileException" подстрокой в тексте сообщения - крайне хрупко. Любое изменение формата сообщения от Jenkins/JVM/плагина - детект тихо отвалится, и не-БСП сборки снова станут красными. И тест фактически прибит к этой же подстроке.
Если действительно встречаются обёрнутые исключения - лучше:
Пройтись по цепочке cause и проверить через isInstance (общий хелпер):
@NonCPS
private static <T extends Throwable> T findCause(Throwable e, Class<T> type) {
Throwable current = e
while (current != null) {
if (type.isInstance(current)) return type.cast(current)
if (current.cause === current) break
current = current.cause
}
return null
}Если instanceof не работает из-за CPS/classloader-ной подмены типа - на крайний случай матчить по class.simpleName == 'NoSuchFileException', но НЕ по подстроке в .message.
There was a problem hiding this comment.
Исправили: убрали поиск NoSuchFileException по подстроке в message. Добавлен findCause(...), который проходит цепочку cause и проверяет тип через Class.isInstance.
| Logger.println("В файле со статусом возврата ${path} записано не числовое значение: ${e.message}") | ||
| return 1 | ||
| } catch (Exception e) { | ||
| if (isNoSuchFileException(e)) { | ||
| Logger.println("Файл со статусом возврата ${path} не найден: ${e.message}. Будет использован переданный статус ${valueIfNoSuchFile}") | ||
| return valueIfNoSuchFile | ||
| } | ||
| Logger.println("При чтении файла со статусом возврата ${path} возникла ошибка: ${e.message}") | ||
| return 1 |
There was a problem hiding this comment.
Сейчас обработка NoSuchFileException живёт в двух разных catch-блоках (71 и 78–81) - разъедется при правках. Объединил бы в один catch (Exception e) с findCause(e, NoSuchFileException):
} catch (NumberFormatException e) {
Logger.println("В файле со статусом возврата ${path} записано не числовое значение: ${e.message}")
return 1
} catch (Exception e) {
if (findCause(e, NoSuchFileException) != null) {
Logger.println("Файл со статусом возврата ${path} не найден: ${e.message}. Будет использован переданный статус ${valueIfNoSuchFile}")
return valueIfNoSuchFile
}
Logger.println("При чтении файла со статусом возврата ${path} возникла ошибка: ${e.message}")
return 1
}There was a problem hiding this comment.
Исправили: обработку отсутствующего файла свели в один catch (Exception e) через findCause(e, NoSuchFileException). Отдельный catch NoSuchFileException больше не нужен.
| @Test | ||
| void readExitStatusFromFile_jenkins_wrapped_no_such_file_uses_provided_fallback() { | ||
|
|
||
| // given | ||
| String resource = "build/migration-exit-status.log"; | ||
| IStepExecutor steps = TestUtils.getMockedStepExecutor(); | ||
| TestUtils.setupMockedContext(steps); | ||
| doThrow(new RuntimeException("java.nio.file.NoSuchFileException: " + resource)) | ||
| .when(steps).readFile(resource); | ||
|
|
||
| // when | ||
| Integer exitStatus = VRunner.readExitStatusFromFile(resource, 0); | ||
|
|
||
| // then | ||
| assertThat(exitStatus).isEqualTo(0); | ||
| } | ||
| } |
There was a problem hiding this comment.
Тест намертво зацементирован к реализации, которую как раз стоит переписать:
doThrow(new RuntimeException("java.nio.file.NoSuchFileException: " + resource))
.when(steps).readFile(resource);подаётся RuntimeException БЕЗ настоящей причины, и проверяется, что код найдёт имя класса в .message. То есть тест валидирует именно эвристику.
Лучше сделать честную цепочку причин - это ближе к реальности и переживёт нормальный рефакторинг:
doThrow(new RuntimeException("read failed", new NoSuchFileException(resource)))
.when(steps).readFile(resource);There was a problem hiding this comment.
Исправили: тест больше не проверяет эвристику по тексту сообщения. Теперь мокается честная цепочка причин: RuntimeException("read failed", new NoSuchFileException(resource)).
| vrunner.when(() -> VRunner.readExitStatusFromFile("build/migration-exit-status.log", 0)).thenReturn(0); | ||
|
|
||
| // when | ||
| new InitInfoBase(jobConfiguration).run(); | ||
|
|
||
| // then | ||
| assertThat(commands.get(0)).contains("--exitCodePath \"build/migration-exit-status.log\""); | ||
| vrunner.verify(() -> VRunner.readExitStatusFromFile("build/migration-exit-status.log", 0)); | ||
| verify(steps, never()).error(anyString()); | ||
| } | ||
| } | ||
|
|
||
| @Test | ||
| void runDoesNotUseMigrationStatusFileForNonBspConfiguration() throws IOException { | ||
|
|
||
| // given | ||
| String config = IOUtils.resourceToString( | ||
| "initInfoBaseRunMigration.json", | ||
| StandardCharsets.UTF_8, | ||
| this.getClass().getClassLoader() | ||
| ); | ||
| JobConfiguration jobConfiguration = ConfigurationReader.create(config); | ||
| when(steps.findFiles(anyString())).thenReturn(new FileWrapper[0]); | ||
| List<String> commands = new ArrayList<>(); | ||
|
|
||
| try (MockedStatic<VRunner> vrunner = Mockito.mockStatic(VRunner.class)) { | ||
| vrunner.when(VRunner::getVRunnerPath).thenReturn("vrunner"); | ||
| vrunner.when(() -> VRunner.exec(anyString(), eq(true))).thenAnswer(invocation -> { | ||
| commands.add(invocation.getArgument(0)); | ||
| return 0; | ||
| }); | ||
|
|
||
| // when | ||
| new InitInfoBase(jobConfiguration).run(); | ||
|
|
||
| // then | ||
| assertThat(commands.get(0)).doesNotContain("--exitCodePath"); | ||
| vrunner.verify(() -> VRunner.readExitStatusFromFile(anyString(), any(Integer.class)), never()); | ||
| verify(steps, never()).error(anyString()); |
There was a problem hiding this comment.
Если согласимся убрать эвристику isBspConfiguration в пользу надёжного fallback в VRunner.readExitStatusFromFile (см. комментарий к InitInfoBase.groovy#L70-84), этот тест становится не нужен - поведение для non-BSP покрывается тестом из VRunnerTest. А тест runUsesMigrationStatusFileForBspConfiguration можно вернуть к исходному runUsesCommandExitStatusWhenMigrationStatusFileIsMissing - он проверяет тот же контракт, но без мокания findFiles.
Если всё-таки оставлять детект - мокание findFiles стоит делать через утилиту в BspDetector, а не через **/... glob, и тест надо подправить под новый путь.
There was a problem hiding this comment.
Исправили: тесты InitInfoBase подстроены под новый BspDetector и fileExists. Для non-BSP больше не мокается рекурсивный findFiles, отдельно проверяем, что --exitCodePath не добавляется и readExitStatusFromFile не вызывается.
isBspConfiguration сохранен и остальное все учтено/поправлено. firstBitMarksistskaya#205
isBspConfiguration сохранен и остальное все учтено/поправлено. firstBitMarksistskaya#205
|
@nixel2007 @johnnyshut все замечания поправил. |
| static boolean isBspConfiguration(JobConfiguration config, IStepExecutor steps) { | ||
| String sourceDir = config.srcDir?.trim()?.replace('\\', '/') | ||
| if (!sourceDir) { | ||
| Logger.println("Не указан srcDir, конфигурация считается не на БСП") | ||
| return false | ||
| } | ||
|
|
||
| String moduleName = getInfoBaseUpdateModuleName(config) | ||
| String modulePath = getCommonModulePath(sourceDir, config.sourceFormat, moduleName) | ||
| boolean bspConfiguration = steps.fileExists(modulePath) | ||
|
|
||
| Logger.println("Определение БСП по общему модулю ${modulePath}: ${bspConfiguration ? 'БСП' : 'не БСП'}") | ||
| return bspConfiguration | ||
| } |
There was a problem hiding this comment.
Несимметричный API: steps принимается параметром и используется в fileExists (строка 20), а Logger.println (строки 14, 22) внутри тянет IStepExecutor из ContextRegistry. В тестах BspDetectorTest нет setupMockedContext(steps), поэтому fileExists уходит на переданный мок, а echo - либо в null (NPE), либо в чужой мок, оставшийся в ContextRegistry от другого теста. Это и причина того, что тесты сейчас работают «случайно» (зависят от порядка прогона).
Выбрать одно из двух:
(а) убрать IStepExecutor steps из сигнатуры и брать его из ContextRegistry, как делают остальные шаги - единообразно с проектом:
static boolean isBspConfiguration(JobConfiguration config) {
IStepExecutor steps = ContextRegistry.getContext().getStepExecutor()
...
}Тогда в InitInfoBase (строка 71) вызов становится BspDetector.isBspConfiguration(config), а тесты BspDetectorTest обязаны вызывать TestUtils.setupMockedContext(steps) - что и так требуется для Logger.
(б) оставить steps параметром, но логировать через него же - steps.echo(...) вместо Logger.println(...). Тогда тесты не зависят от регистра вообще.
Вариант (а) ближе к стилю проекта (см. Logger, VRunner.getVRunnerPath, InitInfoBase.run - везде steps берётся из ContextRegistry).
There was a problem hiding this comment.
Исправил в 7d7dbac: выбрал вариант (а). BspDetector.isBspConfiguration больше не принимает steps параметром, а берёт IStepExecutor из ContextRegistry, как Logger, VRunner и остальные шаги/утилиты. Вызов в InitInfoBase обновил на BspDetector.isBspConfiguration(config).
| class BspDetectorTest { | ||
|
|
||
| @Test | ||
| void isBspConfiguration_checks_designer_module_path() { | ||
|
|
||
| // given | ||
| IStepExecutor steps = TestUtils.getMockedStepExecutor(); | ||
| JobConfiguration config = createConfig("src/cf", SourceFormat.DESIGNER, ""); | ||
| String expectedPath = "src/cf/CommonModules/" | ||
| + BspDetector.DEFAULT_INFO_BASE_UPDATE_MODULE_NAME | ||
| + "/Ext/Module.bsl"; | ||
| when(steps.fileExists(expectedPath)).thenReturn(true); | ||
|
|
||
| // when | ||
| boolean result = BspDetector.isBspConfiguration(config, steps); | ||
|
|
||
| // then | ||
| assertThat(result).isTrue(); | ||
| verify(steps).fileExists(expectedPath); | ||
| } | ||
|
|
||
| @Test | ||
| void isBspConfiguration_checks_edt_module_path() { | ||
|
|
||
| // given | ||
| IStepExecutor steps = TestUtils.getMockedStepExecutor(); | ||
| JobConfiguration config = createConfig("src/cf", SourceFormat.EDT, ""); | ||
| String expectedPath = "src/cf/src/CommonModules/" | ||
| + BspDetector.DEFAULT_INFO_BASE_UPDATE_MODULE_NAME | ||
| + "/Module.bsl"; | ||
| when(steps.fileExists(expectedPath)).thenReturn(true); | ||
|
|
||
| // when | ||
| boolean result = BspDetector.isBspConfiguration(config, steps); | ||
|
|
||
| // then | ||
| assertThat(result).isTrue(); | ||
| verify(steps).fileExists(expectedPath); | ||
| } | ||
|
|
||
| @Test | ||
| void isBspConfiguration_uses_custom_module_name_and_trims_src_dir() { | ||
|
|
||
| // given | ||
| IStepExecutor steps = TestUtils.getMockedStepExecutor(); | ||
| JobConfiguration config = createConfig(" src\\cf ", SourceFormat.DESIGNER, "InfoBaseUpdateModule"); | ||
| String expectedPath = "src/cf/CommonModules/InfoBaseUpdateModule/Ext/Module.bsl"; | ||
| when(steps.fileExists(expectedPath)).thenReturn(true); | ||
|
|
||
| // when | ||
| boolean result = BspDetector.isBspConfiguration(config, steps); | ||
|
|
||
| // then | ||
| assertThat(result).isTrue(); | ||
| verify(steps).fileExists(expectedPath); | ||
| } | ||
|
|
||
| @Test | ||
| void isBspConfiguration_returns_false_for_blank_src_dir() { | ||
|
|
||
| // given | ||
| IStepExecutor steps = TestUtils.getMockedStepExecutor(); | ||
| JobConfiguration config = createConfig(" ", SourceFormat.DESIGNER, ""); | ||
|
|
||
| // when | ||
| boolean result = BspDetector.isBspConfiguration(config, steps); | ||
|
|
||
| // then | ||
| assertThat(result).isFalse(); | ||
| } | ||
|
|
||
| private static JobConfiguration createConfig(String srcDir, SourceFormat sourceFormat, String moduleName) { | ||
| SonarQubeOptions sonarQubeOptions = new SonarQubeOptions(); | ||
| sonarQubeOptions.setInfoBaseUpdateModuleName(moduleName); | ||
|
|
||
| JobConfiguration config = new JobConfiguration(); | ||
| config.setSrcDir(srcDir); | ||
| config.setSourceFormat(sourceFormat); | ||
| config.setSonarQubeOptions(sonarQubeOptions); | ||
| return config; | ||
| } | ||
| } |
There was a problem hiding this comment.
В классе нет @beforeeach, нигде не вызывается TestUtils.setupMockedContext(steps). Между тем BspDetector.isBspConfiguration(...) дёргает Logger.println(...), который под капотом идёт в ContextRegistry.getContext().getStepExecutor().echo(...):
static void println(String message) {
IStepExecutor steps = ContextRegistry.getContext().getStepExecutor()
steps.echo(message)
}Если ContextRegistry пуст (а в начале этого тест-класса он именно пуст) - Logger.println упадёт NPE. Если другой тест-класс уже зарегистрировал контекст до этого - тест случайно пройдёт, но echo уйдёт не на тот мок, что в verify(steps).fileExists(...). Поведение зависит от порядка прогона тестов и любая параллелизация это сломает.
Минимально:
@BeforeEach
void setUp() {
// регистрируем какой-то контекст по умолчанию,
// в каждом тесте перерегистрируем под нужный mock через setupMockedContext(steps)
TestUtils.setupMockedContext();
}и в каждом тесте:
IStepExecutor steps = TestUtils.getMockedStepExecutor();
TestUtils.setupMockedContext(steps);Тогда Logger.println будет писать ровно в тот мок, что проверяется verify(steps).
Ещё лучше - устранить корень в BspDetector (см. комментарий к BspDetector.groovy#L11-24): если BspDetector сам берёт steps из ContextRegistry, то тесту достаточно одного setupMockedContext(steps) и параметр steps из сигнатуры уходит - становится симметрично с остальным кодом.
There was a problem hiding this comment.
Исправил в 7d7dbac: добавил @BeforeEach в BspDetectorTest, создаю steps = TestUtils.getMockedStepExecutor() и регистрирую именно этот мок через TestUtils.setupMockedContext(steps). Теперь Logger.println и fileExists работают с одним и тем же мок-экземпляром, без зависимости от порядка запуска тестов.
| @Test | ||
| void runDoesNotUseMigrationStatusFileForNonBspConfiguration() throws IOException { | ||
|
|
||
| // given | ||
| String config = IOUtils.resourceToString( | ||
| "initInfoBaseRunMigration.json", | ||
| StandardCharsets.UTF_8, | ||
| this.getClass().getClassLoader() | ||
| ); | ||
| JobConfiguration jobConfiguration = ConfigurationReader.create(config); | ||
| List<String> commands = new ArrayList<>(); | ||
|
|
||
| try (MockedStatic<VRunner> vrunner = Mockito.mockStatic(VRunner.class)) { | ||
| vrunner.when(VRunner::getVRunnerPath).thenReturn("vrunner"); | ||
| vrunner.when(() -> VRunner.exec(anyString(), eq(true))).thenAnswer(invocation -> { | ||
| commands.add(invocation.getArgument(0)); | ||
| return 0; | ||
| }); | ||
|
|
||
| // when | ||
| new InitInfoBase(jobConfiguration).run(); | ||
|
|
||
| // then | ||
| assertThat(commands.get(0)).doesNotContain("--exitCodePath"); | ||
| vrunner.verify(() -> VRunner.readExitStatusFromFile(anyString(), any(Integer.class)), never()); | ||
| verify(steps, never()).error(anyString()); | ||
| } | ||
| } |
There was a problem hiding this comment.
Нет явного стаба steps.fileExists - тест полагается на дефолт TestUtils.getMockedStepExecutor(), который вызывает new File(path).exists():
when(steps.fileExists(anyString())).thenAnswer(invocation -> {
String file = invocation.getArgument(0);
return new File(file).exists();
});То есть тест зелёный только потому, что на машине CI не существует файла src/cf/CommonModules/ОбновлениеИнформационнойБазыБСП/Ext/Module.bsl. Если кто-то однажды положит такой путь в тестовых ресурсах или поменяет дефолт getMockedStepExecutor - тест начнёт мигать, причём симптом будет неочевидный («почему-то решила, что БСП»).
Сделать поведение явным:
// given
...
JobConfiguration jobConfiguration = ConfigurationReader.create(config);
when(steps.fileExists(anyString())).thenReturn(false);
List<String> commands = new ArrayList<>();Тогда тест ловит ровно тот контракт, который проверяет («если BspDetector вернул false - --exitCodePath не добавляется»), без зависимости от FS.
There was a problem hiding this comment.
Исправил в 7d7dbac: в runDoesNotUseMigrationStatusFileForNonBspConfiguration добавил явный стаб when(steps.fileExists(anyString())).thenReturn(false). Теперь тест не зависит от реальной файловой системы и дефолтного поведения TestUtils.getMockedStepExecutor().
Три замечания новых если исправить, то помоему все готово. |
@johnnyshut замечания эти скорректировал. |
| "bouncycastle-api", | ||
| "cloudbees-folder", | ||
| "command-launcher", | ||
| "config-file-provider", |
There was a problem hiding this comment.
Это новая зависимость от фичи с переопределением конфига через файлы в дженкинсе. Пропущено от предыдущего pr
There was a problem hiding this comment.
Да, я затупил, а потом вспомнил
|
|
||
| static boolean isBspConfiguration(JobConfiguration config) { | ||
| IStepExecutor steps = ContextRegistry.getContext().getStepExecutor() | ||
| String sourceDir = config.srcDir?.trim()?.replace('\\', '/') |
There was a problem hiding this comment.
config.srcDir?.trim()?.replace('\','/') не убирает хвостовой слеш → при srcDir с / на конце путь получит //CommonModules. Добавить нормализацию (напр. .replaceAll('/+$',''))
| if (type.isInstance(current)) { | ||
| return type.cast(current) | ||
| } | ||
| if (current.cause != null && current.cause.is(current)) { |
There was a problem hiding this comment.
защита от цикла причин ловит только самоссылку (cause.is(current)), длинный цикл A→B→A зациклит. Низкий риск; надёжнее обходить через identity-set
| steps.error("Инициализация ИБ завершилась с ошибками") | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
добавлена лишняя пустая строка перед закрывающей скобкой класса (косметика)
|
@nixel2007 когда примем PR? |
3 participants
Продолжение #189
При включенном runMigration.
Теперь корректно работает Без БСП:
И корректно с БСП:
