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

Make calledAfter symetric with calledBefore #1407

Merged
merged 1 commit into from May 23, 2017

Conversation

lucasfcosta
Copy link
Member

@lucasfcosta lucasfcosta commented May 17, 2017

Purpose (TL;DR)

This makes calledAfter symetric with calledBefore, as I explained in #1398.

Background (Problem in detail)

Semantically speaking, calledAfter does not do what it says it does.

When you say you expect something to have been calledAfter anotherThing, you don't care if something has been called last or not, all you care about is if something has been called any times after anotherThing.

This is the way calledBefore works. It is not concerned about whether or not the current spy was the first to be called, it just checks if the current spy has been called any times before the passed spy.

Solution

I just check if the last call to the current spy (this) has happened after the first call to the passed spy (spyFn) by comparing their callIds

I also fixed a test which had the incorrect spec for this.

How to verify

  1. Check out this branch (see github instructions below)
  2. npm install
  3. npm test -> This will run all tests including the modified one with the correct spec

@coveralls
Copy link

coveralls commented May 17, 2017

Coverage Status

Coverage remained the same at 95.02% when pulling c94d00a on lucasfcosta:simmetric-callAfter into ad78132 on sinonjs:master.

@mroderick mroderick added the semver:patch changes will cause a new patch version label May 18, 2017
@mroderick
Copy link
Member

This looks good! I've applied the PATCH label, as I feel this is fixing something that is broken, not changing a design decision. Does anyone feel differently about this?

@mantoni
Copy link
Member

mantoni commented May 18, 2017

Patch should be fine. I think this is not going to make any passing tests fail.

@fatso83
Copy link
Contributor

fatso83 commented May 18, 2017

I think I put MAJOR on the accompanying issue as this could break users tests, which AFAIK is the definition of a breaking change (?). But this is almost a semantic discussion. If you are fine with patch, then I'm fine with patch. This is a very small part of Sinon anyhow, and not one that is used that frequently :-)

@lucasfcosta
Copy link
Member Author

lucasfcosta commented May 18, 2017

I'd use PATCH too.

Even though this might break user's tests, it will break them because they were relying on incorrect behavior and since we're just correcting an error it's a fix.

Imagine if this was a more explicit error, such as:

sum(2 + 2) // returns 5

// Test:
it('should sum two and two', () => {
  assert.equals(sum(2, 2), 5)
})

In that case, fixing sum would break user's test, but that's because they were relying on incorrect behavior.

@mroderick mroderick merged commit 36c6cdc into sinonjs:master May 23, 2017
@mroderick
Copy link
Member

This has become sinon@2.3.1 on npm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver:patch changes will cause a new patch version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants