fix(spec): add _clwrc test and remove stale SessionEnd tests - #1212
Conversation
- Add spec/fish/_clwrc_function_test.fish for new _clwrc_function - Remove SessionEnd hook tests from pushover_spec.sh (hook deleted) Entire-Checkpoint: 275a1e436041
|
You do not have enough credits to review this pull request. Please purchase more credits to continue. |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on improving test suite hygiene by introducing new tests for a recently added Fish shell function and removing obsolete tests for a deleted hook. The changes ensure that the test suite accurately reflects the current codebase and prevents failures from irrelevant tests. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
📝 WalkthroughWalkthroughAdded a new Fish test for the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
Mesa DescriptionTL;DRFix shell test failures by adding a test for the What changed?
Description generated by Mesa. Update settings |
There was a problem hiding this comment.
Pull request overview
This PR updates the repository’s test suite to align with recent hook/function changes: it adds missing fish function coverage for _clwrc_function and removes now-invalid Pushover SessionEnd hook tests after the hook was deleted.
Changes:
- Add
spec/fish/_clwrc_function_test.fishto satisfy fish function coverage requirements for_clwrc_function. - Remove stale
SessionEndhook tests fromspec/pushover_spec.sh(script no longer handlesSessionEnd).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| spec/pushover_spec.sh | Removes SessionEnd tests that no longer match config/claude/pushover.sh behavior. |
| spec/fish/_clwrc_function_test.fish | Adds fishtape tests verifying _clwrc_function invokes node ... remote-control --worktree and passes through args. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a new test file, spec/fish/_clwrc_function_test.fish, and removes obsolete tests from spec/pushover_spec.sh. I have provided feedback on the new test file to improve test isolation and maintainability. The removal of stale tests is appropriate.
| set log1 (mktemp) | ||
| set fake_cli (mktemp) | ||
|
|
||
| function which; echo $fake_cli; end | ||
| function realpath; echo $argv[1]; end | ||
| function node; echo "node" $argv >> $log1; end | ||
|
|
||
| _clwrc_function | ||
|
|
||
| @test "calls node directly (not claude symlink)" (grep -c "^node" $log1) -ge 1 | ||
| @test "passes remote-control subcommand" (grep -c "remote-control" $log1) -ge 1 | ||
| @test "passes --worktree flag" (grep -c -- "--worktree" $log1) -ge 1 | ||
|
|
||
| # ── with args: passes through extra args ────────────────────── | ||
| set log2 (mktemp) | ||
| function node; echo "node" $argv >> $log2; end | ||
|
|
||
| _clwrc_function --name mysession | ||
|
|
||
| @test "passes extra args through" (grep -c -- "--name" $log2) -ge 1 | ||
| @test "still includes remote-control with args" (grep -c "remote-control" $log2) -ge 1 | ||
| @test "still includes --worktree with args" (grep -c -- "--worktree" $log2) -ge 1 | ||
|
|
||
| rm -f $log1 $log2 $fake_cli |
There was a problem hiding this comment.
The two test cases in this file are not independent. The second test case relies on mock functions (which, realpath) defined for the first one, and redefines the node mock function. This can make the tests brittle and harder to maintain.
To improve test isolation and clarity, you could define the mock functions once and reuse them for both test cases, clearing the log file between runs. This avoids redefining functions and using separate log files.
# Use a single set of mocks and a single log file for all tests
set logfile (mktemp)
set fake_cli (mktemp)
# Mock external commands
function which; echo $fake_cli; end
function realpath; echo $argv[1]; end
function node; echo "node" $argv >> $logfile; end
# ── basic: resolves symlink and runs node with remote-control --worktree ──
_clwrc_function
@test "calls node directly (not claude symlink)" (grep -c "^node" $logfile) -ge 1
@test "passes remote-control subcommand" (grep -c "remote-control" $logfile) -ge 1
@test "passes --worktree flag" (grep -c -- "--worktree" $logfile) -ge 1
# ── with args: passes through extra args ──────────────────────
: > $logfile # Truncate log for the next test case
_clwrc_function --name mysession
@test "passes extra args through" (grep -c -- "--name" $logfile) -ge 1
@test "still includes remote-control with args" (grep -c "remote-control" $logfile) -ge 1
@test "still includes --worktree with args" (grep -c -- "--worktree" $logfile) -ge 1
# Clean up temporary files
rm -f $logfile $fake_cli
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
spec/fish/_clwrc_function_test.fish (1)
24-26: Strengthen passthrough assertion for--namevalue.Current checks confirm
--nameexists, but they don’t confirm the paired value (mysession) is forwarded. Tightening this makes the arg-pass test more robust.Suggested assertion refinement
-@test "passes extra args through" (grep -c -- "--name" $log2) -ge 1 +@test "passes --name value through" (grep -c -- "--name mysession" $log2) -ge 1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/fish/_clwrc_function_test.fish` around lines 24 - 26, The "passes extra args through" test currently only verifies the presence of "--name" but not its value; update that test in spec/fish/_clwrc_function_test.fish (the `@test` "passes extra args through" block) to assert the paired value "mysession" is forwarded as well by grepping for the combined token "--name mysession" (or an equivalent regex that ensures "--name" is immediately followed by "mysession"), so the assertion confirms both flag and value are passed through.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@spec/fish/_clwrc_function_test.fish`:
- Around line 8-10: The test stubs for which, realpath, and node are left
defined and can leak into other specs; update the cleanup block in
_clwrc_function_test.fish (and similarly in _clrc_function_test.fish) to erase
those function definitions by invoking functions --erase for which, realpath,
and node before the existing rm -f temp-file cleanup so the stubs are removed
from the shell namespace and won't affect subsequent tests.
---
Nitpick comments:
In `@spec/fish/_clwrc_function_test.fish`:
- Around line 24-26: The "passes extra args through" test currently only
verifies the presence of "--name" but not its value; update that test in
spec/fish/_clwrc_function_test.fish (the `@test` "passes extra args through"
block) to assert the paired value "mysession" is forwarded as well by grepping
for the combined token "--name mysession" (or an equivalent regex that ensures
"--name" is immediately followed by "mysession"), so the assertion confirms both
flag and value are passed through.
🪄 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: db9796a9-6840-435f-9576-74b32b0c6e63
📒 Files selected for processing (2)
spec/fish/_clwrc_function_test.fishspec/pushover_spec.sh
💤 Files with no reviewable changes (1)
- spec/pushover_spec.sh
| function which; echo $fake_cli; end | ||
| function realpath; echo $argv[1]; end | ||
| function node; echo "node" $argv >> $log1; end |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether fish test infrastructure can be affected by leaked function stubs.
# 1) Inspect shell-test/fish runner wiring for process/session reuse clues.
rg -n -C3 'shell-test|fish|fishtape|@test|source spec/fish' Makefile
# 2) Inspect fish specs for global stubs and whether they are erased.
rg -n -C2 'function (which|realpath|node)\b|functions --erase (which|realpath|node)\b' spec/fishRepository: shunkakinoki/dotfiles
Length of output: 4940
🏁 Script executed:
# 1) List all test files in spec/fish to understand scope
find spec/fish -name '*_test.fish' -type f | sort
# 2) Check the cleanup sections in the mentioned files
head -n 35 spec/fish/_clwrc_function_test.fish | tail -n +20
# 3) Look for any other function definitions that might pollute
rg -n 'function ' spec/fish --type fish | grep -v '^[^:]*:[0-9]*:function (which|realpath|node)'Repository: shunkakinoki/dotfiles
Length of output: 17321
🏁 Script executed:
# Check _clwrc_function_test.fish lines 18-30
echo "=== _clwrc_function_test.fish ===" && sed -n '18,30p' spec/fish/_clwrc_function_test.fish
# Check _clrc_function_test.fish lines 17-30
echo "=== _clrc_function_test.fish ===" && sed -n '17,30p' spec/fish/_clrc_function_test.fish
# Count total lines in each file
echo "=== Line counts ===" && wc -l spec/fish/_clwrc_function_test.fish spec/fish/_clrc_function_test.fishRepository: shunkakinoki/dotfiles
Length of output: 1022
🏁 Script executed:
# List test files in order to see what comes after _clwrc_function_test.fish
find spec/fish -name '*_test.fish' -type f | sort
# Search for calls to which, realpath, node (not definitions) in test files
echo "=== Looking for calls to which/realpath/node functions ==="
rg -n 'which|realpath|node' spec/fish/*.fish --type fish | grep -v '^[^:]*:[0-9]*:function (which|realpath|node)' | grep -v '^[^:]*:[0-9]*:`@test`' | head -30Repository: shunkakinoki/dotfiles
Length of output: 3341
Add function cleanup to prevent stubs leaking into subsequent specs.
Lines 8–10 override which, realpath, and node but only temp files are erased at cleanup (line 28). Since fishtape runs all test files sequentially in a single process, these function stubs persist and pollute the namespace for any specs that run afterward.
Add functions --erase which realpath node at the cleanup section before the rm -f command to ensure stubs don't affect tests in other files.
Suggested fix
`@test` "still includes remote-control with args" (grep -c "remote-control" $log2) -ge 1
`@test` "still includes --worktree with args" (grep -c -- "--worktree" $log2) -ge 1
+functions --erase which realpath node
rm -f $log1 $log2 $fake_cliThe same issue exists in spec/fish/_clrc_function_test.fish (lines 8–10, cleanup at line 26).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@spec/fish/_clwrc_function_test.fish` around lines 8 - 10, The test stubs for
which, realpath, and node are left defined and can leak into other specs; update
the cleanup block in _clwrc_function_test.fish (and similarly in
_clrc_function_test.fish) to erase those function definitions by invoking
functions --erase for which, realpath, and node before the existing rm -f
temp-file cleanup so the stubs are removed from the shell namespace and won't
affect subsequent tests.
- Pass config arg to make-updater import in services/default.nix - Format spec/pushover_spec.sh (trailing blank line) Entire-Checkpoint: d2aa9c34776d
Add --max-time 5 and --connect-timeout 3 to curl call so the Stop hook doesn't hang waiting for the Pushover API when the network is slow or unreachable, causing 57s+ delays. Entire-Checkpoint: 472f8aeb3785
Summary
spec/fish/_clwrc_function_test.fishto satisfy the fish function coverage spec for the recently added_clwrc_functionSessionEndhook tests fromspec/pushover_spec.sh— the hook was deleted so the tests were failingTest plan
make shell-testpasses: 810 examples, 0 failures🤖 Generated with Claude Code
Summary by cubic
Add Fish spec
spec/fish/_clwrc_function_test.fishto verify_clwrc_functioncallsnodewithremote-control --worktreeand forwards extra args, and remove staleSessionEndtests inspec/pushover_spec.shto fixmake shell-testfailures. Add curl timeouts inconfig/claude/pushover.shto prevent hanging stop hooks, and passconfigto themake-updaterimport inhome-manager/services/default.nixto fix service wiring.Written for commit 62d9f6e. Summary will update on new commits.