test: add Bats coverage for uncovered Makefile targets#301
Merged
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR introduces a complete Bats shell test framework to verify Makefile targets and CI batch scripts, with comprehensive test coverage, contract enforcement, and CI integration. Key additions: test helpers with command stubbing, 265+ lines of Makefile target tests, 87 lines of CI script validation, contract tests ensuring manifest and documentation consistency, GitHub Actions workflow to run tests, and removal of a redundant ChangesBats Shell Testing Infrastructure and Coverage
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 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. 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 |
![qltysh[bot]](https://avatars.githubusercontent.com/in/890766?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 13 files
Confidence score: 4/5
- This PR looks safe to merge overall: both reported issues are low severity (4/10) and appear to be process/security-hardening concerns rather than core runtime regressions.
- In
CONTRIBUTING.md, the step order can mislead contributors—runningmake test-batsbefore updating the coverage TSV can trigger a contract-test diff failure and create avoidable friction. - In
.github/workflows/bats-testing.yml, settingpersist-credentials: falseonactions/checkout@v4would reduce token exposure in.git/configduring the job; this is a good hardening improvement even if current behavior may still work. - Pay close attention to
CONTRIBUTING.mdand.github/workflows/bats-testing.yml- fix the step sequencing to avoid false test failures and tighten checkout credential handling.
Architecture diagram
sequenceDiagram
participant Dev as Developer
participant Make as make
participant Bats as Bats Test Runner
participant Helper as test_helper.bash
participant Stubs as Stub Binaries
participant Targets as Makefile Targets
participant CIScripts as scripts/ci/*.sh
participant Manifest as make-target-coverage.tsv
participant CI as GitHub Actions (PR workflow)
Note over Dev,CI: Bats coverage for Makefile targets & CI helper scripts
Dev->>Make: make test-bats
Make->>Bats: bats --formatter $(BATS_FORMATTER) -r tests/bats
Note over Bats: Environment vars exported (DOCKER_COMPOSE_*, COMMON_HEALTHCHECKS_FILE)
Bats->>Helper: load test_helper.bash
Helper->>Helper: setup_stub_dir()
Helper->>Stubs: create docker, curl, tar, pnpm, etc. stubs
Helper->>Stubs: create make stub (for CI script tests)
Helper->>Targets: copy Makefile to sandbox
Helper->>Targets: setup_makefile_test_env()
alt Coverage rule tests
Bats->>Manifest: test: every Makefile target in manifest
Manifest-->>Bats: return diff result
alt Missing target
Bats-->>Make: FAIL (target not covered)
else All covered
Bats-->>Make: PASS
end
Bats->>Manifest: test: Bats-covered targets point to existing test files
Manifest-->>Bats: validate
else Makefile target validation tests
Bats->>Targets: run_make_target help
Targets->>Stubs: stub help output
Stubs-->>Targets: usage text
Targets-->>Bats: assert "test-bats" in output
loop For each DIND target
Bats->>Targets: run_make_target create-temp-dev-container-dind TEMP_CONTAINER_NAME=website-dev-test
Targets->>Stubs: docker rm -f …, docker compose run …
Stubs-->>Targets: log entry
Targets-->>Bats: assert_log_contains 'docker rm …'
end
Bats->>Targets: run_make_target target_name (no required var)
alt Required variable missing
Targets->>Stubs: exit with error
Stubs-->>Targets: error message
Bats->>Bats: assert status != 0, output contains "Error: … is required."
else Variable provided
Targets->>Stubs: normal stub execution
Bats->>Bats: assert expected commands logged
end
else CI script integration tests
Bats->>CIScripts: run_ci_script scripts/ci/batch_pw_load.sh test-e2e
CIScripts->>Stubs: make start-prod, make test-e2e, docker compose …
Stubs-->>CIScripts: log all commands
CIScripts-->>Bats: return status 0
Bats->>Bats: assert_log_contains 'make start-prod'
Bats->>Bats: assert_log_contains 'make test-e2e'
Bats->>CIScripts: run_ci_script scripts/ci/batch_lhci_leak.sh test-memory-leak
alt CodeBuild environment
CIScripts->>CIScripts: CODEBUILD_BUILD_ID set → skip memory‑leak
CIScripts-->>Bats: output "Memory leak tests: SKIPPED"
Bats->>Bats: assert_output_contains 'SKIPPED'
else Non-CodeBuild
CIScripts->>Stubs: normal flow (make start-prod, etc.)
end
else Contract tests (issue 175)
Bats->>Manifest: test: every Makefile target appears in manifest
Manifest-->>Bats: targets list
Bats->>Targets: extract targets from Makefile
Targets-->>Bats: list of targets
Bats->>Bats: diff between expected and manifest
alt Missing target
Bats-->>Make: FAIL
else All present
Bats-->>Make: PASS
end
Bats->>CIScripts: test: CI scripts only reference existing Make targets
CIScripts-->>Bats: list of make calls from scripts
Bats->>Targets: extract targets from Makefile
Targets-->>Bats: list of targets
Bats->>Bats: comm -23 script_targets known_targets
alt Has missing target
Bats-->>Make: FAIL
else
Bats-->>Make: PASS
end
Bats->>CI: test: PR workflow runs make test-bats with read-only permissions
CI-->>Bats: verify .github/workflows/bats-testing.yml
Bats->>Bats: asserts pull_request trigger and contents:read
Bats-->>Make: PASS
end
Bats-->>Make: Test suite results (exit code)
Make-->>Dev: output and exit status
Note over CI: On pull_request to main, read-only
CI->>CI: checkout, setup Node, pnpm install
CI->>Make: make test-bats BATS_FORMATTER=tap
Note over CI: Ensure no write permissions (contents: read)
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
tests/bats/test_helper.bash (1)
151-160: ⚡ Quick winDowngrade:
env -Cshould work on macOS/BSD; this is not a portability bug
Line 156’senv -C "$SCRIPT_SANDBOX"is documented on BSD-derivedenv(1)(including macOS), so the “breaks on macOS/BSD” claim doesn’t hold. Thecd "$SCRIPT_SANDBOX" && run env ...change is optional hardening for broader non-BSD env compatibility.🤖 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/bats/test_helper.bash` around lines 151 - 160, The claim that env -C is incompatible with macOS/BSD is incorrect; keep the current run_ci_script implementation using run env -C "$SCRIPT_SANDBOX" rather than changing to cd "$SCRIPT_SANDBOX" && run env ...; ensure the run_ci_script function (and its use of SCRIPT_SANDBOX / run env -C) remains as-is and optionally add a brief comment noting that env -C is supported on BSD/macOS to prevent future reverts.tests/bats/ci_scripts.bats (1)
50-58: ⚡ Quick winMINOR: Add an explicit regression check that
batch_pw_load.shnever callsmake build-prod(Line 53+).This PR’s key contract is removing that stale target call; add one negative assertion so future edits can’t silently reintroduce it.
Suggested patch
run_ci_script "$script_path" test-load [ "$status" -eq 0 ] assert_log_contains 'make start-prod' + if grep -F -- 'make build-prod' "$COMMAND_LOG" >/dev/null 2>&1; then + echo "Unexpected stale target invocation: make build-prod" >&2 + return 1 + fi assert_log_contains 'make build-k6' assert_log_contains 'make create-k6-helper-container-dind K6_HELPER_NAME=website-k6-helper' assert_log_contains 'make run-load-tests-dind K6_HELPER_NAME=website-k6-helper' assert_log_contains 'docker cp src/test/load/. website-k6-helper:/loadTests/'🤖 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/bats/ci_scripts.bats` around lines 50 - 58, Add a negative regression check to ensure the CI run for "test-load" never invokes the stale target "make build-prod": after the run_ci_script "$script_path" test-load invocation (and alongside the existing assert_log_contains checks), add an assertion that the captured log does not include 'make build-prod' (use the project's existing negative assertion helper, e.g., assert_log_not_contains or equivalent refute/assert pattern) so future changes cannot reintroduce that call.
🤖 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/bats-testing.yml:
- Around line 17-20: Replace tag-based action references with pinned commit SHAs
for both actions/checkout and actions/setup-node to prevent unexpected changes;
update the checkout step (actions/checkout) to include with:
persist-credentials: false and ensure workflow-level permissions includes
contents: read since no pushing occurs. Locate the steps that reference "uses:
actions/checkout@v4" and "uses: actions/setup-node@v4", swap the `@v4` tags for
the corresponding commit SHAs and add the persist-credentials setting to the
checkout step, and set workflow permissions to "contents: read".
In `@tests/bats/issue_175_contract.bats`:
- Around line 67-73: The test only inspects the first workflow_path from
"$workflows_with_bats" and only asserts presence of "contents: read"; update the
test to iterate over every workflow listed in "$workflows_with_bats" (e.g.,
replace the single-assignment of workflow_path with a loop like `while read -r
workflow_path; do ... done < "$workflows_with_bats"`) and for each file run the
existing checks `grep -F 'pull_request:'` and `grep -F 'contents: read'`, plus
add a negative check that fails if `grep -F 'contents: write'` matches (ensure
the test asserts non-zero status for any contents: write match).
- Around line 33-35: The test currently uses substring matching when grepping
for "$target" in "$PROJECT_ROOT/$evidence" which can false-pass (e.g., "test"
matches "test-bats"); update the grep invocation in
tests/bats/issue_175_contract.bats to perform an exact full-line fixed-string
match instead of a substring match (use grep's fixed-string plus "match whole
line" option) when checking the variable target against the evidence file so
only an exact line equal to target will succeed; adjust the command that
references the variables target, PROJECT_ROOT and evidence accordingly.
In `@tests/bats/makefile_targets.bats`:
- Around line 20-24: The negative-path tests are flaky if required env vars like
TEMP_CONTAINER_NAME or K6_HELPER_NAME are already exported; before calling
run_make_target in the while loop that reads target and required_var, explicitly
unset the environment variable named by $required_var (e.g., run unset
"$required_var") so the failure path is deterministic, then call run_make_target
and perform the existing assertions.
---
Nitpick comments:
In `@tests/bats/ci_scripts.bats`:
- Around line 50-58: Add a negative regression check to ensure the CI run for
"test-load" never invokes the stale target "make build-prod": after the
run_ci_script "$script_path" test-load invocation (and alongside the existing
assert_log_contains checks), add an assertion that the captured log does not
include 'make build-prod' (use the project's existing negative assertion helper,
e.g., assert_log_not_contains or equivalent refute/assert pattern) so future
changes cannot reintroduce that call.
In `@tests/bats/test_helper.bash`:
- Around line 151-160: The claim that env -C is incompatible with macOS/BSD is
incorrect; keep the current run_ci_script implementation using run env -C
"$SCRIPT_SANDBOX" rather than changing to cd "$SCRIPT_SANDBOX" && run env ...;
ensure the run_ci_script function (and its use of SCRIPT_SANDBOX / run env -C)
remains as-is and optionally add a brief comment noting that env -C is supported
on BSD/macOS to prevent future reverts.
🪄 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
Run ID: 35854083-481d-4d8f-a24d-10a1dfe06d5f
⛔ Files ignored due to path filters (2)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamltests/bats/make-target-coverage.tsvis excluded by!**/*.tsv
📒 Files selected for processing (11)
.github/workflows/bats-testing.ymlCONTRIBUTING.mdMakefileREADME.mdpackage.jsonscripts/ci/batch_pw_load.shtests/bats/README.mdtests/bats/ci_scripts.batstests/bats/issue_175_contract.batstests/bats/makefile_targets.batstests/bats/test_helper.bash
Contributor
Author
|
@cubic-dev-ai review |
@RudoiDmytro I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
1 issue found
Confidence score: 5/5
- This PR is safe to merge with low risk: the reported issue in
.github/workflows/bats-testing.ymlis low severity (3/10) and affects CI efficiency rather than product behavior. - The missing
actions/cachestep fornode_modulesmay increase workflow runtime and network usage, but it is unlikely to introduce functional regressions in the codebase. - Pay close attention to
.github/workflows/bats-testing.yml- add pnpm dependency caching to match existing workflow patterns and avoid repeated package downloads.
Architecture diagram
sequenceDiagram
participant Dev as Developer
participant CI as GitHub Actions CI
participant Makefile as Makefile
participant Bats as Bats Runner
participant Helper as Test Helper (test_helper.bash)
participant Stubs as Stubbed Commands
participant Manifest as Coverage Manifest (make-target-coverage.tsv)
participant Scripts as CI Scripts (scripts/ci/)
Note over Dev,Scripts: NEW: Bats shell coverage suite
Dev->>Makefile: make test-bats BATS_FORMATTER=pretty
CI->>Makefile: make test-bats BATS_FORMATTER=tap (on PR to main)
Makefile->>Makefile: Export DOCKER_COMPOSE_TEST_FILE, DEV_FILE, etc.
Makefile->>Bats: pnpm exec bats --formatter $(BATS_FORMATTER) -r tests/bats
Bats->>Helper: setup() per test file
Helper->>Stubs: Create stubs (docker, curl, pnpm, make, next, etc.) in $STUB_BIN_DIR
Helper->>Stubs: Override PATH to use stubs
alt Test Makefile target (e.g., create-temp-dev-container-dind)
Bats->>Helper: run_make_target <target> [VAR=value]
Helper->>Makefile: make -C sandbox <target> BIN_DIR=$STUB_BIN_DIR ...
Makefile->>Stubs: Execute real shell commands via stubbed PATH
Stubs->>Stubs: Log invocation to COMMAND_LOG
Stubs-->>Makefile: exit 0
Makefile-->>Helper: status code, output
Helper-->>Bats: return raw output
Bats->>Helper: assert_log_contains "docker exec ..."
Helper->>Stubs: grep COMMAND_LOG for expected string
Stubs-->>Helper: match / no match
Helper-->>Bats: pass / fail
else Test CI script (e.g., batch_unit_mutation_lint.sh)
Bats->>Helper: run_ci_script <script> <subcommand>
Helper->>Scripts: Execute actual script with stubbed PATH & env
Scripts->>Stubs: Calls to docker, make, tar etc. go to stubs
Stubs->>Stubs: Log invocations
Stubs-->>Scripts: exit 0
Scripts-->>Helper: status, output
Helper-->>Bats: return raw output
Bats->>Helper: assert_log_contains "make build"
Helper->>Stubs: check log
Stubs-->>Helper: result
Helper-->>Bats: pass / fail
else Validate coverage manifest
Bats->>Manifest: Read make-target-coverage.tsv
Bats->>Makefile: Extract all target names (awk)
Bats->>Bats: diff manifest targets vs extracted targets
Bats->>Manifest: For each bats-covered target, verify evidence file exists and references target
Bats->>Scripts: grep -R 'make <target>' to detect hardcoded targets
Bats->>Makefile: compare against known targets
alt Missing target
Bats->>Bats: fail test
else All accounted for
Bats->>Bats: pass
end
end
Note over Bats: Results aggregated from all test files
Bats-->>Makefile: exit status
Makefile-->>Dev: output and return code
Makefile-->>CI: output and return code (TAP format)
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
Kravalg
approved these changes
Jun 4, 2026
| ${{ runner.os }}-dependencies-node-${{ vars.NODE_VERSION }}- | ||
|
|
||
| - name: Install pnpm | ||
| run: npm install -g pnpm@10.6.5 |
Contributor
There was a problem hiding this comment.
Let's add GitHub issue where we will move from pnpm to faster package manager that we are using in the CRM
This was referenced Jun 5, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Test Plan
Issue