Feature/spy call order by parameter #75

cdgn-coding · 2017-06-29T02:38:26Z

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 · 2017-06-29T04:56:23Z

Good job!

cdgn-coding · 2017-06-30T00:52:02Z

Thank you @stalniy! Looking forward to contribute.

Regards

keithamus · 2017-06-30T11:18:40Z

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)

cdgn-coding · 2017-07-01T13:16:17Z

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:

nth with ~ given (spy, n, expectedArguments) => the nth call of the spy should be a superset of expectedArguments
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 · 2017-07-01T22:44:49Z

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.

cdgn-coding · 2017-07-10T01:56:40Z

@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.

keithamus · 2017-07-10T06:09:36Z

test/spies.js

@@ -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 () {


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

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

cdgn-coding · 2017-07-13T02:05:04Z

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

cdgn-coding · 2017-07-17T21:22:57Z

Hello, are there any news on this?

keithamus

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 👍

stalniy

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

stalniy · 2017-06-29T04:54:04Z

test/spies.js

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


Redundant. Assertion below covers this case

stalniy · 2017-06-29T04:55:02Z

test/spies.js

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


Redundant. Covered by assertions below

stalniy · 2017-06-29T04:55:31Z

test/spies.js

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


Redundant. The same reason

stalniy · 2017-07-20T07:44:30Z

README.md

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


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

stalniy · 2017-07-20T07:49:56Z

lib/spy.js

+      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


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

stalniy · 2017-07-20T07:55:45Z

lib/spy.js

+      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


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

stalniy · 2017-07-27T08:09:36Z

@cnexans do you have time to fix this?

cdgn-coding · 2017-07-31T23:50:30Z

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

stalniy · 2017-08-16T18:52:10Z

@cnexans any progress?

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

stalniy · 2017-09-05T02:51:19Z

@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 :)

cdgn-coding closed this Jun 29, 2017

cdgn-coding reopened this Jun 29, 2017

cdgn-coding mentioned this pull request Jul 1, 2017

How to spy on asynchronous calls #64

Closed

keithamus reviewed Jul 10, 2017

View reviewed changes

keithamus approved these changes Jul 17, 2017

View reviewed changes

stalniy requested changes Jul 20, 2017

View reviewed changes

stalniy mentioned this pull request Jul 27, 2017

refactor(spy): removes reset method #74

Merged

stalniy force-pushed the feature/spy-call-order-by-parameter branch from 287657b to 3d2ba5e Compare September 5, 2017 02:47

feat(spy): adds support for spy.nth call assertions

4b1661c

stalniy force-pushed the feature/spy-call-order-by-parameter branch from 3d2ba5e to 4b1661c Compare September 5, 2017 02:50

stalniy approved these changes Sep 5, 2017

View reviewed changes

stalniy merged commit ba7636b into chaijs:master Sep 5, 2017

stalniy mentioned this pull request Sep 5, 2017

Check on spy call order by parameter #59

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/spy call order by parameter #75

Feature/spy call order by parameter #75

cdgn-coding commented Jun 29, 2017

stalniy commented Jun 29, 2017

cdgn-coding commented Jun 30, 2017

keithamus commented Jun 30, 2017 •

edited

cdgn-coding commented Jul 1, 2017 •

edited

keithamus commented Jul 1, 2017

cdgn-coding commented Jul 10, 2017 •

edited

keithamus Jul 10, 2017

cdgn-coding Jul 11, 2017

cdgn-coding commented Jul 13, 2017 •

edited

cdgn-coding commented Jul 17, 2017

keithamus left a comment

stalniy left a comment

stalniy Jun 29, 2017

stalniy Jun 29, 2017

stalniy Jun 29, 2017

stalniy Jul 20, 2017

stalniy Jul 20, 2017

stalniy Jul 20, 2017

stalniy commented Jul 27, 2017

cdgn-coding commented Jul 31, 2017

stalniy commented Aug 16, 2017 •

edited

stalniy commented Sep 5, 2017 •

edited

Feature/spy call order by parameter #75

Feature/spy call order by parameter #75

Conversation

cdgn-coding commented Jun 29, 2017

stalniy commented Jun 29, 2017

cdgn-coding commented Jun 30, 2017

keithamus commented Jun 30, 2017 • edited

cdgn-coding commented Jul 1, 2017 • edited

keithamus commented Jul 1, 2017

cdgn-coding commented Jul 10, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdgn-coding commented Jul 13, 2017 • edited

cdgn-coding commented Jul 17, 2017

keithamus left a comment

Choose a reason for hiding this comment

stalniy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stalniy commented Jul 27, 2017

cdgn-coding commented Jul 31, 2017

stalniy commented Aug 16, 2017 • edited

stalniy commented Sep 5, 2017 • edited

keithamus commented Jun 30, 2017 •

edited

cdgn-coding commented Jul 1, 2017 •

edited

cdgn-coding commented Jul 10, 2017 •

edited

cdgn-coding commented Jul 13, 2017 •

edited

stalniy commented Aug 16, 2017 •

edited

stalniy commented Sep 5, 2017 •

edited