refactor(integration-tests): Use ExternalAction flows throughout the testing suite; deprecate log_subprocess_output_to_file().#2242
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThis PR replaces ad-hoc subprocess invocation helpers with an ExternalAction abstraction across integration tests and fixtures, introduces fs_validation helpers for filesystem/JSON comparison, removes the old subprocess logging utility, and updates call sites to check ExternalAction.completed_proc.returncode and call pytest.fail(format_action_failure_msg(...)) on failures. (39 words) ChangesIntegration-test External Command and FS Validation Refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Ruff (0.15.12)integration-tests/tests/utils/docker_utils.py�[1;31mruff failed�[0m integration-tests/tests/utils/fs_validation.py�[1;31mruff failed�[0m 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: 2
🤖 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 `@integration-tests/tests/test_log_converter.py`:
- Around line 98-99: The conditional `if stdout else []` is redundant—use the
captured stdout directly: replace the `stdout =
search_action.completed_proc.stdout` / `lines = stdout.splitlines() if stdout
else []` pattern by calling `splitlines()` on
`search_action.completed_proc.stdout` (i.e., `lines =
search_action.completed_proc.stdout.splitlines()`) so `lines` is produced
correctly from the `stdout` string; refer to the `stdout` variable and
`search_action.completed_proc.stdout`/`lines` in the change.
In `@integration-tests/tests/utils/fs_validation.py`:
- Around line 18-19: Replace the literal "diff" command with a resolved binary
path using get_binary_path("diff") so missing-binary errors raise RuntimeError
immediately and match the docstring; update the cmd construction in the block
that creates ExternalAction (the variable diff_action and cmd assignment) to
call get_binary_path("diff") and use that returned path, leaving ExternalAction
and ExternalAction._run_subprocess() unchanged.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 221bb561-877c-439c-9998-a608a4d4700b
📒 Files selected for processing (11)
integration-tests/tests/fixtures/integration_test_logs.pyintegration-tests/tests/test_identity_transformation.pyintegration-tests/tests/test_log_converter.pyintegration-tests/tests/utils/asserting_utils.pyintegration-tests/tests/utils/classes.pyintegration-tests/tests/utils/docker_utils.pyintegration-tests/tests/utils/fs_validation.pyintegration-tests/tests/utils/logging_utils.pyintegration-tests/tests/utils/package_utils.pyintegration-tests/tests/utils/subprocess_utils.pyintegration-tests/tests/utils/utils.py
💤 Files with no reviewable changes (2)
- integration-tests/tests/utils/logging_utils.py
- integration-tests/tests/utils/subprocess_utils.py
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@integration-tests/tests/utils/fs_validation.py`:
- Around line 58-63: The NamedTemporaryFile call explicitly passes delete=True
which is redundant because delete=True is the default; update the call that
creates sorted_fp (the NamedTemporaryFile instantiation in fs_validation.py
where sorted_fp is assigned) to omit the delete=True argument so it reads simply
NamedTemporaryFile(mode="w+"), leaving the rest of the usage (write/flush/seek)
unchanged.
- Line 55: The error message for a failing jq run (built from jq_rc and json_fp)
should include jq_action.completed_proc.stderr when available; update the
construction of err_msg in the fs_validation.py logic (where err_msg = f"jq
failed with exit code {jq_rc} for {json_fp}") to append the captured stderr
output (e.g., include a suffix like "stderr:
{jq_action.completed_proc.stderr!r}" only when stderr is present) so diagnostics
are preserved while retaining the existing exit code and filepath information.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 0c3d895c-9aa4-4f8a-b7cd-a9b87a3e0ea0
📒 Files selected for processing (2)
integration-tests/tests/test_log_converter.pyintegration-tests/tests/utils/fs_validation.py
1 participant
Description
This PR migrates the testing suite with respect to the new designs implemented in #2182 (
ExternalAction,CmdMsg). The previous flow that usedlog_subprocess_output_to_file()has been removed, and all related flows have been modified accordingly.Checklist
breaking change.
Validation performed
Ran
uv run pytest -m 'package'anduv run pytest -m 'core'; all tests pass.Summary by CodeRabbit
Tests
Chores