fixes issue #1368 by adding stub#resolvesThis #1517
Conversation
lib/sinon/default-behaviors.js
Outdated
| resolvesThis: function resolvesThis(fake) { | ||
| fake.resolveThis = true; | ||
| fake.resolve = false; | ||
| fake.reject = false; |
There was a problem hiding this comment.
IMO we should keep this consistent with other behaviors, such as rejects, which also sets many other flags, as you can see here. Even though this is enough to make it work, I think that by keeping the whole API consistent when it comes to flagging fakes is extremely important.
There was a problem hiding this comment.
I think you created the wrong link? It just points back to your comment.
There was a problem hiding this comment.
I guess I can set returnValueDefined and exception flags to false to keep consistency. Do you think some other flags need to be turned off?
There was a problem hiding this comment.
@fatso83 I think that link only works inside the files tab. When I click it inside of it the link works.
Basically I think the following flags should be set:
fake.returnValuefake.returnValueDefinedfake.exception
There was a problem hiding this comment.
Okay now resolves, rejects and resolvesThis all act on the same flags.
I set returnValue to undefined as the stub resolves with itself.
Is it okay by you?
|
Hi @HugoMuller, thanks for this awesome Pull Request! I really liked your tests and your clean code. It works, indeed and it is an extremely important feature IMO. I just had a comment regarding the flags being set when the Please let me know if you disagree with anything or if you have any further doubts. I'm looking forward to being able to merging this as soon as everyone approves it. Thanks again for your work 😄 |
test/stub-test.js
Outdated
| var strictMode = (function () { | ||
| return this; | ||
| }()) === undefined; | ||
| if (strictMode) { |
There was a problem hiding this comment.
I am not sure a conditional for strict mode is necessary:
- Sinon's source is meant to be ES5.1, strict mode and expects to run in engines that supports that.
- The test file has
use strict;at the top.
If for some reason, someone decides to run the tests in non-strict mode or in engines that doesn't support that, then I think the tests should be allowed to fail.
There was a problem hiding this comment.
Actually, the tests I wrote are very similar to those written for returnsThis.
So the same logic applies here. But if this one does not make sense, then I can remove it.
Same remark for the unclear expectations.
test/stub-test.js
Outdated
| }); | ||
| } | ||
|
|
||
| it("stub respects call", function () { |
There was a problem hiding this comment.
Can this expectation be more specific, to make it easier to understand what "respects call" means?
test/stub-test.js
Outdated
| }); | ||
| }); | ||
|
|
||
| it("stub respects apply", function () { |
There was a problem hiding this comment.
Can this expectation be more specific, to make it easier to understand what "respects apply" means?
test/stub-test.js
Outdated
| @@ -2171,6 +2230,16 @@ describe("stub", function () { | |||
| refute.defined(instance.stub()); | |||
| }); | |||
|
|
|||
| it("cleans 'resolvesThis' behavior", function () { | |||
There was a problem hiding this comment.
It is generally better to be very specific when writing expectations, especially in codebases with distributed contributors. Generic expectations often lead to readers trying to derive the intent from the implementation of the test. If there's a mistake in the implementation, then the reader is likely to end up with an incorrect understanding of then intended behaviour.
I would suggest rewriting the expectation to something like this:
it("cleans 'resolvesThis' behavior using stub.resetBehavior", function () {
it("calls stub's resetBehavior method to clean behavior", function () {There was a problem hiding this comment.
I'll fix these expectations. Thanks for the suggestions.
stub#resolvesThis sets some more flags better unit tests expectations to improve readability remove the "strictMode" test that probably does not make sense
|
👍 |
between resolves, rejects and resolvesThis
closes sinonjs#1368 by adding stub#resolvesThis
Purpose (TL;DR) - mandatory
Background (Problem in detail) - optional
Solution - optional
How to verify - mandatory
npm installnpm test