Fix #1487: incorrect withArgs().returnValue #1490
Conversation
There was a problem hiding this comment.
Thanks, for yet another pull request, @takasmiley! I did have some objections to it, though, which is mainly summed up in KISS :-) The regression test is just too complex for this quite simple bug. Just simplify it. I think you could probably get away with 1/3 of the code.
| @@ -124,6 +124,39 @@ describe("stub", function () { | |||
| refute.isNull(stub.withArgs(1).firstCall); | |||
| }); | |||
|
|
|||
| describe("should work firstCall and lastCall", function () { | |||
There was a problem hiding this comment.
I'd put this test in issues-test.js with the description "#1487 - withArgs() returnValue" and split the testcase into two separate testcases. Right now it could fail at any of the eight assertions.
| @@ -124,6 +124,39 @@ describe("stub", function () { | |||
| refute.isNull(stub.withArgs(1).firstCall); | |||
| }); | |||
|
|
|||
| describe("should work firstCall and lastCall", function () { | |||
| var testComponentA = function () { return { render: function () { return "test a"; } }; }; | |||
| var testComponentB = function () { return { render: function () { return "test b"; } }; }; | |||
There was a problem hiding this comment.
It's great that you could directly use the test case from #1487, but I think all the nested functions here obscure the actual bug and what is going on.
It really shouldn't be necessary with this much ceremony.
| var config = { option: "a" }; | ||
| var component = this.stub(testComponentA)(config); | ||
|
|
||
| assert.isTrue(this.stub.calledWith(testComponentA)); |
There was a problem hiding this comment.
We already have a test for this in call-test.js. This can be deleted.
| var component = this.stub(testComponentA)(config); | ||
|
|
||
| assert.isTrue(this.stub.calledWith(testComponentA)); | ||
| assert.isFalse(this.stub.calledWith(testComponentB)); |
There was a problem hiding this comment.
Same as above - already tested here.
| assert.isTrue(this.stub.calledWith(testComponentA)); | ||
| assert.isFalse(this.stub.calledWith(testComponentB)); | ||
|
|
||
| assert.isFunction(component.render); |
There was a problem hiding this comment.
Delete. You are testing that the test stub you created yourself above is working, not the library.
| assert.isFalse(this.stub.calledWith(testComponentB)); | ||
|
|
||
| assert.isFunction(component.render); | ||
| assert.equals(component.render(), "fake component a"); |
There was a problem hiding this comment.
Delete. Not necessary to test your test code. Maybe you try to assert that stubbing functionality works in general, but we already know that through other tests. #1487 is explicitly about returnValues in combo with withArgs and lastCall/firstCall.
| assert.equals(component.render(), "fake component a"); | ||
|
|
||
| assert.isTrue(this.stub.withArgs(testComponentA).returnValues[0].calledWith(config)); | ||
| assert.isTrue(this.stub.withArgs(testComponentA).getCall(0).returnValue.calledWith(config)); |
There was a problem hiding this comment.
These two assertions could be one test: "it should return the same for getCall(0).returnValue and returnValues[0]", where you simply assert equality between the two calls.
There was a problem hiding this comment.
Do note that having calledWith is an uneeded complexity that has little with the core of the original issue. Just assert equality between the returnValue.
| assert.isTrue(this.stub.withArgs(testComponentA).returnValues[0].calledWith(config)); | ||
| assert.isTrue(this.stub.withArgs(testComponentA).getCall(0).returnValue.calledWith(config)); | ||
|
|
||
| assert.isTrue(this.stub.withArgs(testComponentA).firstCall.returnValue.calledWith(config)); |
There was a problem hiding this comment.
You could do the same for this call.
| assert.isTrue(this.stub.withArgs(testComponentA).getCall(0).returnValue.calledWith(config)); | ||
|
|
||
| assert.isTrue(this.stub.withArgs(testComponentA).firstCall.returnValue.calledWith(config)); | ||
| assert.isTrue(this.stub.withArgs(testComponentA).lastCall.returnValue.calledWith(config)); |
There was a problem hiding this comment.
This could have been a separate, second test to avoid confusion, where you might call the stub twice, just to be sure that getCall(1).returnValue and lastCall.returnValue returns the same.
|
Really looking forward to having this land! |
|
@brian-mann, if you check out the branch and fix the remaining issues I mentioned above you can get it out whenever you want. Just push it out and open another PR, leaving the original commit to recognize the original work. I'll see to it that it gets merged pronto. |
|
Okay @chrisbreiding is gonna hack on this. |
|
This just got merged as part of #1500, so closing. Thanks a lot for the work, @takasmiley! We appreciate it greatly. |
|
This will be out as soon as we find out what is causing the problems in #1498. |
Fix issue #1487
The cause of this bug is that createCallProperties(matching) is never called to make returnValue and exception.