Implement arguments aware overridable. #247
Conversation
|
|
WalkthroughThis update introduces argument-aware overriding for expected calls in the gomock framework. A new boolean field, Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test/Example
participant Controller
participant CallSet
Test->>Controller: Create Controller (WithOverridableExpectationsArgsAware)
Controller->>CallSet: Initialize with allowOverrideArgsAware=true
Test->>Controller: Set expectation (method, args1)
Controller->>CallSet: Add(method, args1)
CallSet->>CallSet: Remove existing call with matching args1
Test->>Controller: Set expectation (method, args2)
Controller->>CallSet: Add(method, args2)
CallSet->>CallSet: Remove existing call with matching args2
Test->>Controller: Set expectation (method, args1) again
Controller->>CallSet: Add(method, args1)
CallSet->>CallSet: Remove previous call with args1 only
Test->>Controller: Call method with args1
Controller->>CallSet: Match expectation for args1 (most recent)
Poem
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Nitpick comments (3)
gomock/callset.go (1)
86-93: Efficient implementation of argument-aware overridingThe implementation correctly identifies and removes only the calls with matching arguments before adding the new call. This provides the expected behavior where only expectations with matching arguments are overridden.
One suggestion to consider:
- calls := cs.expected[key] - for i, c := range calls { - if slices.Equal(c.args, call.args) { - cs.expected[key] = append(calls[:i], calls[i+1:]...) - } - } + // Filter out any calls with matching arguments + calls := cs.expected[key] + filteredCalls := make([]*Call, 0, len(calls)) + for _, c := range calls { + if !slices.Equal(c.args, call.args) { + filteredCalls = append(filteredCalls, c) + } + } + cs.expected[key] = filteredCallsThis alternative approach uses a filter pattern that avoids in-place slice modifications, which can be error-prone when modifying a slice you're iterating over. However, your current implementation is also correct.
gomock/overridable_controller_test.go (2)
59-77: Good test for argument matcher equality in overriding behavior.This test correctly demonstrates that expectations with different argument matchers can coexist without overriding each other. The test shows that specific argument matchers (exact string "foo") and generic matchers (
gomock.Any()) are treated as distinct expectations, allowing both to be satisfied in the expected order.However, consider adding another test case that demonstrates what happens when you override a generic matcher with a specific one and vice versa. This would provide additional clarity on the precedence rules when multiple matchers could potentially match the same arguments.
36-77: Consider adding edge case tests for argument-aware overriding.The current tests cover the basic functionality well, but consider adding tests for edge cases such as:
- What happens with multiple identical argument matchers and
Times(n)expectations?- Interaction with ordered/unordered expectations
- Behavior when overriding expectations with partial argument matcher matches (e.g., multiple arguments where only some match)
These additional tests would help ensure the feature is robust across different usage patterns.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
gomock/callset.go(4 hunks)gomock/callset_test.go(1 hunks)gomock/controller.go(1 hunks)gomock/example_test.go(1 hunks)gomock/overridable_controller_test.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
gomock/callset.go (1)
gomock/call.go (1)
Call(25-45)
gomock/overridable_controller_test.go (1)
gomock/controller.go (2)
NewController(84-104)WithOverridableExpectationsArgsAware(127-129)
🔇 Additional comments (7)
gomock/example_test.go (1)
71-94: Great example for the new feature!This example clearly demonstrates how the new argument-aware overridable expectations feature works. It shows that when using
WithOverridableExpectationsArgsAware(), only the expectations with matching arguments ("foo") are overridden, resulting in the second expectation's behavior ("I'm NOT sleepy") taking precedence.gomock/controller.go (1)
123-133: Proper implementation of the controller optionThe implementation follows the established pattern for controller options in the codebase. The documentation comment clearly explains the purpose and behavior of this option, which will help users understand when to use it.
gomock/callset_test.go (1)
63-86: Comprehensive test for argument-aware override behaviorThis test effectively verifies the key functionality of the new feature by:
- Adding a call with argument "foo"
- Adding a call with argument "bar"
- Re-adding a call with argument "foo"
- Confirming that only the matching "foo" call is overridden while the "bar" call remains
The assertions correctly validate that the total number of expected calls remains 2, showing that only the matching call was replaced.
gomock/callset.go (3)
21-21: Appropriate use of standard libraryGood choice using the
slicespackage for equality comparison. This is more efficient and less error-prone than implementing a custom comparison.
35-37: Clear documentation for new fieldThe comment explains the purpose of the field well, which will help future maintainers understand the code.
63-71: Good initialization of the new call set typeCreating a separate constructor function is consistent with the existing pattern in the codebase. Setting
allowOverride: falseexplicitly makes it clear that the two override modes are mutually exclusive.gomock/overridable_controller_test.go (1)
36-57: Well-structured test for argument-aware overriding base case.This test case effectively demonstrates the new argument-aware overriding functionality. It properly shows that with the
WithOverridableExpectationsArgsAware()option, only expectations with matching arguments are overridden, while others remain intact. The test verifies both the overridden and non-overridden expectations, providing complete coverage of the feature's core behavior.
Currently, a mock "reset" is supported via allowOverride parameter. Consider an example when a mock is called multiple times with different arguments:
In this case if only one call needs to be overridden it is impossible to use
allowOverrideas it resets all the calls.This PR adds a new param which is set up via
WithOverridableExpectationsArgsAware. When provided, only expectations with matching argument matchers will be overridden, see the test examples.Additional context:
reset feature request, override expectations feature request
Related recent control over expectations feature request
Summary by CodeRabbit