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
Add firstArg to spy calls and fakes. #2150
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -133,6 +133,35 @@ describe("fake", function() { | |
}); | ||
}); | ||
|
||
describe(".firstArg", function() { | ||
it("should be the first argument from the last call", function() { | ||
var f = fake(); | ||
f(41, 42, 43); | ||
assert.equals(f.firstArg, 41); | ||
|
||
f(44, 45); | ||
assert.equals(f.firstArg, 44); | ||
|
||
f(46); | ||
assert.equals(f.firstArg, 46); | ||
|
||
f(false, true, 47, "string"); | ||
assert.equals(f.firstArg, false); | ||
|
||
f("string", false, true, 47); | ||
assert.equals(f.firstArg, "string"); | ||
|
||
f(47, "string", false, true); | ||
assert.equals(f.firstArg, 47); | ||
|
||
f(true, 47, "string", false); | ||
assert.equals(f.firstArg, true); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also copied the tests for |
||
|
||
f(); | ||
assert.isUndefined(f.firstArg); | ||
}); | ||
}); | ||
|
||
describe(".lastArg", function() { | ||
it("should be the last argument from the last call", function() { | ||
var f = fake(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -553,6 +553,24 @@ describe("sinonSpy.call", function() { | |
}); | ||
}); | ||
|
||
describe(".firstArg", function() { | ||
it("should be the first argument from the call", function() { | ||
var spy = sinonSpy(); | ||
|
||
spy(41, 42, 43); | ||
assert.equals(spy.getCall(0).firstArg, 41); | ||
|
||
spy(44, 45); | ||
assert.equals(spy.getCall(1).firstArg, 44); | ||
|
||
spy(46); | ||
assert.equals(spy.getCall(2).firstArg, 46); | ||
|
||
spy(); | ||
assert.equals(spy.getCall(3).firstArg, undefined); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You could use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I just copied the tests for |
||
}); | ||
}); | ||
|
||
describe(".lastArg", function() { | ||
it("should be the last argument from the call", function() { | ||
var spy = sinonSpy(); | ||
|
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.
It's a bit weird/inconsistent that the values start out as
undefined
, but becomenull
after beingreset()
.Not sure this needs to be fixed, but just making note of it.
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.
I noticed this too. I was going to change it, but didn't want to assume whether the values should be
undefined
ornull
. (My vote isundefined
.)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.
I agree with the idea of returning to
undefined
. Do you want to make the change, before we merge this in?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.
Sure. Did you just want me to change this line to
undefined
? I feel like it'd be weird to havefirstArg
andlastArg
default to different values, but changinglastArg
's default value is a big change. Maybe we could do that in a separate PR and just changefirstArg
to default toundefined
for now?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.
I agree. Let's leave
lastArg
unchanged in this PR.A separate PR to tidy up the initial and reset values would be most welcome, and will trigger a new major release.