Called in order considers callCount #1406

lucasfcosta · 2017-05-17T21:35:17Z

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

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

EDIT: Building is breaking on PhantomJS, I gotta see why. Let me check this...

lucasfcosta · 2017-05-18T01:27:49Z

Hmm, I spent an hour playing with mochify and phantomjs related configs but I couldn't get it to work.

I even tried to run a single test and change the callInOrder function to always return true in order to check for any better output but I couldn't get it to work in any way.

I'll dig deeper into this as soon as I've got more time to, but if any of you can remember any detail that might help me fix this, please let me know.

fatso83 · 2017-05-18T03:23:37Z

Hmm... are you able to make it pass locally? I always found mochify (really it's phantom) to be a bit of a black box with regards to diagnostics.

lucasfcosta · 2017-05-18T18:56:51Z

Nope, I should've tested that before opening that PR, it was my fault to submit a breaking PR, but since I cannot track down the reason of this I'm now thinking it was a good idea do open this PR in order to get feedback.

A funny thing is that if I enable the --consolify flag, tests pass.

mantoni · 2017-05-18T19:14:14Z

If you run --consolify it generates a web page to run in a browser. It always exits with code 0. Trying to run the tests in a real browser might shed some light on this though. Also, you can try to run in real browsers with --wd.

lucasfcosta · 2017-05-18T21:07:49Z

@mantoni Just tried to play around with mochify in the last hour and couldn't get it to run tests on any real browsers :(

I always end up getting this:

> mochify --recursive -R dot --grep WebWorker --invert test/ --wd                                                                                         │  6             callMap[spy.id] = 0;
                                                                                                                                                          │  5         }
POST /wd/hub/session                                                                                                                                      │  4
                                                                                                                                                          │  3         return callMap[spy.id] < spy.callCount;
Unexcected request error: connect ECONNREFUSED 127.0.0.1:4444                                                                                             │  2     }
    Error: connect ECONNREFUSED 127.0.0.1:4444                                                                                                            │  1
        at Object.exports._errnoException (util.js:1022:11)                                                                                               │14      if (arguments.length > 1) {
        at exports._exceptionWithHostPort (util.js:1045:20)                                                                                               │  1         spies = Array.prototype.slice.call(arguments);
        at TCPConnectWrap.afterConnect [as oncomplete] (net.js:1090:14)                                                                                   │  2     }
                                                                                                                                                          │  3
Error: Unexcected request error: connect ECONNREFUSED 127.0.0.1:4444

Any ideas on how to solve this and investigate further?

mantoni · 2017-05-19T06:33:50Z

Ok, so I checked out your branch and ran the tests locally. I found your issue: It's the arrow function in the test case. You have to use function () { ... or phantomjs (and Safari and IE 11 on SauseLabs) will fail with really odd messages. It needs investigation on how we can make PhantomJS / Phantomic / Mochify spit out a more helpful message in cases where the script cannot be parsed.

If you want your local setup working, check the following:

The --consolify option works like this: mochify --recursive -R dot --grep WebWorker --invert test/ --consolify test.html, then open test.html.
The --wd option connects to a Selenium webdriver server. In the Sinon.JS project, it's configured to connect to SauceLabs, but for you it seems to try a local connection. There are instructions in the Mochify readme on how to setup a local webdriver instance.

mantoni · 2017-05-19T07:37:41Z

FYI: I'm working on a fix for Phantomic to catch parser errors. It's quite a hack, but I should have something soon.

mantoni · 2017-05-19T07:56:10Z

Finally! This was a bug I was trying to hunt down for quite some time. Your issue nudged me in the right direction 🙌

If you update phantomic to v1.5.2 (see mantoni/phantomic@47e7cdd), running the tests on your branch will yield

# phantomjs:
SyntaxError: Unexpected token ')'
      at test/issues/issues-test.js:205
Error: Exit 1

🐛 🎉

fatso83 · 2017-05-19T07:58:06Z

@mantoni that is great news! debugging/finding parsing errors was the main pain point of using mochify/phantomic so far. in truth a big improvement!

mantoni · 2017-05-19T08:03:34Z

I restarted the Travis job, but it still shows the old behavior. I think the dependencies are cached. Does anybody know how to clear node_modules for Travis builds?

fearphage

The test case feels like unexpected behavior. If this feature is useful to someone, could we give it a different/special name perhaps?

fearphage · 2017-05-20T03:58:16Z

test/issues/issues-test.js

