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

Feature/spy call order by parameter #75

Merged
merged 1 commit into from Sep 5, 2017

Conversation

@cnexans
Copy link
Contributor

commented Jun 29, 2017

#59

Adds the following assertions

to.have.been.first.called.with
to.have.been.second.called.with
to.have.been.third.called.with
on.nth(i).be.called.with

Regards

@stalniy

This comment has been minimized.

Copy link
Contributor

commented Jun 29, 2017

Good job!

@cnexans cnexans closed this Jun 29, 2017
@cnexans cnexans reopened this Jun 29, 2017
@cnexans

This comment has been minimized.

Copy link
Contributor Author

commented Jun 30, 2017

Thank you @stalniy! Looking forward to contribute.

Regards

@keithamus

This comment has been minimized.

Copy link
Member

commented Jun 30, 2017

This is looking good so far. I'd also like to see some tests that address partial arguments - for example what do these do?

spy(1,2,3)
spy(3,4,5)
// These should definitely pass - let's see tests for that
expect(spy).first.to.be.called.with(1,2,3)
expect(spy).second.to.be.called.with(3,4,5)
expect(spy).first.to.be.called.with.exactly(1,2,3)
expect(spy).second.to.be.called.with.exactly(3,4,5)

// These should definitely fail - let's see tests for that
expect(spy).first.to.be.called.with(3,4,5)
expect(spy).second.to.be.called.with(1,2,3)
expect(spy).first.to.be.called.with.exactly(1)
expect(spy).first.to.be.called.with.exactly(2)
expect(spy).first.to.be.called.with.exactly(3)
expect(spy).second.to.be.called.with.exactly(3)
expect(spy).second.to.be.called.with.exactly(4)
expect(spy).second.to.be.called.with.exactly(5)


// These should definitely pass - let's see tests for that
expect(spy).first.to.not.be.called.with(3,4,5)
expect(spy).second.to.not.be.called.with(1,2,3)
expect(spy).first.to.not.be.called.with.exactly(1)
expect(spy).first.to.not.be.called.with.exactly(2)
expect(spy).first.to.not.be.called.with.exactly(3)
expect(spy).second.to.not.be.called.with.exactly(3)
expect(spy).second.to.not.be.called.with.exactly(4)
expect(spy).second.to.not.be.called.with.exactly(5)

// Should these all pass? My intuition says yes
expect(spy).first.to.be.called.with(2)
expect(spy).first.to.be.called.with(2,3)
expect(spy).first.to.be.called.with(1)
expect(spy).first.to.be.called.with(1,2)
expect(spy).second.to.be.called.with(3,4)
expect(spy).second.to.be.called.with(4,5)
expect(spy).first.to.be.called.with(3)
expect(spy).second.to.be.called.with(3)

// Should these all fail? My intuition says yes
expect(spy).first.to.not.be.called.with(2)
expect(spy).first.to.not.be.called.with(2,3)
expect(spy).first.to.not.be.called.with(1)
expect(spy).first.to.not.be.called.with(1,2)
expect(spy).second.to.not.be.called.with(3,4)
expect(spy).second.to.not.be.called.with(4,5)
expect(spy).first.to.not.be.called.with(3)
expect(spy).second.to.not.be.called.with(3)
@cnexans

This comment has been minimized.

Copy link
Contributor Author

commented Jul 1, 2017

Hello @keithamus I would like to have a good definition of nth with and nth exactly with. This is what I'm thinking and please correct me if you don't agree:

  1. nth with ~ given (spy, n, expectedArguments) => the nth call of the spy should be a superset of expectedArguments
  2. nth exactly with ~ given (spy, n, expectedArguments) => the nth call of the spy should be equal as array to expectedArguments

At this moment, I am receiving Error: Invalid Chai property: exactly.with. See docs for proper usage of "exactly".

The first part is actually covered as all the tests without exactly pass or fail as expected. I will update the PR to fit spec 2, this should only require:

  • Add multiple arguments to existing tests
  • Add exactly as a property flag
  • Add exactly case to with method

Regards

@keithamus

This comment has been minimized.

Copy link
Member

commented Jul 1, 2017

Sorry yeah, I forgot the syntax - it should be with.exactly not exactly.with I'll fix my comment. So .called.with - as you rightly say - only ever asserts for subsets of arguments, while .called.with.exactly should test the whole argument set. I believe the same should apply to the nth flags.

@cnexans

This comment has been minimized.

Copy link
Contributor Author

commented Jul 10, 2017

@keithamus Sorry for the delay, I had a complicated week. I just added first, second, third and nth with.exactly assertions.

This is the thing I found about your last post:

spy(1, 2, 3)

// passes as {1} is a subset of {1, 2, 3} --- {1, 2, 3} is a superset of {1}
spy.should.be.first.called.with(1)

// passes as {3, 2, 1} is actually a subset of {1, 2, 3}, order does not matter!
spy.should.be.first.called.with(3, 2, 1)

// fails as {4} is not a subset of {1, 2, 3}
spy.should.be.first.called.with(4)

// fails as {1, 2, 4} is not a subset of {1, 2, 3}
spy.should.be.first.called.with(4)

