feat(test): expose stable JUnit and Allure artifacts - #52
Conversation
- specify native JUnit and Allure contracts - define report-authoritative terminal classification
- define TDD implementation tasks - require independent verification and Rust review
- distinguish JUnit and Allure outputs - retain supported test artifacts on success
- configure simultaneous native reports - inventory only materialized run outputs
- materialize JUnit and Allure fixture outputs\n- expose only existing retained artifact paths
- aggregate deterministic JUnit summaries - preserve test failures across nonzero exits
- attach existing run artifacts to post-setup interruptions - make test completion policies exhaustive
- cover CLI and MCP artifact contracts - document retained test diagnostics
- restore legacy artifact compatibility and retain build failures - secure runner log materialization and iterative artifact discovery - inventory optional diagnostics with deterministic typed paths
- reject source reparse points through verified no-follow handles - atomically replace logs with write-through MoveFileExW - preserve extended-length paths and add Windows guard coverage
- preserve no-build preflight before artifact allocation - retain build-first artifacts across infrastructure failures - align typed test errors and external artifact documentation
|
Warning Review limit reached
Next review available in: 36 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
WalkthroughИзменён контракт результатов тестов для YaXUnit и Vanessa: добавлены одновременные JUnit и Allure, стабильная инвентаризация артефактов, report-first классификация исходов, сохранение run-директорий и расширенные CLI/MCP-тесты. ChangesКонтракт и статусы
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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
🧹 Nitpick comments (8)
src/use_cases/run_tests/helpers.rs (1)
173-182: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winТест не проверяет сам инвентарь, только
root_dir.
artifacts.push(...)добавляетArtifactRefс рольюARTIFACT_ROLE_RUN_DIR, но ассерты его не касаются — реализация, теряющаяitems, всё равно прошла бы тест, хотя имя теста обещает «retains existing inventory».💚 Предлагаемое усиление
- assert_eq!( - result.execution.artifacts.and_then(|set| set.root_dir), - Some(run_dir.path().to_path_buf()) - ); + let retained = result.execution.artifacts.expect("retained artifacts"); + assert_eq!(retained.root_dir, Some(run_dir.path().to_path_buf())); + assert_eq!(retained.items.len(), 1); + assert_eq!(retained.items[0].kind, ArtifactKind::RunDirectory); + assert_eq!( + retained.items[0].role.as_deref(), + Some(ARTIFACT_ROLE_RUN_DIR) + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/use_cases/run_tests/helpers.rs` around lines 173 - 182, Усильте проверки в тесте, сохраняющем инвентарь, рядом с ассертом `result.execution.artifacts`: проверьте, что коллекция `items` содержит добавленный `ArtifactRef` с ролью `ARTIFACT_ROLE_RUN_DIR`. Сохраните существующую проверку `root_dir` и убедитесь, что тест завершится ошибкой при потере или изменении записи инвентаря.tests/cli_test.rs (3)
311-321: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueОбёртка
setup_project_with_additional_launch_keysстала пустым делегатом.Она передаёт аргументы в
setup_project_with_native_reportsодин в один. Можно оставить одну функцию (или сделать обёртку#[inline]-алиасом), чтобы не плодить два имени с одинаковой сигнатурой.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/cli_test.rs` around lines 311 - 321, Remove the redundant setup_project_with_additional_launch_keys wrapper, since it delegates unchanged to setup_project_with_native_reports; update its callers to use setup_project_with_native_reports directly while preserving the existing arguments and behavior.
468-484: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueПять почти идентичных блоков скраба.
Можно свернуть в цикл по парам
(key, placeholder).♻️ Вариант
- if value["data"]["retained_paths"]["config_json"].is_string() { - value["data"]["retained_paths"]["config_json"] = Value::String("<config_json>".to_owned()); - } - if value["data"]["retained_paths"]["junit_xml"].is_string() { - value["data"]["retained_paths"]["junit_xml"] = Value::String("<junit_xml>".to_owned()); - } - if value["data"]["retained_paths"]["allure_results"].is_string() { - value["data"]["retained_paths"]["allure_results"] = - Value::String("<allure_results>".to_owned()); - } - if value["data"]["retained_paths"]["yaxunit_log"].is_string() { - value["data"]["retained_paths"]["yaxunit_log"] = Value::String("<yaxunit_log>".to_owned()); - } - if value["data"]["retained_paths"]["platform_log"].is_string() { - value["data"]["retained_paths"]["platform_log"] = - Value::String("<platform_log>".to_owned()); - } + for key in [ + "config_json", + "junit_xml", + "allure_results", + "yaxunit_log", + "platform_log", + ] { + if value["data"]["retained_paths"][key].is_string() { + value["data"]["retained_paths"][key] = Value::String(format!("<{key}>")); + } + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/cli_test.rs` around lines 468 - 484, Сократите повторяющиеся блоки нормализации в обработке retained_paths, объединив ключи и соответствующие плейсхолдеры в цикл по парам. Сохраните текущую проверку is_string и те же значения для config_json, junit_xml, allure_results, yaxunit_log и platform_log.
191-224: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winМолчаливые
String::replaceвwrite_va_test_scriptмогут дать ложно-зелёные тесты.Скрипт собирается серией текстовых замен: если хотя бы один шаблон (
junit_output,allure_output,mkdir -p "$report_dir" "$allure_dir") перестанет совпадать после правки базовогоbody,replaceтихо ничего не заменит и фикстураMissingJunit/EmptyAllureфактически превратится вComplete— тест перестанет проверять то, что заявлено. Для YaXUnit рядом уже используется более надёжный подход (NativeReportFixture::materializationсобирает фрагменты сразу). Как минимум стоит проверять, что замена состоялась.♻️ Минимальная страховка
+ fn replace_once(body: &str, from: &str, to: &str) -> String { + let replaced = body.replace(from, to); + assert_ne!(replaced, body, "VA script fixture pattern not found: {from}"); + replaced + }Далее использовать
replace_onceвместоbody.replace(...)во всех веткахmatch native_reportsи в подготовительных заменах.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/cli_test.rs` around lines 191 - 224, Update write_va_test_script so every fixture-specific replacement, including the preparatory body replacements and all NativeReportFixture match branches, uses a checked single-replacement operation such as replace_once instead of silent String::replace. Ensure each expected template match is validated and fails if absent, preserving the intended MissingJunit, EmptyJunit, MissingAllure, and EmptyAllure fixture behavior.tests/mcp_stdio.rs (1)
346-346: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winВыбирайте отчеты по
format, а не по позиции вreports.Mock парсит
reports[0]как JUnit иreports[1]как Allure, хотя текущий YaXUnit-генератор хранит отчеты только в одном порядке. Этот подход делает фикстуру хрупкой: изменение/перестановка порядка отчетов в конфиге может закомить XML-файл в Allure-каталог или наоборот. Выбирайте запись по значениюformat/typeв обеих мок-фикстурах —tests/mcp_stdio.rs#L346и аналогичные места вtests/cli_test.rs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/mcp_stdio.rs` at line 346, Update the mock report-selection logic in the embedded script and analogous fixtures in tests/cli_test.rs to identify JUnit and Allure reports by their format/type fields instead of reports array positions. Use the matching report paths for XML and Allure output, preserving the existing fixture generation behavior.src/domain/artifact.rs (1)
97-102: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueРазвязать лайфтайм
roleи&selfвget_all_by_role.Сейчас
role: &'a strпривязан к тому же лайфтайму, что и&'a self, поэтому временнаяStringв качестве роли не скомпилируется, хотя итератор ссылки на неё не хранит.♻️ Предлагаемая сигнатура
- pub fn get_all_by_role<'a>(&'a self, role: &'a str) -> impl Iterator<Item = &'a Path> + 'a { + pub fn get_all_by_role<'a, 'r: 'a>( + &'a self, + role: &'r str, + ) -> impl Iterator<Item = &'a Path> + 'a { self.items .iter() .filter(move |item| item.role.as_deref() == Some(role)) .map(|item| item.path.as_path()) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/domain/artifact.rs` around lines 97 - 102, Update get_all_by_role so the role parameter has an independent lifetime from the borrow of self, while retaining the iterator’s lifetime tied to self and preserving the existing role comparison and path mapping.src/cli/execute.rs (1)
1877-1898: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueДобавьте
allure_resultsв текстовый перечень артефактов.
RetainedPathsтеперь содержит каталог Allure, но текстовый вывод перечисляет только run_dir, report, runner_log и platform_log, поэтому пользователю CLI путь к Allure не виден (он доступен лишь в JSON).♻️ Предлагаемое дополнение
if let Some(junit_xml) = paths.junit_xml { push_unique_detail( details, format!("[artifact] report -> {}", junit_xml.display()), ); } + if let Some(allure_results) = paths.allure_results { + push_unique_detail( + details, + format!("[artifact] allure_results -> {}", allure_results.display()), + ); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cli/execute.rs` around lines 1877 - 1898, Добавьте в текстовый перечень артефактов в соответствующем блоке обработки RetainedPaths запись для каталога allure_results, используя push_unique_detail и формат, аналогичный run_dir/report, чтобы путь к Allure отображался в CLI наряду с остальными артефактами.src/use_cases/run_tests.rs (1)
796-807: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueИнвентарь необязательных диагностик не ограничен по количеству.
push_optional_diagnosticsдобавляет в публичныйArtifactSetкаждый файл изerror-details/иscreenshots/рекурсивно. Прогон Vanessa со скриншотом на каждый шаг может дать тысячи элементов и раздуть JSON-конверт CLI/MCP. Рассмотрите верхний предел с усечением и отдельным элементом-каталогом.Also applies to: 853-860
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/use_cases/run_tests.rs` around lines 796 - 807, Ограничьте количество необязательных диагностик, добавляемых через push_optional_diagnostics для error_details_dir и screenshots_dir, единым верхним пределом. При превышении лимита сохраняйте усечение и добавляйте отдельный элемент-каталог, указывающий на оставшиеся файлы, чтобы ArtifactSet и CLI/MCP JSON не разрастались.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/use_cases/run_tests.rs`:
- Around line 232-416: Добавьте Windows-тестовое покрытие для
materialize_vanessa_runner_log и связанных функций open_file_no_follow,
ensure_windows_handle_is_not_reparse_point и replace_file, поскольку текущий
windows-latest Rust CI выполняет только cargo check. Обеспечьте тесты для
успешной материализации, обработки symlink/reparse-point источника и замены
существующего файла через Windows-реализацию MoveFileExW, используя
Windows-совместимые временные пути и настройки тестового запуска.
---
Nitpick comments:
In `@src/cli/execute.rs`:
- Around line 1877-1898: Добавьте в текстовый перечень артефактов в
соответствующем блоке обработки RetainedPaths запись для каталога
allure_results, используя push_unique_detail и формат, аналогичный
run_dir/report, чтобы путь к Allure отображался в CLI наряду с остальными
артефактами.
In `@src/domain/artifact.rs`:
- Around line 97-102: Update get_all_by_role so the role parameter has an
independent lifetime from the borrow of self, while retaining the iterator’s
lifetime tied to self and preserving the existing role comparison and path
mapping.
In `@src/use_cases/run_tests.rs`:
- Around line 796-807: Ограничьте количество необязательных диагностик,
добавляемых через push_optional_diagnostics для error_details_dir и
screenshots_dir, единым верхним пределом. При превышении лимита сохраняйте
усечение и добавляйте отдельный элемент-каталог, указывающий на оставшиеся
файлы, чтобы ArtifactSet и CLI/MCP JSON не разрастались.
In `@src/use_cases/run_tests/helpers.rs`:
- Around line 173-182: Усильте проверки в тесте, сохраняющем инвентарь, рядом с
ассертом `result.execution.artifacts`: проверьте, что коллекция `items` содержит
добавленный `ArtifactRef` с ролью `ARTIFACT_ROLE_RUN_DIR`. Сохраните
существующую проверку `root_dir` и убедитесь, что тест завершится ошибкой при
потере или изменении записи инвентаря.
In `@tests/cli_test.rs`:
- Around line 311-321: Remove the redundant
setup_project_with_additional_launch_keys wrapper, since it delegates unchanged
to setup_project_with_native_reports; update its callers to use
setup_project_with_native_reports directly while preserving the existing
arguments and behavior.
- Around line 468-484: Сократите повторяющиеся блоки нормализации в обработке
retained_paths, объединив ключи и соответствующие плейсхолдеры в цикл по парам.
Сохраните текущую проверку is_string и те же значения для config_json,
junit_xml, allure_results, yaxunit_log и platform_log.
- Around line 191-224: Update write_va_test_script so every fixture-specific
replacement, including the preparatory body replacements and all
NativeReportFixture match branches, uses a checked single-replacement operation
such as replace_once instead of silent String::replace. Ensure each expected
template match is validated and fails if absent, preserving the intended
MissingJunit, EmptyJunit, MissingAllure, and EmptyAllure fixture behavior.
In `@tests/mcp_stdio.rs`:
- Line 346: Update the mock report-selection logic in the embedded script and
analogous fixtures in tests/cli_test.rs to identify JUnit and Allure reports by
their format/type fields instead of reports array positions. Use the matching
report paths for XML and Allure output, preserving the existing fixture
generation behavior.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 85d9ed81-f097-4da9-933e-fdbf56ead8b6
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.locktests/snapshots/cli_test__test_module_compact_json.snapis excluded by!**/*.snaptests/snapshots/cli_test__test_module_full_json.snapis excluded by!**/*.snap
📒 Files selected for processing (19)
Cargo.tomlSKILL/SKILL.mdSKILL/references/testing.mddocs/CAPABILITIES.mddocs/superpowers/plans/2026-07-26-test-result-artifacts.mddocs/superpowers/specs/2026-07-26-test-result-artifacts-design.mdsrc/cli/execute.rssrc/domain/artifact.rssrc/domain/runner.rssrc/domain/test.rssrc/mcp/service.rssrc/use_cases/request.rssrc/use_cases/run_tests.rssrc/use_cases/run_tests/coordinator.rssrc/use_cases/run_tests/helpers.rssrc/use_cases/vanessa.rstests/architecture_guardrails.rstests/cli_test.rstests/mcp_stdio.rs
- publish timeout logs only when the regular file exists - validate exact text and JSON artifact paths across platforms
- expose Allure artifacts in text output and strengthen inventory assertions - run targeted Windows materialization and atomic replacement tests - document the expanded Windows contract coverage
- bound optional diagnostic inventory with directory fallbacks - document fixture hardening and lifetime waiver
- define TDD steps for bounded diagnostics - cover fixture hardening and verification gates
- cap diagnostic files across error details and screenshots - retain truncated category directories and document the contract
- remove positional assumptions from CLI and MCP fixtures
- fail fast when Vanessa fixture templates drift - remove duplicated setup and snapshot normalization
|
Дополнительные review nitpicks исправлены в
Замечание по lifetime Targeted verification: artifact collector 5/5, CLI suite 30/30, затронутые MCP contracts 3/3. Полный результат Linux/Windows подтвердит CI ветки. |
- propagate selector failures before artifact writes - cover reordered and missing report formats - document shared diagnostic inventory bounds
|
Финальный review выявил и исправил ещё один fixture-safety edge case в Свежая локальная проверка HEAD: artifact collector 5/5, CLI suite 32/32, затронутые MCP contracts 3/3, |
1 participant
Summary
--no-buildcontract: file-infobase preflight runs before artifact allocation, while MCP remains build-firstCloses #26
Verification
cargo fmt --all -- --checkcargo check --lockedcargo test --locked --test cli_test— 30 passedcargo test --locked --bin v8-runner use_cases::run_tests— 30 passedcargo test --locked --test architecture_guardrails— 5 passedcargo test --locked --bin v8-runner mcp::service::tests— 36 passedcargo test --locked --test mcp_stdio mcp_stdio_run_— 2 passedcargo test --locked --all-targets— 718 passed; the same 48 known macOS/sandbox-sensitive baseline failures remain (/private/varcanonicalization, denied local binds, EDT/interactive timing, and launch temp-log races); no issue test: возвращать JUnit/Allure/log как стабильные artifacts #26 test failedReview notes
windows-sys 0.61.2and covered by architecture guards; this macOS host could not execute the Windows path, so the repository Windows CI job is the runtime gateSummary by CodeRabbit