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

Check on spy call order by parameter #59

Closed
teone opened this issue Aug 5, 2016 · 9 comments

Comments

@teone
Copy link

commented Aug 5, 2016

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');
@cnexans

This comment has been minimized.

Copy link
Contributor

commented Jun 27, 2017

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

@keithamus

This comment has been minimized.

Copy link
Member

commented Jun 27, 2017

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.

@cnexans

This comment has been minimized.

Copy link
Contributor

commented Jun 27, 2017

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.

@cnexans

This comment has been minimized.

Copy link
Contributor

commented Jun 28, 2017

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

This comment has been minimized.

Copy link
Member

commented Jun 28, 2017

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 😄

@cnexans

This comment has been minimized.

Copy link
Contributor

commented Jun 29, 2017

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

This comment has been minimized.

Copy link
Contributor

commented Jun 29, 2017

@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

This comment has been minimized.

Copy link
Member

commented Jun 29, 2017

@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

This comment has been minimized.

Copy link
Contributor

commented Sep 5, 2017

closed by #75

@stalniy stalniy closed this Sep 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.