Check on spy call order by parameter

[teone]

In some cases is needed to check that the same spy has been called multiple times with different parameters (eg: while testing a curried function)

This is a possible desired feature, suggested by @keithamus:

const spy = chai.spy();
spy('a');
spy('b');
spy('c');
expect(spy).to.first.be.called.with('a');
expect(spy).to.second.be.called.with('b');
expect(spy).to.third.be.called.with('c');

It could be hard to match/generate first, second, third, nth property in the assertion chain.

Another possible assertion structure is:

const spy = chai.spy();
spy('a');
spy('b');
spy('c');
expect(spy).to.have.been.called.with.ordered('a', 'b', 'c');

[cdgn-coding]

Any new about this? I want to work for this feature.

[keithamus]

We store the calls as an array of arguments, so this seems like an entirely reasonable feature to add. I'd personally like to see this feature as well. In addition I'd like to see:

const spy = chai.spy();
spy('a')
spy('b')
spy('c')
expect(spy).to.be.called.with('b').after.being.called.with('a')

Perhaps that's a discussion for another time though.

---

The change:

Adding the flags first, second and third and modifying the with assertion to accept those flags. As an optional stretch goal, adding an after flag

Who can make this PR?

Anyone who wants to contribute to this project is welcome to make this PR!

I want to make this PR!

Great! You should take a look at our Code of Conduct and Contribution Guidelines (note: we need to work on our contribution guidelines, so these might not apply exactly to this sub-project).

If you want to work on this, you should comment below, something like:

Hey, I'd like to work on this issue, I'll be making a PR shortly.

If you don't comment you might run the risk of someone else doing the work before you!

If someone has already commented, then you should leave this PR for them to work on.

If someone has commented, but a few weeks have passed - it is worth asking the person who has commented if they're still interested in working on this, and if you can pick it up instead.

How to make a PR for this change:

You'll need to add the flags first, second, third - take a look at the always property to see how that's done. You could do something like:

  Assertion.addProperty('first', function () {
    if ('undefined' !== this._obj.__spy) {
      _.flag(this, 'spy call index', 1);
    }
  });

  Assertion.addProperty('second', function () {
    if ('undefined' !== this._obj.__spy) {
      _.flag(this, 'spy call index', 2);
    }
  });

  Assertion.addProperty('third', function () {
    if ('undefined' !== this._obj.__spy) {
      _.flag(this, 'spy call index', 3);
    }
  });

You'll then need to modify the .with function to listen to those flags. I'd recommend storing an index as part of the calls.forEach and then checking if the index flag exist, and if those two match - so assertWith becomes something like:

function assertWith () {
    new Assertion(this._obj).to.be.spy;
    var expArgs = [].slice.call(arguments, 0)
      , calls = this._obj.__spy.calls
      , always = _.flag(this, 'spy always')
      , expectedCallIndex = _.flag(this, 'spy call index')
      , callIndex = null
      , passed = 0;

    calls.forEach(function (call, index) {
      var actArgs = call.slice()
        , found = 0;

      expArgs.forEach(function (expArg) {
        for (var i = 0; i < actArgs.length; i++) {
          if (_.eql(actArgs[i], expArg)) {
            found++;
            actArgs.splice(i, 1);
            break;
          }
        }
      });
      if (found === expArgs.length) {
        passed++;
        callIndex = callIndex === null ? index : callIndex;
      }
    });

    if (typeof expectedCallIndex === 'number') {
      var extraMessage = 'was not called'
      if (callIndex >= 0) {
        extraMessage = 'was called on call ' + callIndex
      }
      this.assert(
        expectedCallIndex === callIndex
      , 'expected ' + this._obj + ' to have been called with #{exp} on call ' + expectedCallIndex + ' but ' + extraMessage,
      , 'expected ' + this._obj + ' to not have been called with #{exp} on call ' + expectedCallindex
      , expArgs
      )
    } else if (always) {
      this.assert(
          passed === calls.length
        , 'expected ' + this._obj + ' to have been always called with #{exp} but got ' + passed + ' out of ' + calls.length
        , 'expected ' + this._obj + ' to have not always been called with #{exp}'
        , expArgs
      );
    } else {
      this.assert(
          passed > 0
        , 'expected ' + this._obj + ' to have been called with #{exp}'
        , 'expected ' + this._obj + ' to have not been called with #{exp} but got ' + passed + ' times'
        , expArgs
      );
    }
  }

As always, you'll need to write some tests to back this up and also add some docs in the README. If you want to implement an after flag as I mentioned above, please also feel free 😄. If you'd like some assistance with this - please feel free to ask!

Once if you done this, if you run the npm test command then you should see it all passes. If you get stuck, just pop on here to explain the problem and ping the @chaijs/chai-spies team, who will try to help with the issue.

What happens then?

We'll need 2 contributors to accept the change, and it'll be merged. Once merged, we'll need to take some time to make a manual release, which will take a while - as we need to prepare release notes. It'll likely be released as part of a set of changes for a major chai-spies version.

[cdgn-coding]

I want to make this pull request. I forked yesterday and I will keep working on this and come back to you with news.

Regards.

[cdgn-coding]

I commited first, second and third modifiers for the with method. It may make sense to also have nth modifier/method, what do you think? For the after modifier I have to plan first how to do that hehe

On the other hand, which branch should I pick to create the pull request against this project?

Regards.

[keithamus]

Great work @cnexans! We could add an nth, not sure how the grammar would work for that... expect(spy).on.nth(7).be.called.with('a')? Doesn't exactly flow off the tongue 😆

For now - feel free to PR against master and we can begin reviewing the code 😄

[cdgn-coding]

Hey @keithamus thank you. Created the pr for the first four assertions: first, second, third and nth. After code review I want to create a pr for the after method (if welcomed).

I guess we'd talk about nth grammar during the code review. I kept .on.nth(...).be.called.with(...) tentatively because I'm sure more ideas will come up during next phase.

Regards

[stalniy]

@keithamus how should it work in case of checking multiple args?

spy(1,2,3)
expect(spy).to.first.be.called.with(2,3) // is this true or error? I'd expect error

expect(spy).to.first.be.called.with(1) // the same question, true or error? I'd expect true

Alternatively there may be something like be.called.exactly.with(1,2,3).

[keithamus]

@stalniy we already have an exactly; which I suppose would be the preferred use-case, otherwise I would expect both of those assertions in your example to pass, as the first call did include those arguments. Such is the nature of called.with without exactly.

[stalniy]

closed by #75