Add act/Docker integration tests for subprocess boundary and _extract…#43
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Docker-backed integration tests and CI separation: pytest now defines an ChangesIntegration Test Coverage for Act Subprocess Boundary
sequenceDiagram
participant CI as GitHub Actions CI
participant Unit as pytest (unit)
participant Integration as pytest (integration job)
participant Act as act
participant Docker as Docker
CI->>Unit: run `test` job (pytest -m "not integration")
CI->>Integration: run `integration` job (install act, run pytest -m integration)
Integration->>Act: invoke act to execute workflows
Act->>Docker: run workflow steps in Docker containers
Act->>Integration: stream captured output/logs
Integration->>CI: report integration test results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 3
🤖 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 @.github/workflows/ci.yml:
- Around line 45-50: The workflow uses floating action tags and persists
checkout credentials; update the uses entries for actions/checkout and
actions/setup-python to their full commit SHA pins (replace values like
actions/checkout@v4 and actions/setup-python@v5 with the corresponding
full-length commit SHAs), add persist-credentials: false under the
actions/checkout step to disable credential persistence, and replace the current
act installer invocation (the curl .../nektos/act/master/install.sh | bash
pattern) with a safe, pinned install flow: fetch a specific release/tag or
commit of nektos/act, verify integrity (e.g., checksum/signature) and run the
installer without piping from an unpinned branch.
- Around line 55-58: Replace the mutable master URL used by the curl in the CI
step that installs act with a tag-pinned raw script URL so the executed
installer is immutable; specifically update the curl target that currently
points to "raw.githubusercontent.com/nektos/act/master/install.sh" (the line
invoking curl in the "Install act" step) to the tag-refed raw URL that
corresponds to the installer version being passed (e.g., the v0.2.76 tag) so the
downloaded script matches the intended release.
In `@cli/tests/integration/test_act_subprocess.py`:
- Around line 18-31: The helper functions are using the module-level
FIXTURE_PROJECT which breaks per-test isolation; modify _workflow_path and
_build_and_run to accept a project_dir (or project_path) parameter and use that
instead of FIXTURE_PROJECT, then update all call sites (including the other
occurrences referenced) to pass the per-test copied project path provided by
conftest; ensure _workflow_path(name, project_dir) builds the Path from
project_dir / ".github/workflows" / name and _build_and_run(workflow_name,
logs_dir, act_runner_image, project_dir) sets project_dir from the passed-in
argument before running the analyzer and building entry.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: fe8f5be5-49b9-4608-ba1b-cc3359789726
📒 Files selected for processing (11)
.github/workflows/ci.ymlcli/README.mdcli/pyproject.tomlcli/tests/fixtures/integration/project/.github/workflows/invalid.ymlcli/tests/fixtures/integration/project/.github/workflows/test-fail.ymlcli/tests/fixtures/integration/project/.github/workflows/test.ymlcli/tests/fixtures/integration/project/.localci.ymlcli/tests/integration/__init__.pycli/tests/integration/conftest.pycli/tests/integration/test_act_subprocess.pycli/tests/integration/test_localci_run.py
|
Actionable comments posted: 0 |
|
@coderabbitai continue to review |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Get PR details and changed files
gh pr view 43 --repo cppalliance/local-ci-test-system --json files,headRefName,baseRefName
echo "=== List files in the repo ==="
fd . cli/tests/integration --type f 2>/dev/null || echo "not found locally"
fd . cli/tests/fixtures/integration --type f 2>/dev/null || echo "not found locally"
echo "=== conftest.py ==="
cat cli/tests/integration/conftest.py 2>/dev/null || echo "file not found"
echo "=== test_act_subprocess.py ==="
cat cli/tests/integration/test_act_subprocess.py 2>/dev/null || echo "file not found"
echo "=== test_localci_run.py ==="
cat cli/tests/integration/test_localci_run.py 2>/dev/null || echo "file not found"
echo "=== workflow fixtures ==="
cat "cli/tests/fixtures/integration/project/.github/workflows/test.yml" 2>/dev/null || echo "not found"
cat "cli/tests/fixtures/integration/project/.github/workflows/test-fail.yml" 2>/dev/null || echo "not found"
cat "cli/tests/fixtures/integration/project/.github/workflows/invalid.yml" 2>/dev/null || echo "not found"
cat "cli/tests/fixtures/integration/project/.localci.yml" 2>/dev/null || echo "not found"
echo "=== pyproject.toml ==="
cat cli/pyproject.toml 2>/dev/null || echo "not found"
echo "=== CI workflow ==="
cat .github/workflows/ci.yml 2>/dev/null || echo "not found"
echo "=== README ==="
cat cli/README.md 2>/dev/null || echo "not found"Length of output: 18789 🏁 Script executed: #!/bin/bash
echo "=== localci run CLI args ==="
rg -n "timeout|logs_dir|log_file" cli/localci/core/executor.py -A2 -B2 2>/dev/null | head -80
echo "=== Check if JobExecutor creates logs_dir ==="
rg -n "logs_dir|mkdir|makedirs" cli/localci/core/executor.py 2>/dev/null | head -40
echo "=== localci run click command signature ==="
rg -n "timeout|parallel|no.cache" cli/localci/cli/ -A1 -B1 2>/dev/null | head -60
echo "=== _extract_error method ==="
rg -n "_extract_error" cli/localci/core/executor.py -A10 2>/dev/null | head -40
echo "=== JobStatus ==="
rg -n "class JobStatus|PASSED|FAILED" cli/localci/core/executor.py 2>/dev/null | head -20
echo "=== ExecutionSummary.load ==="
rg -n "class ExecutionSummary|def load|last.run" cli/localci/core/results.py 2>/dev/null | head -30Length of output: 8461
Review: Add act/Docker Integration Tests (
|
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cli/tests/integration/conftest.py (1)
49-54:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHandle docker pull timeout as a skip, not an error.
cli/tests/integration/conftest.py’sact_runner_imagecallssubprocess.run(..., timeout=DOCKER_PULL_TIMEOUT)but only handlesreturncode; a timeout raisessubprocess.TimeoutExpiredand will fail the session instead of skipping integration tests.Proposed fix
def act_runner_image(require_act_and_docker: None) -> str: """Pull the act runner image used for ubuntu-latest jobs.""" - pull = subprocess.run( - ["docker", "pull", ACT_RUNNER_IMAGE], - capture_output=True, - text=True, - timeout=DOCKER_PULL_TIMEOUT, - ) + try: + pull = subprocess.run( + ["docker", "pull", ACT_RUNNER_IMAGE], + capture_output=True, + text=True, + timeout=DOCKER_PULL_TIMEOUT, + ) + except subprocess.TimeoutExpired: + pytest.skip( + f"docker pull timed out after {DOCKER_PULL_TIMEOUT}s for {ACT_RUNNER_IMAGE}" + ) if pull.returncode != 0: pytest.skip( f"could not pull {ACT_RUNNER_IMAGE}: " f"{pull.stderr.strip() or pull.stdout.strip()}" )🤖 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 `@cli/tests/integration/conftest.py` around lines 49 - 54, The docker pull call in act_runner_image can raise subprocess.TimeoutExpired and should cause tests to be skipped instead of failing; wrap the subprocess.run([...], timeout=DOCKER_PULL_TIMEOUT) call in a try/except that catches subprocess.TimeoutExpired and calls pytest.skip with a clear message (include DOCKER_PULL_TIMEOUT and ACT_RUNNER_IMAGE in the message) so integration tests are skipped when the pull times out; keep the existing returncode handling for non-zero exits.
🤖 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.
Outside diff comments:
In `@cli/tests/integration/conftest.py`:
- Around line 49-54: The docker pull call in act_runner_image can raise
subprocess.TimeoutExpired and should cause tests to be skipped instead of
failing; wrap the subprocess.run([...], timeout=DOCKER_PULL_TIMEOUT) call in a
try/except that catches subprocess.TimeoutExpired and calls pytest.skip with a
clear message (include DOCKER_PULL_TIMEOUT and ACT_RUNNER_IMAGE in the message)
so integration tests are skipped when the pull times out; keep the existing
returncode handling for non-zero exits.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 602e3d68-97a2-4794-8656-56ba419c0256
📒 Files selected for processing (4)
.github/workflows/ci.ymlcli/tests/integration/conftest.pycli/tests/integration/test_act_subprocess.pycli/tests/integration/test_localci_run.py
🚧 Files skipped from review as they are similar to previous changes (1)
- cli/tests/integration/test_act_subprocess.py
There was a problem hiding this comment.
Strong addition for #39 — real act/Docker coverage and a high-value _extract_error check in test_act_subprocess.py. CI layout (unit vs integration job, pinned act binary, needs: [test]) looks good.
Please address the inline comments (mainly docker timeout handling and end-to-end failure assertions on the localci run path) before I approve. Happy to re-review after updates.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@cli/tests/integration/test_localci_run.py`:
- Around line 84-90: The strict equality assertion comparing job.error_message
to JobExecutor._extract_error(captured) is brittle; change it to a containment
check so the normalized stored message can match even if the extracted text has
extra status lines. Replace the equality assertion with: assert
job.error_message in JobExecutor._extract_error(captured), keeping the same
variables (job.error_message, JobExecutor._extract_error, captured).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 16c53ed0-4452-4bf5-8bba-09b7548b2d4d
📒 Files selected for processing (5)
.github/workflows/ci.ymlcli/README.mdcli/tests/integration/conftest.pycli/tests/integration/test_act_subprocess.pycli/tests/integration/test_localci_run.py
💤 Files with no reviewable changes (1)
- .github/workflows/ci.yml
✅ Files skipped from review due to trivial changes (1)
- cli/README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- cli/tests/integration/test_act_subprocess.py
3 participants
Summary
Adds optional pytest integration coverage for the real act/Docker subprocess path (w4_issue_02 / Test 31). Unit tests mock act extensively; these tests validate
JobExecutor.run(),localci run, and_extract_error()against live act output.Changes
cli/tests/integration/)test_act_subprocess.py— success run and failing run with_extract_error()checked against captured act stdout/stderrtest_localci_run.py—localci runfor success, failure, and invalid workflow (no traceback on parse errors)conftest.py— skip when act/Docker missing; pull/tagcatthehacker/ubuntu:act-24.04for CLI runscli/tests/fixtures/integration/project/)test.yml(echo),test-fail.yml(exit 1),invalid.yml(malformed YAML).localci.yml(cache off, single parallel job)cli/pyproject.toml) —integrationmarker;norecursedirsexcludes integration from default collection.github/workflows/ci.yml)pytest -m "not integration"cli/README.md) — how to run unit vs integration testsDesign notes
strategy.matrix.include(one entry) becauseWorkflowAnalyzeronly produces runnable matrix entries that way.capy-ubuntu-latest-gcc15:latestname so the orchestrator finds a local image.test_run_invalid_workflowdoes not require act/Docker (parse fails before execution).Test plan
cd cli && pytest -m "not integration"— unit tests passpytest tests/integration -m integration— invalid-workflow test passes without act; others skip or run when act/Docker availableRelated
Summary by CodeRabbit
Tests
Documentation
Chores