callOrder missbehaves

[mir3z]

• Sinon version : 2.1.0
• Environment : node
• Example URL :
• Other libraries you are using:

What did you expect to happen?
I expect test to fail

What actually happens
Test passes

How to reproduce

        const s1 = spy();
        const s2 = spy();

        s1();
        s2();
        s1();

        assert.callOrder(s2, s1, s2);

[lucasfcosta]

Hi @mir3z, thanks for your issue!

You seem to be right, but the real problem with this issue is semantics.

I've reimplemented the called-in-order core function to take into account both if a spy has been called after the previous one and before the next one. Now your example will not pass because s2 has not been called after s1.

Before the changes I've got here, called-in-order only checked if a passed spy was called before the next one.

However, the calledAfter function I am using in the called-in-order core function has a problem.

The calledBefore checks if the first call of the current spy (this) happened before the last call of the passed spy. This means that if the passed spy has been called after the first time we invoked the current spy, the current spy will be considered to have been called before (this function will return true).

However, the calledAfter function has different semantics. Instead of checking whether or not the current spy (this) has been called after any calls to the passed spy, it checks if the current spy has been called more recently than the passed spy (it means the current spy should have been the last one to be called instead of just being called after any calls to the passed spy).

Given this I think we must refactor the calledAfter function to check if the current spy has been called anytime after the first passed spy invocation.

If we want to keep the old behavior for calledAfter I suggest we rename it to calledLast or something like that.

Thus, this test will start breaking because if we take into account the exact meaning of "spyA being called after spyB" it is true indeed, because even though spyA was not the last one to be called, it was still called after spyB. So we either gotta remove it or start using calledLast for this test.

Btw, this can be considered a breaking change to the API depending on what we were expecting calledAfter to do. If I am correct and you consider this to be a bug we can release this as a fix.

My second option is to implement the correct logic for calledAfter inside called-in-order and stop calling spy.calledAfter. But then I believe the code will not be modular and will become inconsistent.

Let's wait for a maintainer's response so that I can send a PR with changes since I've got both options ready for a git push here in my machine.

[fatso83]

Why oh why do you keep finding all these hard problems, @lucasfcosta? And it seems I always try to read your findings when I am sleep deprived 😪

😉

But yeah, I think I understand what you are saying. Basically try to make the logic in calledBefore/calledAfter symmetric, as apposed to now. I'd say go on this (approach 1).

[lucasfcosta]

I'd really like to know why GitHub has linked all these commits in branches that do not exist anymore in here.