// fails as It is not true for each element of {{1, 2, 2}} its multiplicity 
// is equal to its multiplicity in {{1, 2, 3}}, denoting {{...}} as multiset
spy.should.be.first.called.with(1, 2, 2)

// passes as [1, 2, 3] is equal to [1, 2, 3]
spy.should.be.first.called.with.exactly(1, 2, 3)

// fails as [2, 1, 3] is not equal to [1, 2, 3], order matters!
spy.should.be.first.called.with.exactly(2, 1, 3)

// passes as [1, 2] is not equal to [1, 2, 3], length matters!
spy.should.be.first.called.with.exactly(1, 2)

This open my mind to a some assertion, I may make an issue on this matter.

@@ -357,47 +434,65 @@ describe('Chai Spies', function () {
});
});

describe('.always.with.exactly(arg, ...)', function () {
it('should pass when called with an argument', function () {
describe('.nth(...).with.exactly(arg, ...)', function () {

This comment has been minimized.

Copy link
@keithamus

keithamus Jul 10, 2017

Member

Curious here why always.with.exactly tests have been removed?

This comment has been minimized.

Copy link
@cnexans

cnexans Jul 11, 2017

Author Contributor

Hey, seems I accidentally replaced those tests. I will be restoring them.

@cnexans

This comment has been minimized.

Copy link
Contributor Author

commented Jul 13, 2017

Tests appearently lost, restored. I apologize about that :)

@cnexans

This comment has been minimized.

Copy link
Contributor Author

commented Jul 17, 2017

Hello, are there any news on this?

Copy link
Member

left a comment

Sorry @cnexans - missed the last notification. This all looks great to me! As it's a big change I'd like @stalniy to take a look and merge it if good, but it's a LGTM from me 👍

Copy link
Contributor

left a comment

All good, I'd like @cnexans to fix few minor bugs related to documentation and error messages

spy(1);
spy(2);
spy.should.have.been.first.called.with(1);
spy.should.not.have.been.first.called.with(2);

This comment has been minimized.

Copy link
@stalniy

stalniy Jul 20, 2017

Contributor

Redundant. Assertion below covers this case

var spy = chai.spy();
spy(1);
spy(2);
spy.should.have.not.been.second.called.with(1);

This comment has been minimized.

Copy link
@stalniy

stalniy Jul 20, 2017

Contributor

Redundant. Covered by assertions below

spy(0);
spy(1);
spy(2);
spy.should.have.not.been.third.called.with(1);

This comment has been minimized.

Copy link
@stalniy

stalniy Jul 20, 2017

Contributor

Redundant. The same reason

README.md Outdated
```js
spy('foo');
spy('bar');
expect(spy).to.have.been.first.called.with('baz');

This comment has been minimized.

Copy link
@stalniy

stalniy Jul 20, 2017

Contributor

I guess it should be to.have.been.THIRD.called as you then say that spy has not been called third time.

lib/spy.js Outdated
new Assertion(this._obj).to.be.have.been.called.min(nthCall);
this.assert(
nthCallWith(spy, nthCall - 1, expArgs)
, 'expected ' + this._obj + ' to have been called at the ' + ordinalNumber + ' time with #{exp} but got ' + actArgs

This comment has been minimized.

Copy link
@stalniy

stalniy Jul 20, 2017

Contributor

actArgs is an array, if so then better to use #{act} and pass 5th argument in this.assert

lib/spy.js Outdated
new Assertion(this._obj).to.be.have.been.called.min(nthCall);
this.assert(
_.eql(actArgs, args)
, 'expected ' + this._obj + ' to have been called at the ' + ordinalNumber + ' time with exactly #{exp} but got ' + args

This comment has been minimized.

Copy link
@stalniy

stalniy Jul 20, 2017

Contributor

same here I guess args can be an array of objects and if so, then you will have not well formatted error messages

@stalniy

This comment has been minimized.

Copy link
Contributor

commented Jul 27, 2017

@cnexans do you have time to fix this?

@cnexans

This comment has been minimized.

Copy link
Contributor Author

commented Jul 31, 2017

Hi @stalniy I'm sorry I've been busy these days. I'll be fixing this next days and report back to you

@stalniy

This comment has been minimized.

Copy link
Contributor

commented Aug 16, 2017

@cnexans any progress?

update: feature is almost done it'd be good to merge it soon!

@stalniy stalniy force-pushed the cnexans:feature/spy-call-order-by-parameter branch from 287657b to 3d2ba5e Sep 5, 2017
@stalniy stalniy force-pushed the cnexans:feature/spy-call-order-by-parameter branch from 3d2ba5e to 4b1661c Sep 5, 2017
@stalniy

This comment has been minimized.

Copy link
Contributor

commented Sep 5, 2017

@cnexans @keithamus I fixed the issues and squashed commits into one. So, now it looks good and can be merged. By the way found a bug with incorrect arguments call comparison. Fixed that as well :)

@stalniy
stalniy approved these changes Sep 5, 2017
@stalniy stalniy merged commit ba7636b into chaijs:master Sep 5, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.