+            s1();
+
+            assert.exception(function () {
+                sinon.assert.callOrder(s2, s1, s2);


This looks broken. That is not the order they were called in.

Yup, In this test I'm expecting an error because that is not the order those spies are called in.

Basically I'm asserting this feature fails when it should.

My mistake for misreading.

No problem, buddy
I think it's ready for a merge now.

lucasfcosta · 2017-05-20T11:25:18Z

@fearphage thanks for your review 😄

Since this feature has already been released I'm not sure changing its name would be a good idea (this is my personal opinion). Because it adds effort for people that area already using it and I also think that calledInOrder is a good name.

It denotes exactly what we want to assert on and also does not include the word immediately, which is also great because then people know there might be interleaving calls between the passed args.

What does everyone think? Let's gather some more opinions.

@mantoni thanks for your help, it's awesome to collaborate with such helpful and hard-working people ❤️

Btw, sorry for the misattention. I'm so used to arrow functions that my brain just goes straight through them.

I just fixed this PR. It is now ready to merge if everyone agrees.

coveralls · 2017-05-20T11:30:36Z

Coverage increased (+0.02%) to 95.042% when pulling 4a144d5 on lucasfcosta:called-in-order-considers-callcount into 91c376e on sinonjs:master.

fearphage · 2017-05-20T12:52:39Z

test/issues/issues-test.js

+            s1();
+
+            assert.exception(function () {
+                sinon.assert.callOrder(s2, s1, s2);


My mistake for misreading.

fearphage · 2017-05-21T17:23:26Z

lib/sinon/util/core/called-in-order.js

    if (arguments.length > 1) {
-        spies = arguments;
+        spies = Array.prototype.slice.call(arguments);


MINOR: The only reason to use slice here would be if you plan to used spies.every(...) later. However you are using every.call(...) so you can revert this line.

@fearphage well noticed! I just fixed it.

Thanks for your careful review, this was indeed something that needed to be fixed 👍

fearphage · 2017-05-21T17:24:10Z

@lucasfcosta One small thing, but I think it's good to go after that.

coveralls · 2017-05-21T22:17:27Z

Coverage increased (+0.02%) to 95.074% when pulling 54b5837 on lucasfcosta:called-in-order-considers-callcount into fb514cf on sinonjs:master.

fearphage requested changes May 20, 2017

View reviewed changes

lucasfcosta force-pushed the called-in-order-considers-callcount branch from e580963 to 4a144d5 Compare May 20, 2017 11:29

fearphage approved these changes May 20, 2017

View reviewed changes

test/issues/issues-test.js

s1();

assert.exception(function () {

sinon.assert.callOrder(s2, s1, s2);

Copy link

Member

fearphage May 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My mistake for misreading.

fearphage reviewed May 21, 2017

View reviewed changes

fearphage added the semver:patch changes will cause a new patch version label May 21, 2017

Called in order takes callCount into account. Closes sinonjs#1398.

54b5837

lucasfcosta force-pushed the called-in-order-considers-callcount branch from 4a144d5 to 54b5837 Compare May 21, 2017 22:14

fearphage merged commit 2f941e2 into sinonjs:master May 26, 2017

renovate bot mentioned this pull request Jul 8, 2017

chore(deps): Update dependency sinon to version ^2.0.0 factset/labeledpipe#15

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Called in order considers callCount #1406

Called in order considers callCount #1406

lucasfcosta commented May 17, 2017 •

edited

lucasfcosta commented May 18, 2017

fatso83 commented May 18, 2017

lucasfcosta commented May 18, 2017

mantoni commented May 18, 2017

lucasfcosta commented May 18, 2017

mantoni commented May 19, 2017

mantoni commented May 19, 2017

mantoni commented May 19, 2017

fatso83 commented May 19, 2017

mantoni commented May 19, 2017

fearphage left a comment

fearphage May 20, 2017

lucasfcosta May 20, 2017

fearphage May 20, 2017

lucasfcosta May 21, 2017

lucasfcosta commented May 20, 2017 •

edited

coveralls commented May 20, 2017 •

edited

fearphage May 20, 2017

fearphage May 21, 2017

lucasfcosta May 21, 2017 •

edited

fearphage commented May 21, 2017

coveralls commented May 21, 2017 •

edited

Called in order considers callCount #1406

Called in order considers callCount #1406

Conversation

lucasfcosta commented May 17, 2017 • edited

Purpose (TL;DR)

Background (Problem in detail)

Solution

How to verify

lucasfcosta commented May 18, 2017

fatso83 commented May 18, 2017

lucasfcosta commented May 18, 2017

mantoni commented May 18, 2017

lucasfcosta commented May 18, 2017

mantoni commented May 19, 2017

mantoni commented May 19, 2017

mantoni commented May 19, 2017

fatso83 commented May 19, 2017

mantoni commented May 19, 2017

fearphage left a comment

Choose a reason for hiding this comment

fearphage May 20, 2017

Choose a reason for hiding this comment

lucasfcosta May 20, 2017

Choose a reason for hiding this comment

fearphage May 20, 2017

Choose a reason for hiding this comment

lucasfcosta May 21, 2017

Choose a reason for hiding this comment

lucasfcosta commented May 20, 2017 • edited

coveralls commented May 20, 2017 • edited

fearphage May 20, 2017

Choose a reason for hiding this comment

fearphage May 21, 2017

Choose a reason for hiding this comment

lucasfcosta May 21, 2017 • edited

Choose a reason for hiding this comment

fearphage commented May 21, 2017

coveralls commented May 21, 2017 • edited

lucasfcosta commented May 17, 2017 •

edited

lucasfcosta commented May 20, 2017 •

edited

coveralls commented May 20, 2017 •

edited

lucasfcosta May 21, 2017 •

edited

coveralls commented May 21, 2017 •

edited