Improve test assertions using explicit have_received checks #854
Conversation
Replace indirect call_count counter patterns with explicit RSpec spy assertions for verifying method invocations. This makes tests more self-documenting and robust. Changes: - Use `have_received(:capture3).with(hook_command)` to explicitly verify hook was called - Use `not_to have_received` to explicitly verify hook was skipped - Remove call_count variables and conditional logic based on counts - Simplify mock setup by using direct return values where possible Benefits: - Tests explicitly state what is being verified (hook called/skipped) - More robust to implementation changes - Clearer failure messages 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
WalkthroughTests for precompile-hook behavior were refactored to replace manual call-count tracking with explicit Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related issues
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (7)📓 Common learnings📚 Learning: 2025-11-25T05:14:43.766ZApplied to files:
📚 Learning: 2025-11-25T05:14:43.766ZApplied to files:
📚 Learning: 2024-10-09T10:52:40.920ZApplied to files:
📚 Learning: 2025-11-25T05:14:43.766ZApplied to files:
📚 Learning: 2025-11-25T05:14:43.766ZApplied to files:
📚 Learning: 2024-10-09T10:50:16.512ZApplied to files:
⏰ 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). (20)
🔇 Additional comments (2)
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 |
Code ReviewOverviewThis PR improves test assertions by replacing indirect ✅ Strengths
🔍 Observations & Suggestions1. Inconsistent RefactoringSeveral tests still use the
Recommendation: For consistency and to maximize the benefits of this refactor, consider updating these remaining tests to use explicit 2. Minor: Assertion SpecificityIn test at line 82-96 ("does not run precompile_hook when not configured"), the assertion: expect(Open3).to have_received(:capture3).onceThis verifies webpack was called but doesn't prove the hook wasn't called with a specific command. Consider adding a more explicit negative assertion like the one in the skip test. 3. Test CoverageThe changes don't affect test coverage - all existing scenarios remain tested. The refactoring is purely improving assertion quality. ✅ 🔒 Security & Performance
📋 Code Quality
🎯 Recommendations
Final VerdictApproved ✅ This is a high-quality refactoring that improves test maintainability without changing behavior. The code is more readable, the assertions are more explicit, and future developers will have an easier time understanding and modifying these tests. Great work on identifying and addressing this testing pattern improvement! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
spec/shakapacker/compiler_spec.rb (3)
112-133: Consider refactoring to use the new explicit assertion pattern.This test still uses the
call_countpattern (lines 120-128) and lacks explicit verification of what was called—it only checks thatcompilereturns true. This is inconsistent with the PR objective to replace call_count with explicithave_receivedchecks.Consider refactoring to match the pattern used in lines 59-80:
it "handles hook with both stdout and stderr" do mocked_strategy = spy("Strategy") allow(mocked_strategy).to receive(:stale?).and_return(true) allow(Shakapacker.compiler).to receive(:strategy).and_return(mocked_strategy) hook_status = OpenStruct.new(success?: true, exitstatus: 0) webpack_status = OpenStruct.new(success?: true) + hook_command = "bin/verbose-hook" - call_count = 0 - allow(Open3).to receive(:capture3) do |*args| - call_count += 1 - if call_count == 1 + allow(Open3).to receive(:capture3) do |env, *args| + if args[0] == hook_command ["Standard output", "Warning message", hook_status] else ["", "", webpack_status] end end - allow(Shakapacker.config).to receive(:precompile_hook).and_return("bin/verbose-hook") + allow(Shakapacker.config).to receive(:precompile_hook).and_return(hook_command) expect(Shakapacker.compiler.compile).to be true + expect(Open3).to have_received(:capture3).with(hash_including, hook_command, hash_including).once end
168-192: Consider refactoring to use the new explicit assertion pattern.This test still uses the
call_countpattern and lacks explicit verification. For consistency with the PR objective, consider refactoring to use the hook_command dispatch pattern.Note: Since this command includes arguments (
--arg1 --arg2), you may need a more flexible verification approach. For example, you could verify the command executable was called:expect(Open3).to have_received(:capture3) do |env, *args| expect(args[0]).to eq("bin/my script") endOr, if the implementation passes the command as a single string, the existing hash_including pattern might work. Based on learnings, run the corresponding RSpec test to verify the behavior.
194-219: Consider refactoring to use the new explicit assertion pattern.This test still uses the
call_countpattern (lines 202-210) and doesn't explicitly verify that the hook was called. For consistency with the PR objective, refactor to use thehook_commanddispatch pattern similar to lines 59-80.
🧹 Nitpick comments (2)
spec/shakapacker/compiler_spec.rb (2)
241-272: Consider refactoring for consistency, though verification is present.While this test does verify the hook was called (by checking
captured_args), it still uses thecall_countpattern. For consistency with the PR objective and other refactored tests, consider using the hook_command dispatch pattern.You could refactor to capture args when detecting the hook command:
- call_count = 0 captured_args = [] allow(Open3).to receive(:capture3) do |env, *args| - call_count += 1 - captured_args << args if call_count == 1 - if call_count == 1 + if args[0] == "bin/prepare" + captured_args << args ["", "", hook_status] else ["", "", webpack_status] end endThen add explicit verification:
expect(Open3).to have_received(:capture3).with(hash_including, "bin/prepare", any_args).once
274-302: Consider refactoring for consistency, though verification is present.While this test verifies the hook was called with the correct environment variables (by checking
captured_env), it still uses thecall_countpattern. For consistency with the PR objective, consider refactoring to use the hook_command dispatch pattern.You could refactor to:
- call_count = 0 captured_env = nil allow(Open3).to receive(:capture3) do |env, *args| - call_count += 1 - captured_env = env if call_count == 1 - if call_count == 1 + if args[0] == "bin/hook" + captured_env = env ["", "", hook_status] else ["", "", webpack_status] end end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
spec/shakapacker/compiler_spec.rb(5 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-11-25T05:14:43.766Z
Learnt from: CR
Repo: shakacode/shakapacker PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T05:14:43.766Z
Learning: Run corresponding RSpec tests when changing source files (e.g., when changing `lib/shakapacker/foo.rb`, run `spec/shakapacker/foo_spec.rb`)
Applied to files:
spec/shakapacker/compiler_spec.rb
📚 Learning: 2025-11-25T05:14:43.766Z
Learnt from: CR
Repo: shakacode/shakapacker PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T05:14:43.766Z
Learning: This gem supports both webpack and rspack configurations - test changes with both bundlers when modifying core functionality
Applied to files:
spec/shakapacker/compiler_spec.rb
📚 Learning: 2024-10-09T10:50:16.512Z
Learnt from: marvinthepa
Repo: shakacode/shakapacker PR: 520
File: spec/shakapacker/utils_manager_spec.rb:104-116
Timestamp: 2024-10-09T10:50:16.512Z
Learning: In `spec/shakapacker/utils_manager_spec.rb`, `error_unless_package_manager_is_obvious` is not a test method, so extracting into a shared context may not be helpful.
Applied to files:
spec/shakapacker/compiler_spec.rb
📚 Learning: 2024-10-09T10:47:17.620Z
Learnt from: marvinthepa
Repo: shakacode/shakapacker PR: 520
File: spec/shakapacker/utils_manager_spec.rb:68-89
Timestamp: 2024-10-09T10:47:17.620Z
Learning: In `spec/shakapacker/utils_manager_spec.rb`, the code within the context `"when lockfile is in Rails.root, but pwd is different"` is not duplicated for each package manager.
Applied to files:
spec/shakapacker/compiler_spec.rb
📚 Learning: 2025-11-25T05:14:43.766Z
Learnt from: CR
Repo: shakacode/shakapacker PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T05:14:43.766Z
Learning: Run the full test suite with `bundle exec rspec` before pushing
Applied to files:
spec/shakapacker/compiler_spec.rb
🧬 Code graph analysis (1)
spec/shakapacker/compiler_spec.rb (2)
lib/shakapacker/compiler.rb (1)
compile(19-36)lib/shakapacker/configuration.rb (1)
precompile_hook(297-306)
⏰ 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). (20)
- GitHub Check: Test with RSpack
- GitHub Check: Testing (ubuntu-latest, 2.7, gemfiles/Gemfile-rails.7.1.x)
- GitHub Check: test
- GitHub Check: Generator specs (ubuntu-latest, 3.4, gemfiles/Gemfile-rails.8.0.x)
- GitHub Check: Generator specs (ubuntu-latest, 3.4, gemfiles/Gemfile-rails.7.2.x)
- GitHub Check: Generator specs (ubuntu-latest, 3.1, gemfiles/Gemfile-rails.7.2.x)
- GitHub Check: Generator specs (ubuntu-latest, 3.4, gemfiles/Gemfile-rails.7.1.x)
- GitHub Check: Generator specs (ubuntu-latest, 3.2, gemfiles/Gemfile-rails.8.0.x)
- GitHub Check: Generator specs (ubuntu-latest, 2.7, gemfiles/Gemfile-rails.7.0.x)
- GitHub Check: Generator specs (ubuntu-latest, 3.3, gemfiles/Gemfile-rails.8.0.x)
- GitHub Check: Generator specs (ubuntu-latest, 3.2, gemfiles/Gemfile-rails.7.1.x)
- GitHub Check: Generator specs (ubuntu-latest, 3.3, gemfiles/Gemfile-rails.7.0.x)
- GitHub Check: Generator specs (ubuntu-latest, 3.3, gemfiles/Gemfile-rails.7.1.x)
- GitHub Check: Generator specs (ubuntu-latest, 2.7, gemfiles/Gemfile-rails.7.1.x)
- GitHub Check: Generator specs (ubuntu-latest, 3.0, gemfiles/Gemfile-rails.7.0.x)
- GitHub Check: Generator specs (ubuntu-latest, 3.1, gemfiles/Gemfile-rails.7.0.x)
- GitHub Check: Generator specs (ubuntu-latest, 3.2, gemfiles/Gemfile-rails.7.0.x)
- GitHub Check: Generator specs (ubuntu-latest, 3.2, gemfiles/Gemfile-rails.7.2.x)
- GitHub Check: Generator specs (ubuntu-latest, 3.0, gemfiles/Gemfile-rails.7.1.x)
- GitHub Check: Generator specs (ubuntu-latest, 3.1, gemfiles/Gemfile-rails.7.1.x)
🔇 Additional comments (5)
spec/shakapacker/compiler_spec.rb (5)
59-80: LGTM! Well-executed refactor to explicit assertions.The new pattern with
hook_commandvariable and dispatch-based stubbing makes the test much clearer. The explicit verification usinghave_received(:capture3).with(hash_including, hook_command, hash_including).onceis self-documenting and will produce better failure messages than the old call_count approach.
82-96: LGTM! Appropriate verification for unconfigured hook scenario.The simplified stub and verification correctly tests that when
precompile_hookreturns nil, only webpack is invoked. Checkingonceimplicitly confirms the hook wasn't called, which is appropriate since there's no specific hook command to verify against.
304-325: LGTM! Excellent use of explicit negative assertion.The explicit verification using
not_to have_received(:capture3).with(anything, hook_command, anything)clearly documents that the hook should be skipped, while still verifying webpack was invoked. This is much more explicit and self-documenting than the old pattern.
327-353: LGTM! Consistent with the new assertion pattern.The explicit verification clearly documents that the hook should be called when the skip flag is not set.
355-381: LGTM! Consistent with the new assertion pattern.The explicit verification correctly tests that setting the skip flag to "false" (as opposed to "true") still runs the hook.
Document the preference for using `have_received`/`not_to have_received` assertions over indirect counter patterns in tests. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Code Review - PR #854OverviewThis PR improves test quality by replacing indirect assertion patterns with explicit RSpec spy assertions. The changes are well-aligned with the project's testing guidelines now documented in CLAUDE.md. ✅ Strengths
|
Summary
call_countcounter patterns with explicit RSpec spy assertionshave_received(:capture3).with(hook_command)to verify hook was callednot_to have_receivedto explicitly verify hook was skippedWhy this matters
The previous tests used indirect assertions like
expect(call_count).to eq(1)which:The new assertions like
expect(Open3).not_to have_received(:capture3).with(anything, hook_command, anything)are:Test plan
Based on discussion: https://github.com/shakacode/shakapacker/discussions/new?category=ideas
🤖 Generated with Claude Code
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.