Skip to content
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

Merged
merged 2 commits into from Feb 14, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 16 additions & 0 deletions docs/release-source/release/fakes.md
Expand Up @@ -152,6 +152,22 @@ f.lastCall.callback === cb2;
// true
```

#### `f.firstArg`

This property is a convenient way to get a reference to the first argument passed in the last call to the fake.

```js
var f = sinon.fake();
var date1 = new Date();
var date2 = new Date();

f(date1, 1, 2);
f(date2, 1, 2);

f.firstArg === date2;
// true
```

#### `f.lastArg`

This property is a convenient way to get a reference to the last argument passed in the last call to the fake.
Expand Down
14 changes: 14 additions & 0 deletions docs/release-source/release/spy-call.md
Expand Up @@ -121,6 +121,20 @@ spy.lastCall.callback === callback;
// true
```

#### `spyCall.firstArg`

This property is a convenience for the first argument of the call.

```js
var spy = sinon.spy();
var date = new Date();

spy(date, 1, 2);

spy.lastCall.firstArg === date;
// true
```

#### `spyCall.lastArg`

This property is a convenience for the last argument of the call.
Expand Down
9 changes: 8 additions & 1 deletion lib/sinon/fake.js
Expand Up @@ -14,9 +14,16 @@ var uuid = 0;
function wrapFunc(f) {
var proxy;
var fakeInstance = function() {
var lastArg = arguments.length > 0 ? arguments[arguments.length - 1] : undefined;
var firstArg, lastArg;

if (arguments.length > 0) {
firstArg = arguments[0];
lastArg = arguments[arguments.length - 1];
}

var callback = lastArg && typeof lastArg === "function" ? lastArg : undefined;

proxy.firstArg = firstArg;
proxy.lastArg = lastArg;
proxy.callback = callback;

Expand Down
9 changes: 8 additions & 1 deletion lib/sinon/proxy-call.js
Expand Up @@ -233,13 +233,20 @@ function createProxyCall(proxy, thisValue, args, returnValue, exception, id, err
throw new TypeError("Call id is not a number");
}

var firstArg, lastArg;

if (args.length > 0) {
firstArg = args[0];
lastArg = args[args.length - 1];
}

var proxyCall = Object.create(callProto);
var lastArg = (args.length > 0 && args[args.length - 1]) || undefined;
var callback = lastArg && typeof lastArg === "function" ? lastArg : undefined;

proxyCall.proxy = proxy;
proxyCall.thisValue = thisValue;
proxyCall.args = args;
proxyCall.firstArg = firstArg;
proxyCall.lastArg = lastArg;
proxyCall.callback = callback;
proxyCall.returnValue = returnValue;
Expand Down
2 changes: 2 additions & 0 deletions lib/sinon/proxy.js
Expand Up @@ -123,6 +123,7 @@ var proxyApi = {
this.thirdCall = null;
this.lastCall = null;
this.args = [];
this.firstArg = null;
Copy link
Member

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 become null after being reset().

Not sure this needs to be fixed, but just making note of it.

Copy link
Contributor Author

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 or null. (My vote is undefined.)

Copy link
Member

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?

Copy link
Contributor Author

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 have firstArg and lastArg default to different values, but changing lastArg's default value is a big change. Maybe we could do that in a separate PR and just change firstArg to default to undefined for now?

Copy link
Member

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.

this.lastArg = null;
this.returnValues = [];
this.thisValues = [];
Expand Down Expand Up @@ -272,6 +273,7 @@ function wrapFunction(func, originalFunc) {
calledThrice: false,
callCount: 0,
firstCall: null,
firstArg: null,
secondCall: null,
thirdCall: null,
lastCall: null,
Expand Down
29 changes: 29 additions & 0 deletions test/fake-test.js
Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe assert.isTrue?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also copied the tests for lastArg here as well. Happy to change them though.


f();
assert.isUndefined(f.firstArg);
});
});

describe(".lastArg", function() {
it("should be the last argument from the last call", function() {
var f = fake();
Expand Down
18 changes: 18 additions & 0 deletions test/proxy-call-test.js
Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could use assert.isUndefined here, as you did in fake-test.js.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just copied the tests for lastArg. I can change it though if you'd like.

});
});

describe(".lastArg", function() {
it("should be the last argument from the call", function() {
var spy = sinonSpy();
Expand Down