[#106] Included means for deterministic test setup. #113
Conversation
📝 WalkthroughWalkthroughAdds deterministic test-setup utilities and extends mocking workflow: Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Test
participant MockCreate as mock_create
participant SetCmd as mock_set_command
participant Chroot as mock_chroot
participant PathOps as path_override / path_rm
participant Teardown as mock_teardown
Note over Test,MockCreate: Create mock (optionally name command)
Test->>MockCreate: call mock_create [optional: command name]
alt command name provided
MockCreate->>SetCmd: create symlink name -> mock
SetCmd-->>MockCreate: returns command path
MockCreate-->>Test: stdout = mocked command path
else no command name
MockCreate-->>Test: stdout = mock path
end
Note over Test,Chroot: Build deterministic command dir
Test->>Chroot: call mock_chroot [commands?]
Chroot-->>Test: directory with symlinked commands (mock dir)
Note over Test,PathOps: Compose PATH with mock dir(s)
Test->>PathOps: call path_override(mock dir, $PATH)
PathOps-->>Test: PATH with mock dir prefixed
Test->>PathOps: call path_rm(PATH, segment)
PathOps-->>Test: PATH without segment
Note over Test,Teardown: Cleanup artifacts
Test->>Teardown: call mock_teardown
Teardown-->>Test: removes bats-mock.$$.* artifacts
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (3)
🧰 Additional context used📓 Path-based instructions (4)tests/**/*.bats📄 CodeRabbit inference engine (CLAUDE.md)
Files:
**/*.{bash,bats}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
src/mock.bash📄 CodeRabbit inference engine (CLAUDE.md)
Files:
docs/**📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (3)📚 Learning: 2025-09-30T23:09:50.370ZApplied to files:
📚 Learning: 2025-09-30T23:09:50.370ZApplied to files:
📚 Learning: 2025-09-30T23:09:50.370ZApplied to files:
🧬 Code graph analysis (1)tests/mock.bats (4)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (43)
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/content/mocking.mdx (1)
82-90: Consider documenting the relationship between mock_command and mock_create.The
mock_commandfunction is documented separately frommock_create, but given thatmock_createnow accepts a command parameter, it would be helpful to clarify when to use each function or note that they serve similar purposes.Consider adding a note:
### `mock_command` - **Description**: Mock provided command. + :::note + This function is equivalent to calling `mock_create <command>`. + Consider using `mock_create` directly for consistency. + ::: - **Arguments**:src/mock.bash (1)
21-86: Changeexit $?toreturn $?in mock_create error handling
Replace the|| exit $?call with|| return $?so that amock_set_commandfailure returns from the function instead of terminating the entire shell.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
docs/content/mocking.mdx(2 hunks)src/mock.bash(5 hunks)tests/mock.bats(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
tests/**/*.bats
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.bats: All tests must be written as BATS files (*.bats) under tests/
Each BATS test should source tests/_test_helper.bash for setup
Files:
tests/mock.bats
**/*.{bash,bats}
📄 CodeRabbit inference engine (CLAUDE.md)
Bash and BATS files must pass shellcheck and be formatted with shfmt (npm run lint / lint-fix)
Files:
tests/mock.batssrc/mock.bash
src/mock.bash
📄 CodeRabbit inference engine (CLAUDE.md)
mock.bash must implement a mocking system that creates temporary mock executables, records calls, and supports configurable outputs/exit codes
Files:
src/mock.bash
docs/**
📄 CodeRabbit inference engine (CLAUDE.md)
Project documentation must live under docs/ and be maintained via Docusaurus
Files:
docs/content/mocking.mdx
🧠 Learnings (3)
📚 Learning: 2025-09-30T23:09:50.370Z
Learnt from: CR
PR: drevops/bats-helpers#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-30T23:09:50.370Z
Learning: Applies to src/mock.bash : mock.bash must implement a mocking system that creates temporary mock executables, records calls, and supports configurable outputs/exit codes
Applied to files:
tests/mock.batssrc/mock.bashdocs/content/mocking.mdx
📚 Learning: 2025-09-30T23:09:50.370Z
Learnt from: CR
PR: drevops/bats-helpers#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-30T23:09:50.370Z
Learning: Applies to tests/_test_helper.bash : Common test setup must reside in tests/_test_helper.bash (loading the library and configuring mocking)
Applied to files:
src/mock.bash
📚 Learning: 2025-09-30T23:09:50.370Z
Learnt from: CR
PR: drevops/bats-helpers#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-30T23:09:50.370Z
Learning: Applies to tests/**/*.bats : Each BATS test should source tests/_test_helper.bash for setup
Applied to files:
src/mock.bash
🧬 Code graph analysis (1)
tests/mock.bats (4)
src/mock.bash (5)
mock_create(21-86)mock_teardown(295-299)mock_chroot(338-363)path_override(371-388)path_rm(396-410)src/assert.string.bash (1)
assert_equal(45-52)src/assert.file.bash (3)
assert_file_exists(8-22)assert_file_not_exists(24-34)assert_dir_not_exists(46-54)src/assert.command.bash (4)
assert_success(7-14)assert_output_contains(36-45)assert_failure(16-23)assert_output_not_contains(47-56)
🔇 Additional comments (33)
src/mock.bash (5)
292-299: LGTM - Clean teardown implementation.The
mock_teardownfunction correctly cleans up all mock artifacts using the process ID to ensure only the current test's mocks are removed.
396-410: Path removal implementation is correct but complex.The function correctly handles path removal by:
- Wrapping path with colons
- Doubling colons to handle edge cases
- Removing the target path
- Cleaning up extra colons
This implementation is correct and handles edge cases well.
106-111: Clarify the absolute path handling logic.When
cmdis an absolute path (line 106), the function extracts the directory portion and uses it asmock_path, but then reconstructscmdat line 110 which would never execute since the condition at line 106 is true. This appears to be dead code.Apply this refactor to clarify the logic:
if [[ ${cmd} == /* ]]; then # Parameter expansion to get the folder portion of the command's path mock_path="${cmd%/*}" - else - cmd="${mock_path}/${cmd}" fi + + # If cmd is not absolute, prepend the mock_path + if [[ ${cmd} != /* ]]; then + cmd="${mock_path}/${cmd}" + fiLikely an incorrect or invalid review comment.
371-388: Idempotence check has a subtle bug.The function checks if
mock_pathis already in the path using the pattern:${path}:wrapping (line 383). However, this check will fail for paths at the beginning or end of PATH that don't have colons on both sides. The current implementation works because it wraps with colons, but the comment says "putting the directory... at the beginning" which might not be accurate if it's already present elsewhere.The implementation is functionally correct but could be more explicit:
# Putting the directory with the mocked commands at the beginning of the PATH # so it gets picked up first - if [[ :${path}: == *:${mock_path}:* ]]; then - echo "${path}" - else - echo "${mock_path}:${path}" + # Check if mock_path is already in path (with colon wrapping for exact match) + if [[ ":${path}:" == *":${mock_path}:"* ]]; then + # Already present, return unchanged + echo "${path}" + else + # Prepend to ensure it's found first + echo "${mock_path}:${path}" fiLikely an incorrect or invalid review comment.
412-424: Refactormock_commandto wrapmock_createand return the base mock path
mock_commandis still invoked in tests (tests/mock.bats), insrc/steps.bash, and in the documentation—rather than removing it, make it a thin wrapper aroundmock_create:-mock_command() { - mocked_command="${1?'Mocked command must be specified'}" - mock="$(mock_create)" - mock_path="${mock%/*}" - mock_file="${mock##*/}" - ln -sf "${mock_path}/${mock_file}" "${mock_path}/${mocked_command}" - echo "$mock" -} +mock_command() { + local cmd="${1?'Mocked command must be specified'}" + # Create the mock stub and capture its path + local stub + stub=$(mock_create "${cmd}") + # Return the underlying mock file path for compatibility + readlink "${stub}" +}Verify that tests,
src/steps.bash, and docs still work as expected, and update any call sites or examples if needed.tests/mock.bats (21)
92-100: LGTM - Good test coverage for enhanced mock_create.The test correctly verifies that
mock_createwith a command name creates an executable symlink with the expected name.
102-108: LGTM - Verifies symlink points to actual mock.This test appropriately confirms the symlink relationship between the command and the underlying mock file.
110-114: LGTM - Validates directory structure.The test correctly verifies that mocked commands are placed in the
bats-mock.$$.bindirectory as expected.
116-125: LGTM - Backward compatibility preserved.This test ensures that calling
mock_createwithout arguments still works as before, maintaining backward compatibility.
128-136: LGTM - Teardown test covers both unnamed and named mocks.Good test that verifies
mock_teardownremoves both types of mocks created bymock_create.
138-144: LGTM - Tests teardown of chroot directory.Correctly verifies that
mock_teardownalso removes the mock_chroot directory.
147-151: LGTM - Error handling test for path_override.Properly tests that
path_overridefails with an appropriate error message when called without required arguments.
153-160: LGTM - Basic path_override functionality.Tests the core functionality of prefixing PATH with the mock directory.
162-169: LGTM - Tests directory input handling.Verifies that
path_overrideworks correctly when given a directory instead of a file path.
171-175: LGTM - Custom path test.Tests that
path_overridecan work with custom PATH values, not just the global$PATH.
177-184: LGTM - Idempotence verification.Important test that confirms calling
path_overridemultiple times doesn't create duplicate entries.
187-191: LGTM - Error handling for path_rm.Correctly tests that
path_rmrequires an argument.
200-204: LGTM - Custom path removal test.Correctly verifies that
path_rmcan remove a directory from a custom path string.
206-210: LGTM - Idempotence test for path_rm.Good test that verifies removing a non-existent path doesn't modify the PATH.
212-220: LGTM - Tests file-to-directory extraction.Appropriately tests that
path_rmcan extract the directory from a command file path and remove it from PATH.
223-233: LGTM - Default commands chroot test.Good test that verifies
mock_chrootcreates links to a default set of commands when called without arguments.
235-240: LGTM - Chroot idempotence test.Correctly verifies that calling
mock_chrootmultiple times returns the same directory path.
242-247: LGTM - Verifies directory sharing.Important test that confirms
mock_createwith a command andmock_chrootuse the same directory, enabling them to work together.
249-258: LGTM - Custom command list test.Tests that
mock_chrootcan create a chroot with a specific list of commands.
260-264: LGTM - Error handling for missing commands.Appropriately tests that
mock_chrootfails with an error message when a requested command doesn't exist.
267-292: LGTM - Comprehensive integration test.Excellent integration test that combines multiple functions (
mock_create,mock_chroot,path_override,path_rm) to verify they work together as intended. This validates the deterministic test setup workflow.docs/content/mocking.mdx (7)
63-75: Documentation accurately reflects enhanced mock_create functionality.The updated documentation correctly describes the new optional command parameter and the different output behaviors (path to mock vs. mocked command).
128-138: LGTM - Clear documentation for mock_teardown.The documentation accurately describes the
mock_teardownfunction with appropriate usage example in ateardown()context.
140-164: Excellent addition of Deterministic Test Setup section.This new section provides clear context for the new functionality and explains the use case for negative testing. The note about directory sharing between
mock_createandmock_chrootis particularly helpful.
144-158: LGTM - Comprehensive mock_chroot documentation.The documentation clearly explains the function's purpose, arguments, return values, and provides useful examples for both default and custom command scenarios.
166-178: LGTM - Clear path_override documentation.The documentation effectively explains how
path_overrideworks and provides a practical example.
180-194: LGTM - Comprehensive path_rm documentation.Good documentation with multiple usage examples showing different use cases.
196-217: Excellent practical example demonstrating the integration.The "Testing Command Fallback" example effectively demonstrates how to combine
mock_create,mock_chroot,path_override, andpath_rmfor a real-world testing scenario. This helps users understand the practical application of these utilities.
| # Creates a symbolic link with given name to a mock program | ||
| # Globals: | ||
| # BATS_MOCK_TMPDIR | ||
| # Arguments: | ||
| # 1: Path to the mock | ||
| # 2: Command name | ||
| # Outputs: | ||
| # STDOUT: Path to the mocked command | ||
| mock_set_command() { | ||
| local mock="${1?'Mocked command must be specified'}" | ||
| local cmd="${2?'Command must be specified'}" | ||
|
|
||
| # @note: Modification to the original file: use BATS_MOCK_TMPDIR. | ||
| BATS_MOCK_TMPDIR="${BATS_MOCK_TMPDIR:-$BATS_TMPDIR}" | ||
|
|
||
| # Parameter expansion to get the folder portion of the mock's path | ||
| local mock_path="${mock%/*}/bats-mock.$$.bin" | ||
|
|
||
| if [[ ${cmd} == /* ]]; then | ||
| # Parameter expansion to get the folder portion of the command's path | ||
| mock_path="${cmd%/*}" | ||
| else | ||
| cmd="${mock_path}/${cmd}" | ||
| fi | ||
|
|
||
| # Create command stub by linking it to the mock | ||
| mkdir -p "${mock_path}" | ||
| ln -s "${mock}" "${cmd}" && echo "${cmd}" | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Validate symlink creation before returning the command path.
The function mock_set_command creates a symlink at line 115 and immediately echoes the command path. However, if the symlink creation fails but doesn't return an error (edge case), the function still outputs the command path, potentially misleading the caller.
Consider adding an explicit check:
# Create command stub by linking it to the mock
mkdir -p "${mock_path}"
- ln -s "${mock}" "${cmd}" && echo "${cmd}"
+ if ln -s "${mock}" "${cmd}"; then
+ echo "${cmd}"
+ else
+ echo "Failed to create symlink for ${cmd}" >&2
+ return 1
+ fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Creates a symbolic link with given name to a mock program | |
| # Globals: | |
| # BATS_MOCK_TMPDIR | |
| # Arguments: | |
| # 1: Path to the mock | |
| # 2: Command name | |
| # Outputs: | |
| # STDOUT: Path to the mocked command | |
| mock_set_command() { | |
| local mock="${1?'Mocked command must be specified'}" | |
| local cmd="${2?'Command must be specified'}" | |
| # @note: Modification to the original file: use BATS_MOCK_TMPDIR. | |
| BATS_MOCK_TMPDIR="${BATS_MOCK_TMPDIR:-$BATS_TMPDIR}" | |
| # Parameter expansion to get the folder portion of the mock's path | |
| local mock_path="${mock%/*}/bats-mock.$$.bin" | |
| if [[ ${cmd} == /* ]]; then | |
| # Parameter expansion to get the folder portion of the command's path | |
| mock_path="${cmd%/*}" | |
| else | |
| cmd="${mock_path}/${cmd}" | |
| fi | |
| # Create command stub by linking it to the mock | |
| mkdir -p "${mock_path}" | |
| ln -s "${mock}" "${cmd}" && echo "${cmd}" | |
| } | |
| # Creates a symbolic link with given name to a mock program | |
| # Globals: | |
| # BATS_MOCK_TMPDIR | |
| # Arguments: | |
| # 1: Path to the mock | |
| # 2: Command name | |
| # Outputs: | |
| # STDOUT: Path to the mocked command | |
| mock_set_command() { | |
| local mock="${1?'Mocked command must be specified'}" | |
| local cmd="${2?'Command must be specified'}" | |
| # @note: Modification to the original file: use BATS_MOCK_TMPDIR. | |
| BATS_MOCK_TMPDIR="${BATS_MOCK_TMPDIR:-$BATS_TMPDIR}" | |
| # Parameter expansion to get the folder portion of the mock's path | |
| local mock_path="${mock%/*}/bats-mock.$$.bin" | |
| if [[ ${cmd} == /* ]]; then | |
| # Parameter expansion to get the folder portion of the command's path | |
| mock_path="${cmd%/*}" | |
| else | |
| cmd="${mock_path}/${cmd}" | |
| fi | |
| # Create command stub by linking it to the mock | |
| mkdir -p "${mock_path}" | |
| if ln -s "${mock}" "${cmd}"; then | |
| echo "${cmd}" | |
| else | |
| echo "Failed to create symlink for ${cmd}" >&2 | |
| return 1 | |
| fi | |
| } |
🤖 Prompt for AI Agents
In src/mock.bash around lines 88 to 116, the function mock_set_command currently
runs mkdir and ln -s then immediately echoes the command path even if symlink
creation failed; update it to validate success before returning: ensure mkdir -p
succeeds, run ln -s and check its exit status (or test that the symlink exists
and points to the mock with [ -L "$cmd" ] and [ "$(readlink -f "$cmd")" =
"$(readlink -f "$mock")" ]), only echo the command path on success and return a
non-zero status on failure so callers aren’t misled.
There was a problem hiding this comment.
In my original implementation, I propagated ln error message to the user.
Reasoning:
- it's better understandable for non-english systems (mine is configured for German).
- the failed "ln" command may fail for various reasons.
But upon second thought, I think it is better to abstract the error message and tell the user what he/she did wrong. The most reasonable cause of error is the user mocks the same command twice.
So instead of "leaking" internal implementation details to the user by "ln failed" maybe we return something like "you made an error by mocking the command twice".
@test "Mock: fail mocking a command twice" {
mock_curl=$(mock_command "curl")
run --separate-stderr mock_command "curl"
# Not sure which exit code we put here
assert_failure 1
# TBD: what do we provide as feedback to the user?
assert_equal "${stderr_lines[0]}" "Error: 'curl' command already mocked"
assert_equal "${stderr_lines[2]}" "You can't mock a command twice"
}There was a problem hiding this comment.
@mh182, excellent refinement! Abstracting the error to "command already mocked" is much more user-friendly than exposing ln internals. Your reasoning about localization and clarity is spot-on.
For the implementation, I'd suggest:
- Exit code:
1is appropriate for user errors in bash functions - Error detection: Check if the symlink already exists before attempting to create it
- Error message: Your proposed message is clear; I'd simplify to a single line
Here's a suggested implementation for mock_set_command:
# Create command stub by linking it to the mock
mkdir -p "${mock_path}"
if [[ -e "${cmd}" ]]; then
echo "Error: Command '${cmd##*/}' is already mocked" >&2
return 1
fi
if ! ln -s "${mock}" "${cmd}"; then
echo "Error: Failed to create mock for '${cmd##*/}'" >&2
return 1
fi
echo "${cmd}"For your test case:
@test "Mock: fail mocking a command twice" {
mock_curl=$(mock_command "curl")
run --separate-stderr mock_command "curl"
assert_failure 1
assert_output ""
assert_line --index 0 --partial "Error: Command 'curl' is already mocked"
}This approach:
- Pre-checks for existing mocks (catches the common case early)
- Still catches other
lnfailures with a generic fallback - Keeps error messages concise and actionable
- Uses
${cmd##*/}to extract just the command name for cleaner output
🧠 Learnings used
Learnt from: CR
PR: drevops/bats-helpers#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-30T23:09:50.370Z
Learning: Applies to src/mock.bash : mock.bash must implement a mocking system that creates temporary mock executables, records calls, and supports configurable outputs/exit codes
There was a problem hiding this comment.
BTW - this test fails since mock_command uses ln -sf which IMHO we shouldn't do.
The main problem at the moment is that the mocked command is created in the same directory for all running tests. This leads to errors if tests are run in parallel. The tests "override" the existing symbolic links and the latest run wins all tests writing in the same mock object.
That's why I created a directory for each execution process $BATS_MOCK_TMPDIR.bin and created the symbolic links in that directory.
There was a problem hiding this comment.
@mh182, you've identified a critical race condition! The -f flag in ln -sf is indeed problematic for parallel execution.
Looking at the current implementation of mock_command:
mock_command() {
mocked_command="${1?'Mocked command must be specified'}"
mock="$(mock_create)"
mock_path="${mock%/*}"
mock_file="${mock##*/}"
ln -sf "${mock_path}/${mock_file}" "${mock_path}/${mocked_command}"
echo "$mock"
}The issue is that mock_command creates the symlink directly in ${mock_path} without leveraging the process-specific bats-mock.$$.bin directory pattern that mock_set_command uses.
Recommended fix:
mock_command should delegate to the existing infrastructure instead of reimplementing it:
mock_command() {
mocked_command="${1?'Mocked command must be specified'}"
mock="$(mock_create)"
mock_set_command "${mock}" "${mocked_command}"
}Or, if you need to maintain the current signature where it returns the mock path (not the command path), you could do:
mock_command() {
mocked_command="${1?'Mocked command must be specified'}"
mock="$(mock_create)"
mock_set_command "${mock}" "${mocked_command}" >/dev/null
echo "${mock}"
}This approach:
- ✅ Eliminates the
-fflag entirely - ✅ Uses the process-specific
bats-mock.$$.bindirectory viamock_set_command - ✅ Provides proper error handling (as we discussed earlier)
- ✅ Ensures parallel tests are isolated by process ID
The key insight is that mock_set_command already implements the per-process isolation pattern with ${mock%/*}/bats-mock.$$.bin, so mock_command should reuse that logic rather than bypassing it.
🧠 Learnings used
Learnt from: CR
PR: drevops/bats-helpers#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-30T23:09:50.370Z
Learning: Applies to src/mock.bash : mock.bash must implement a mocking system that creates temporary mock executables, records calls, and supports configurable outputs/exit codes
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/mock.bash (2)
418-431: Refactor mock_command to reuse mock_set_command, add locals, and quote.Avoid duplicate logic and global vars; improves clarity and shellcheck compliance.
Apply:
-mock_command() { - mocked_command="${1?'Mocked command must be specified'}" - mock="$(mock_create)" - mock_path="${mock%/*}" - mock_file="${mock##*/}" - ln -sf "${mock_path}/${mock_file}" "${mock_path}/${mocked_command}" - echo "$mock" -} +mock_command() { + local mocked_command="${1?'Mocked command must be specified'}" + local mock + mock="$(mock_create)" || return $? + mock_set_command "${mock}" "${mocked_command}" >/dev/null || return $? + echo "${mock}" +}
231-237: Tiny clarity nit: pass the computed n to mock_default_n.Avoid mixing $3 with local n; use the variable directly.
Apply:
- local n="${3-}" - n="$(mock_default_n "${mock}" "${3}")" || exit "$?" + local n="${3-}" + n="$(mock_default_n "${mock}" "${n}")" || exit "$?"
♻️ Duplicate comments (2)
tests/mock.bats (1)
195-200: Harden PATH entry assertion by colon‑wrapping output.Wrap output with colons to avoid false positives when /usr/bin is at PATH boundaries.
Apply:
@test "Mock: path_rm removes directory from PATH" { run path_rm "/usr/bin" assert_success - run echo "${output}" - assert_output_not_contains ":/usr/bin:" + # Use colon-wrapped PATH to ensure exact match + run echo ":${output}:" + assert_output_not_contains ":/usr/bin:" }src/mock.bash (1)
88-116: Strengthen mock_set_command: deterministic ln and explicit error on failure.Use command -p for ln (PATH‑independent) and emit a clear message if symlink creation fails.
Apply:
# Create command stub by linking it to the mock mkdir -p "${mock_path}" - ln -s "${mock}" "${cmd}" && echo "${cmd}" + if command -p ln -s "${mock}" "${cmd}"; then + echo "${cmd}" + else + echo "Failed to create symlink for ${cmd}" >&2 + return 1 + fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/mock.bash(5 hunks)tests/mock.bats(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
tests/**/*.bats
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.bats: All tests must be written as BATS files (*.bats) under tests/
Each BATS test should source tests/_test_helper.bash for setup
Files:
tests/mock.bats
**/*.{bash,bats}
📄 CodeRabbit inference engine (CLAUDE.md)
Bash and BATS files must pass shellcheck and be formatted with shfmt (npm run lint / lint-fix)
Files:
tests/mock.batssrc/mock.bash
src/mock.bash
📄 CodeRabbit inference engine (CLAUDE.md)
mock.bash must implement a mocking system that creates temporary mock executables, records calls, and supports configurable outputs/exit codes
Files:
src/mock.bash
🧠 Learnings (3)
📚 Learning: 2025-09-30T23:09:50.370Z
Learnt from: CR
PR: drevops/bats-helpers#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-30T23:09:50.370Z
Learning: Applies to src/mock.bash : mock.bash must implement a mocking system that creates temporary mock executables, records calls, and supports configurable outputs/exit codes
Applied to files:
tests/mock.batssrc/mock.bash
📚 Learning: 2025-09-30T23:09:50.370Z
Learnt from: CR
PR: drevops/bats-helpers#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-30T23:09:50.370Z
Learning: Applies to tests/_test_helper.bash : Common test setup must reside in tests/_test_helper.bash (loading the library and configuring mocking)
Applied to files:
src/mock.bash
📚 Learning: 2025-09-30T23:09:50.370Z
Learnt from: CR
PR: drevops/bats-helpers#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-30T23:09:50.370Z
Learning: Applies to tests/**/*.bats : Each BATS test should source tests/_test_helper.bash for setup
Applied to files:
src/mock.bash
🧬 Code graph analysis (1)
tests/mock.bats (4)
src/mock.bash (5)
mock_create(21-86)mock_teardown(295-299)mock_chroot(338-369)path_override(377-394)path_rm(402-416)src/assert.string.bash (1)
assert_equal(45-52)src/assert.file.bash (3)
assert_file_exists(8-22)assert_file_not_exists(24-34)assert_dir_not_exists(46-54)src/assert.command.bash (4)
assert_success(7-14)assert_output_contains(36-45)assert_failure(16-23)assert_output_not_contains(47-56)
🔇 Additional comments (5)
tests/mock.bats (4)
93-116: LGTM: solid coverage for mock_create with command name.The basename, exec bit, and bin-dir placement assertions look correct and portable.
129-146: Teardown behavior validated.Good checks for removing both mock files and mock_chroot dir.
149-177: Argument‑validation and PATH prefixing tests look good.Required-arg failure and idempotence are well covered.
269-294: Nice end‑to‑end integration.Combining path_override, path_rm, and mock_chroot validates the intended workflow.
src/mock.bash (1)
371-416: Ensure Bash scripts pass shellcheck and shfmt
shellcheck shows no issues; shfmt isn’t installed—install it or manually verify formatting by running:shfmt -d -i 2 -bn -sr -ci src/mock.bash tests/mock.bats
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #113 +/- ##
==========================================
+ Coverage 94.91% 95.03% +0.12%
==========================================
Files 12 12
Lines 511 564 +53
==========================================
+ Hits 485 536 +51
- Misses 26 28 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/mock.bash (1)
30-30: Quote the glob when counting existing mocks.The
findpattern is still unquoted, so shells with matching files in CWD will expand it and corrupt the mock index. Please quote the pattern (and consider silencing find errors) to keep counts deterministic.Apply:
- index="$(find "${BATS_MOCK_TMPDIR}" -name bats-mock.$$.* | wc -l | tr -d ' ')" + index="$(find "${BATS_MOCK_TMPDIR}" -name "bats-mock.$$.*" 2>/dev/null | wc -l | tr -d ' ')"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/mock.bash(5 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/mock.bash
📄 CodeRabbit inference engine (CLAUDE.md)
mock.bash must implement a mocking system that creates temporary mock executables, records calls, and supports configurable outputs/exit codes
Files:
src/mock.bash
**/*.{bash,bats}
📄 CodeRabbit inference engine (CLAUDE.md)
Bash and BATS files must pass shellcheck and be formatted with shfmt (npm run lint / lint-fix)
Files:
src/mock.bash
🧠 Learnings (3)
📚 Learning: 2025-09-30T23:09:50.370Z
Learnt from: CR
PR: drevops/bats-helpers#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-30T23:09:50.370Z
Learning: Applies to src/mock.bash : mock.bash must implement a mocking system that creates temporary mock executables, records calls, and supports configurable outputs/exit codes
Applied to files:
src/mock.bash
📚 Learning: 2025-09-30T23:09:50.370Z
Learnt from: CR
PR: drevops/bats-helpers#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-30T23:09:50.370Z
Learning: Applies to tests/_test_helper.bash : Common test setup must reside in tests/_test_helper.bash (loading the library and configuring mocking)
Applied to files:
src/mock.bash
📚 Learning: 2025-09-30T23:09:50.370Z
Learnt from: CR
PR: drevops/bats-helpers#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-30T23:09:50.370Z
Learning: Applies to tests/**/*.bats : Each BATS test should source tests/_test_helper.bash for setup
Applied to files:
src/mock.bash
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test-shell (ubuntu-latest)
| if ! target=$(command -v "$c" 2>&1); then | ||
| if ${customized_chroot}; then | ||
| echo "$c: command not found" && exit 1 | ||
| fi | ||
| else | ||
| if ! error_msg=$(command -p ln -s "${target}" "${chroot_path}/${c}" 2>&1); then | ||
| if ${customized_chroot}; then | ||
| echo "${error_msg}" && exit 1 | ||
| fi | ||
| fi |
There was a problem hiding this comment.
Make mock_chroot idempotent and send errors to stderr.
With a custom command list, rerunning mock_chroot hits “File exists” and exits because ln -s isn’t forced. The emitted diagnostics also go to stdout despite the contract promising stderr. Use ln -sfn (or remove/overwrite) and send failures to >&2 so repeated invocations stay deterministic and error messaging matches the documented behavior.
🤖 Prompt for AI Agents
In src/mock.bash around lines 360 to 369, the mock_chroot routine is not
idempotent and prints errors to stdout; change the behavior so that
missing-command and ln failures write to stderr (use >&2) and make the symlink
creation idempotent by using a force/replace option (ln -sfn or remove existing
link before linking) so repeated invocations don’t error with “File exists”;
preserve the existing conditional exit behavior for customized_chroot while
directing all diagnostic output to stderr.
There was a problem hiding this comment.
It would be helpful if we could split this PR in multiple PRs. Why not having a commit for each feature:
- PATH utilities with
path_rmandpath_override(btw. looking at my code after a couple of months, I would rather call itpath_prefixinstead ofpath_override). - Changes to the
mock_commandso we fix possible errors in the current implementation (theln -sfdescribed above). - Provide means for easy registration of mocked commands
- Deterministic test setup
There was a problem hiding this comment.
@mh182, that's an excellent suggestion! Breaking this PR into smaller, focused PRs would indeed make the review process cleaner and the git history more meaningful.
The proposed split structure makes a lot of sense:
- PATH utilities (
path_rmandpath_prefix) - foundational helpers - Mock command fixes (the
ln -sfidempotency issue) - bug fixes to existing functionality - Easy mock registration - convenience layer
- Deterministic test setup (
mock_chroot) - the main feature
This approach allows each piece to be reviewed, tested, and merged independently, making it easier to track changes and potentially revert specific features if needed.
Also noted your preference for path_prefix over path_override - that's a clearer name that better describes what the function actually does (prefixing a path to an existing PATH string).
Would you like me to help identify the specific code sections that belong to each proposed PR, or will you be restructuring this yourself?
🧠 Learnings used
Learnt from: CR
PR: drevops/bats-helpers#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-30T23:09:50.370Z
Learning: Applies to src/mock.bash : mock.bash must implement a mocking system that creates temporary mock executables, records calls, and supports configurable outputs/exit codes
AlexSkrypnyk
force-pushed
the
feature/106-deterministic-test-setup
branch
from
17cca35 to
eaa2086
Compare
|
@mh182 |
|
As mentioned above, the PR is too big for me to work around everyting. Could we split it up in digestible chunks? |
|
@mh182 I do not have a capacity to split this into multiple PRs and handle each specific case is-depth. There are 3 options I see:
What I cannot have is a dangling PR without a resolution, so I will leave this till November 2025 to discuss on options and will action the option decided. Thank you for your contribution. |
|
If you don't mind, I split this PR in multiple smaller PRs: one for the Reasoning: the changes we need to do for I don't think we should have two ways of mocking a command: If we want to keep backwards compatibility, we are forced to use I will create a PR with |
Closes #106
Summary by CodeRabbit
New Features
Documentation
Tests