-
-
Notifications
You must be signed in to change notification settings - Fork 22
refactor: 2.0 readiness #280
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
- removed Substitute.disableFor
The king has returned @notanengineercom! ❤️ 🙏 |
I finally found the time to focus on the finishing touches 😄. |
spec/ClearSubstitute.spec.ts
Outdated
}) | ||
|
||
test('clears received calls on a substitute', t => { | ||
test.skip('clears received calls on a substitute', t => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the only failing test atm (failed regression?) due to how we handle the substitution logic.
When we call clearReceivedCalls() directly (not via the symbol), the state machine does not know if it's the actual Substitute method or a method from the original class it's mocking. On all other first level Substitute methods (receive, didNotReceive, configure) this issue does not happen because we specify the kind of Substitute it is when entering that second level. E.g:
interface Calculator {
received(): boolean
sum(a, b): number
}
const calculator = Substitute.for<Calculator>()
calculator.received() // This is marked as a method call from calculator
calculator[received](1).received() // Because of the typescript hints we use the symbol
calculator.didNotReceive(1).sum(0, 0) // This is marked as a substitute assertion because we a different property (sum in the second level) is accessed after a possible assertion method (didNotReceive in the first level)
calculator.clearReceivedCalls() // We don't know internally if it's the substitute call or potentially a method of calculator
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I see two potential workarounds:
- Deviate slightly from the original spec and invoke clearReceivedCalls from the configure() scope
- Force the use of symbols for the clearReceivedCalls method always
E.g:
calculator.configure().clearReceivedCalls() // 1
calculator[clearReceivedCalls]() // 2
What do you think? Do you have other ideas or suggestions?
@ffMathy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ufff, that's a very tricky one indeed. I feel like the Symbol approach somehow leans closer to the dotnet world (where this is ported from). There, you can pick which "overload" you want (if multiple methods exist), and C# "understands" which exact method you picked. Symbols give that very same value.
Does that sound good?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, in that case we'll force the use of the clearReceivedCalls symbol to make it straightforward 👌🏼
So does that mean we can merge it? ❤️ |
This PR is work in progress for getting 2.0 released. Thanks Bitwarden and @notanengineercom for fueling this.