Upgraded the BATS tests to 'bats-helpers' 2.0. - #2894
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 11 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: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (26)
WalkthroughThe PR upgrades ChangesBats helpers migration
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
2 similar comments
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
![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 @.vortex/tooling/tests/unit/fetch-db-container-registry.bats:
- Around line 98-102: Update the existing-host test around the current “Skip
fetch” test name to reflect that fetching continues when no expanded archive
exists. Add an assertion requiring the `Fetching myorg/myapp image from the
registry.` message, while preserving the existing login and pull mock sequence.
- Around line 124-125: The fetch-db-container-registry test currently only stubs
Docker output and does not verify the actual registry/image arguments. Update
the unit test around the mock_docker setup to use STEPS with steps_run "setup"
and steps_run "assert", and assert the Docker call log includes the login target
docker.io and the pull command `@docker` pull docker.io/myorg/myapp. Keep the
existing test flow focused on the fetch-db-container-registry behavior while
replacing the fixed side effect with argument validation.
In @.vortex/tooling/tests/unit/provision.bats:
- Around line 206-211: Update the tooling dependency configuration in
package.json and yarn.lock so bats-helpers resolves to the published npm alias
npm:`@drevops/bats-helpers`@^2.0 rather than the GitHub main branch, keeping the
existing dependency usage unchanged.
🪄 Autofix
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: ASSERTIVE
Plan: Pro Plus
Run ID: 5cf7455b-8a93-4d5f-9d45-21ecf6bda062
⛔ Files ignored due to path filters (1)
.vortex/tooling/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (26)
.vortex/tooling/package.json.vortex/tooling/tests/_helper.bash.vortex/tooling/tests/unit/deploy-artifact.bats.vortex/tooling/tests/unit/deploy-lagoon.bats.vortex/tooling/tests/unit/fetch-db-acquia.bats.vortex/tooling/tests/unit/fetch-db-container-registry.bats.vortex/tooling/tests/unit/fetch-db-lagoon.bats.vortex/tooling/tests/unit/fetch-db-s3.bats.vortex/tooling/tests/unit/fetch-db-url.bats.vortex/tooling/tests/unit/helpers.bats.vortex/tooling/tests/unit/import-db-file.bats.vortex/tooling/tests/unit/login-container-registry.bats.vortex/tooling/tests/unit/notify-github.bats.vortex/tooling/tests/unit/notify-jira.bats.vortex/tooling/tests/unit/notify-newrelic.bats.vortex/tooling/tests/unit/post-coverage-comment.bats.vortex/tooling/tests/unit/provision-enable-demo-modules.bats.vortex/tooling/tests/unit/provision-enable-dev-modules.bats.vortex/tooling/tests/unit/provision-example.bats.vortex/tooling/tests/unit/provision-migration.bats.vortex/tooling/tests/unit/provision-search-index.bats.vortex/tooling/tests/unit/provision.bats.vortex/tooling/tests/unit/push-container-registry.bats.vortex/tooling/tests/unit/push-db-s3.bats.vortex/tooling/tests/unit/setup-ssh.bats.vortex/tooling/tests/unit/update-vortex.bats
| mock_set_side_effect "${mock_docker}" "echo 'logged in'" 2 | ||
| mock_set_side_effect "${mock_docker}" "echo 'pulled from docker.io'" 3 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 5 'mock_(command|set_side_effect)|call|history|verify|assert' \
.vortex/tooling/tests/_helper.bashRepository: drevops/vortex
Length of output: 2981
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- target test ---'
sed -n '90,145p' .vortex/tooling/tests/unit/fetch-db-container-registry.bats
printf '%s\n' '--- helper mock references ---'
rg -n -C 4 'mock_(setup|command|set_side_effect)|mock_[a-z_]+|run_command|output|status|lines|history|call|invocation|docker' \
.vortex/tooling/tests .vortex/tooling 2>/dev/null | head -n 300
printf '%s\n' '--- repository files related to mocking ---'
git ls-files | rg '(^|/)(mock|.*helper.*|.*bats.*)$|bats-helpers|fetch-db-container-registry'Repository: drevops/vortex
Length of output: 30576
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- mock setup and helper loading ---'
sed -n '1,90p' .vortex/tooling/tests/_helper.bash
rg -n -C 8 'steps_run|mock_set_side_effect|mock_set_output|mock_command|mock_setup' \
.vortex/tooling/tests
printf '%s\n' '--- invocation assertions ---'
rg -n -C 8 'STEPS=|steps_run|`@docker`|assert.*(output|line|contains)|mocks=' \
.vortex/tooling/tests/unit | head -n 400
printf '%s\n' '--- target script ---'
sed -n '1,220p' .vortex/tooling/src/vortex-fetch-db-container-registryRepository: drevops/vortex
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- helper loading and step API ---'
sed -n '1,75p' .vortex/tooling/tests/_helper.bash
rg -n -C 12 'steps_run' .vortex/tooling/tests/unit/fetch-db-container-registry.bats
rg -n -C 8 'mock_command|mock_set_side_effect|steps_run' .vortex/tooling/tests/_helper.bash
printf '%s\n' '--- target script ---'
sed -n '1,220p' .vortex/tooling/src/vortex-fetch-db-container-registry
printf '%s\n' '--- relevant assertion examples ---'
sed -n '60,115p' .vortex/tooling/tests/unit/push-container-registry.bats
sed -n '1,65p' .vortex/tooling/tests/unit/fetch-db-s3.batsRepository: drevops/vortex
Length of output: 3271
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- target script ---'
sed -n '1,220p' .vortex/tooling/src/vortex-fetch-db-container-registry
printf '%s\n' '--- target test and step assertions ---'
sed -n '1,180p' .vortex/tooling/tests/unit/fetch-db-container-registry.bats
rg -n -C 12 'steps_run|STEPS=|`@docker`|mock_command|mock_set_side_effect' \
.vortex/tooling/tests/unit/fetch-db-container-registry.bats \
.vortex/tooling/tests/unit/push-container-registry.bats \
.vortex/tooling/tests/unit/fetch-db-s3.bats || true
printf '%s\n' '--- mock library source references ---'
find .vortex/tooling -type f -path '*bats-helpers*' -o -type f -iname '*mock*' 2>/dev/null | head -n 100
rg -n -C 8 'steps_run|mock_set_side_effect|mock_command' \
.vortex/tooling/node_modules 2>/dev/null | head -n 200 || trueRepository: drevops/vortex
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- dependency and mock-library references ---'
rg -n -C 6 '"bats-helpers"|bats_helpers|bats-helpers' \
.vortex/tooling/package.json .vortex/tooling/package-lock.json \
.vortex/tooling/yarn.lock .vortex/tooling/pnpm-lock.yaml \
.github .vortex 2>/dev/null | head -n 160 || true
printf '%s\n' '--- all fetch-db-container-registry step assertions ---'
rg -n -C 5 'STEPS=|steps_run|`@docker`' \
.vortex/tooling/tests/unit/fetch-db-container-registry.bats || true
printf '%s\n' '--- login script command shape ---'
sed -n '1,180p' .vortex/tooling/src/vortex-login-container-registry
printf '%s\n' '--- repository documentation for steps_run ---'
rg -n -C 8 'steps_run|mock_set_side_effect|mock_command' \
README.md .vortex/docs .vortex/tooling 2>/dev/null | head -n 240 || trueRepository: drevops/vortex
Length of output: 29946
🌐 Web query:
Find the steps_run, mock_command, and mock_set_side_effectimplementations indrevops/bats-helpersversion 1.6.0, including the expectedSTEPS syntax for asserting mocked command invocations.
💡 Result:
In drevops/bats-helpers version 1.6.0, the Step Runner is a tool used to process sequences of string and command assertions [1][2]. The steps_run function (referenced as the process for running steps) is used within the Step Runner to process assertions [1][3]. It typically works in conjunction with process_steps, which facilitates the setup phase of mocks and the subsequent assertion phase of testing [3]. The mocking functions are defined as follows: mock_command Arguments: command_name Description: Mocks the provided command and returns the path to the mock [1][2][3]. mock_set_side_effect Arguments: mock, code, [call_index] Description: Sets shell code (a side effect) to execute when the mock is called [1][2][3]. The side effect is executed in the context of the mock, runs after output generation but before the exit status is returned, and can be chained using; or && [1][3]. STEPS Syntax for Mocked Commands The expected syntax for asserting mocked command invocations within the Step Runner is: @ [ |*] # <mock_status> [ # <mock_output> [ # <mock_side_effect> ]] In this syntax: - <mock_status> is the required exit status. - <mock_output> is the optional output. - <mock_side_effect> is the optional Bash code to be executed when the mock is called [1][3]. The side effect is specifically useful for simulating complex behaviors, such as logging, setting environment variables, or creating files and directories during the test execution [1][2].
Citations:
- 1: https://github.com/drevops/bats-helpers
- 2: https://registry.npmjs.org/@drevops/bats-helpers
- 3: https://registry.npmjs.org/%40drevops%2Fbats-helpers
🌐 Web query:
site:github.com/drevops/bats-helpers "steps_run \"setup\"" "mock_set_side_effect"
💡 Result:
The functions mentioned, including steps_run (specifically in the context of the process_steps wrapper), setup, and mock_set_side_effect, are part of the drevops/bats-helpers library, which is a collection of utilities for BATS (Bash Automated Testing System) [1]. In this framework, these components are used to mock command-line programs and verify their behavior during testing [1]. 1. setup: When using the Step Runner functionality, you typically initialize the mock environment by calling process_steps "setup" [1]. This phase creates the necessary mocks and prepares the testing environment [1]. 2. mock_set_side_effect: This function is used to define custom Bash code that should be executed whenever a specific mocked command is called [1]. - Syntax: mock_set_side_effect <mock> <code> [call_index] [1]. - Usage: It is highly effective for simulating complex command behaviors, such as creating files or directories, modifying environment variables, or logging activity [1]. - Execution: The provided code runs in the context of the mock, executing after the mock's output is generated but before it returns its exit status [1]. Multiple commands can be chained using standard shell operators like ; or && [1]. When defining steps for the Step Runner, the syntax for a mocked command assertion allows you to include the side effect directly in the step definition: @ [ |*] # <mock_status> [ # <mock_output> [ # <mock_side_effect> ]] [1].
Citations:
Assert the default registry and image in the Docker call log.
The fixed echo 'pulled from docker.io' side effect does not validate Docker arguments. Use STEPS with steps_run "setup" and steps_run "assert" to assert @docker pull docker.io/myorg/myapp (and the login target docker.io).
🤖 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 @.vortex/tooling/tests/unit/fetch-db-container-registry.bats around lines 124
- 125, The fetch-db-container-registry test currently only stubs Docker output
and does not verify the actual registry/image arguments. Update the unit test
around the mock_docker setup to use STEPS with steps_run "setup" and steps_run
"assert", and assert the Docker call log includes the login target docker.io and
the pull command `@docker` pull docker.io/myorg/myapp. Keep the existing test flow
focused on the fetch-db-container-registry behavior while replacing the fixed
side effect with argument validation.
There was a problem hiding this comment.
Fixed in f1735e9, though with a lighter change than suggested. The gap was real: the test is named for the default registry but nothing verified that docker.io actually reached the pull, since the registry never appears in the script output. It now asserts the recorded arguments directly with assert_string_contains on mock_get_call_args for call 3, checking 'pull docker.io/myorg/myapp'. That covers the substance without converting the test to STEPS - the rest of this file uses the mock_set_side_effect style, and a partial conversion would leave two idioms in one file for no extra coverage. The login target is not asserted because the login runs in vortex-login-container-registry, which this test does not exercise; login-container-registry.bats covers it.
There was a problem hiding this comment.
@AlexSkrypnyk, acknowledged. The direct mock_get_call_args assertion verifies the default-registry pull without introducing a second test idiom in this file.
The login-target assertion does not belong in this test. vortex-login-container-registry owns that Docker invocation, and login-container-registry.bats covers it.
✏️ Learnings added
Learnt from: AlexSkrypnyk
Repo: drevops/vortex PR: 2894
File: .vortex/tooling/tests/unit/fetch-db-container-registry.bats:124-125
Timestamp: 2026-08-05T08:25:32.115Z
Learning: In `.vortex/tooling/tests/unit/fetch-db-container-registry.bats`, default-registry coverage should assert the Docker pull arguments with `mock_get_call_args`; the fetch script invokes `vortex-login-container-registry` separately, so Docker login arguments belong in `.vortex/tooling/tests/unit/login-container-registry.bats`.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
You are interacting with an AI system.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2894 +/- ##
==========================================
- Coverage 87.05% 86.63% -0.43%
==========================================
Files 101 94 -7
Lines 4820 4661 -159
Branches 47 3 -44
==========================================
- Hits 4196 4038 -158
+ Misses 624 623 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
📖 Documentation preview for this pull request has been deployed to Netlify: https://6a73b7e673ac8b3c39a230ff--vortex-docs.netlify.app This preview is rebuilt on every commit and is not the production documentation site. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.vortex/tooling/tests/unit/fetch-db-container-registry.bats (1)
13-19: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd assertions to verify recorded Docker commands in each test scenario.
These tests configure side effects by call index but do not verify the actual commands. A missing login or pull operation can consume a different side effect and still return success.
- Lines 13–19: Assert the login target and
pull registry.example.com/myorg/myapp.- Lines 73–75: Assert
pull registry.example.com/myorg/base.- Lines 98–102: Assert the login and pull calls for
registry.example.com/myorg/myapp.- Line 114: Supplement the fetch-message assertion with the recorded pull assertion.
- Lines 152–153: Assert the indexed pull target
registry.example.com/myorg/migration-db.🤖 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 @.vortex/tooling/tests/unit/fetch-db-container-registry.bats around lines 13 - 19, Verify the recorded Docker commands in each scenario so configured side effects cannot mask missing operations. In .vortex/tooling/tests/unit/fetch-db-container-registry.bats at lines 13-19, assert the login target and pull of registry.example.com/myorg/myapp; at lines 73-75, assert the pull of registry.example.com/myorg/base; at lines 98-102, assert the login and pull for registry.example.com/myorg/myapp; at line 114, retain the fetch-message assertion and add the recorded pull assertion; and at lines 152-153, assert the indexed pull target registry.example.com/myorg/migration-db.
🤖 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 @.vortex/tooling/tests/unit/fetch-db-container-registry.bats:
- Around line 13-19: Verify the recorded Docker commands in each scenario so
configured side effects cannot mask missing operations. In
.vortex/tooling/tests/unit/fetch-db-container-registry.bats at lines 13-19,
assert the login target and pull of registry.example.com/myorg/myapp; at lines
73-75, assert the pull of registry.example.com/myorg/base; at lines 98-102,
assert the login and pull for registry.example.com/myorg/myapp; at line 114,
retain the fetch-message assertion and add the recorded pull assertion; and at
lines 152-153, assert the indexed pull target
registry.example.com/myorg/migration-db.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a3e39493-bcca-4210-b8a8-4d754889de81
📒 Files selected for processing (1)
.vortex/tooling/tests/unit/fetch-db-container-registry.bats
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
2 similar comments
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
AlexSkrypnyk
added
the
Needs review
AlexSkrypnyk
changed the title
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
2 similar comments
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Renamed deprecated helpers to their current names.
…tests. The registry login script is invoked by its own path rather than through PATH, so it runs for real and its 'docker login' is a mocked call in its own right. The S3 response is set on a pre-created mock because a step string is split with 'read' and cannot carry a newline.
The step output field expands backslash escapes again, so the two-line response no longer needs a pre-created mock.
…rget. The existing-host test now names and asserts the fetch that follows, and the default-registry test checks the pull target where the resolved registry is the only evidence of it.
Replaces the development branch reference now that the release the suite was validated against is on the registry.
14e2538 to
6de1dd9
Compare
|
Code coverage (threshold: 90%) Per-class coverage |
This comment has been minimized.
This comment has been minimized.
2 similar comments
This comment has been minimized.
This comment has been minimized.
|
Code coverage (threshold: 90%) Per-class coverage |
1 participant
Summary
This upgrades the BATS suite in
.vortex/tooling/tests/frombats-helpers1.6.0 to 2.0.0 and migrates every call site off the functions and variables that release deprecates. It touches 25 files, renaming 290 deprecated call sites, moving three environment variables onto theBATS_HELPERS_prefix, and correcting four container registry tests whose mocking assumptions turned out to be wrong under the stricter defaults 2.0 introduces. The full suite passes 309/309 with zero deprecation notices, matching a 1.6.0 baseline also run at 309/309.The suite was tracked against the unreleased development branch throughout 2.0's stabilisation, which is where the upstream fixes listed below came from. It now pins the published release.
Changes
.vortex/tooling/package.jsonfromnpm:@drevops/bats-helpers@^1.5.1tonpm:@drevops/bats-helpers@^2.0and migrated 290 deprecated call sites across 25 files:run_stepstosteps_run(277 sites),assert_containstoassert_string_contains(7 sites) andassert_not_containstoassert_string_not_contains(6 sites) - both of which also swap their arguments to haystack-first -setup_mocktomock_setup, andassert_not_git_repotoassert_git_not_repo._helper.bashonto theBATS_HELPERS_prefix:ASSERT_DIR_EXCLUDE,RUN_STEPS_DEBUG, andBATS_FIXTURE_EXPORT_CODEBASE_ENABLED.fetch-db-container-registry.batsto the new strict-mock default, in which a mock carrying indexed responses but no default response rejects any call its expectations do not cover.yarn.lockto the published2.0.0tarball with its integrity hash.Verification
ahoy lint-scriptspasses.Findings
fetch-db-container-registrytests mocked the login script by path viamock_command "./.vortex/tooling/src/vortex-login-container-registry", but the script invokes it as"$(dirname "${BASH_SOURCE[0]}")/vortex-login-container-registry"- a direct path rather than a PATH lookup - so the mock never intercepted it. The real login script always ran, and itsdocker loginlanded as call 2 while the tests assumed call 2 was the pull. The dead mock lines are removed and the expectations now match the realinspect->login->pullsequence.image_expanded_successfullyonly becomes1through thedb.tarbranch, so without an expanded archive the script logs in and pulls regardless of the image being present. Nothing in the test required the fetch path, so the misleading name went unnoticed. It is now named "Fetch image when it exists on host and no archive exists" and asserts the fetch it triggers. That records the current behaviour rather than endorsing it -vortex-fetch-db-container-registryis unchanged here, and whether it should short-circuit when the image is already on the host is tracked separately in Database image is pulled from the registry even when it already exists on the host #2896.docker.ioreached the pull, because the registry appears in the pull target and nowhere in the script output. It now asserts the recorded arguments of the pull call.BATS_HELPERS_prefix, and drevops/bats-helpers#209 documented the mock behaviour changes that previously reached consumers with no notice and restored multi-line responses in command steps. The suite also surfaced a shared-state bug upstream fixed by anchoring the mock directory to the per-test sandbox, which had been making the suite fail non-deterministically at full-suite scale while every file passed in isolation.Before / After
Summary by CodeRabbit
Tests
Chores