Better error output for toHaveBeenCalledWith

[ericclemmons]

First, great project! I'm sure you saw my tweet of praise:

https://twitter.com/ericclemmons/status/681615901343399936

toHaveBeenCalledWith

Anyway, having replaced sinon with this, I've noticed that this very readable call...

expect(this.say).toHaveBeenCalledWith("foo");

...yields an error with only half of the picture. There's no telling what it was called with, or never called at all:

Error: spy was never called with [ 'foo' ]

toEqual

However, explicitly using toEqual is much more useful:

expect(this.say.calls[0].arguments).toEqual(["foo"]);

[
-  "bar"
+  "foo"
]

Proposal

I'd recommend mimicking the output of toEqual.

When the spy was called & matched.

No changes.

(Patch) When the spy was never called.

"say" was never called

(Patch) Show all calls.

"name" was called N times without ["foo"]:

1 with ["bar"]

2 with [undefined]

...

(Breaking Feature) Expect the assertion to always reference the last call (e.g. spy.calls[spy.calls.length -1].

This is a suggestion for a possible future release that's largely unrelated to this issue.

"name" was last called with ["bar"], expected ["foo"].

What I like about this approach is that it works great for repeated assertions:

const query = function(params) {
  return axios.get("/api", { params });
};

var spy = expect.spyOn(axios, "get");

query({ foo: "foo" });
expect(spy).toHaveBeenCalledWith("/api", { foo: "foo" });
query({ bar: "bar" });
expect(spy).toHaveBeenCalledWith("/api", { bar: "bar" });

I only list it here because improving the usability of toHaveBeenCalledWith will lend itself to more use :)

---

What do you think @mjackson?

[mjackson]

Hey @ericclemmons! Thanks for the input. I'm not sure if you saw #48 or not, but it seems you're not the only one who wants better error messages from toHaveBeenCalledWith. I like your suggestions.

Re: only referencing the last call - I think that makes a lot of sense. I can't think of any real scenario where I call a method more than once and then want to assert that one of the calls had a certain set of args. I'd be fine with making that the default behavior. One big upside is that by simplifying we can avoid printing the arguments that were used in multiple calls, which is nice. And we can use the same mechanism as toEqual for printing the diff between the actual and expected args.

Yes, this would be a breaking change. But I'm ok with that. We've been on 1.x for a while now, and this behavior is subtle enough that I doubt it'll break many people's tests.

[ericclemmons]

Whoops! I apologize for not catching #48.

How would you like this to be tackled? Does #48 still apply, or does this subsume it with a new PR?

[mjackson]

I'd say let's get a new PR going. You want to take a shot @ericclemmons?

[ericclemmons]

@mjackson I'll take a stab at it. Thanks for being so receptive!

[ericclemmons]

Taking a stab at this today, but wanted to OK having a separate toHaveBeenCalledWith-test.js file for these.

Rational being, from a cursory inspection, createSpy and spyOn tests should largely ensure that spy objects have been created successfully. From that point on, it's really enforcing the behavior of the individual assertions within Expectation.js.

If you'd prefer I not do this, I'll bake the tests into createSpy-test.js and spyOn-test.js.

Let me know, thanks!

[mjackson]

Ya, a separate file would be great.
On Wed, Dec 30, 2015 at 1:39 PM Eric Clemmons [email protected]
wrote:

Taking a stab at this today, but wanted to OK having a separate
toHaveBeenCalledWith-test.js file for these.

Rational being, from a cursory inspection, createSpy and spyOn tests
should largely ensure that spy objects have been created successfully. From
that point on, it's really enforcing the behavior of the individual
assertions within Expectation.js.

If you'd prefer I not do this, I'll bake the tests into createSpy-test.js
and spyOn-test.js.

Let me know, thanks!

—
Reply to this email directly or view it on GitHub
#60 (comment).

[ericclemmons]

I'm...gonna have to put a pin in this or open a neighboring issue in my fork. Currently deadlocked getting tests going:

  npm test

> [email protected] test /Users/Eric/Projects/ericclemmons/expect
> npm run lint && karma start


> [email protected] lint /Users/Eric/Projects/ericclemmons/expect
> eslint modules

30 12 2015 22:02:03.421:WARN [karma]: No captured browser, open http://localhost:9876/
30 12 2015 22:02:03.429:WARN [karma]: Port 9876 in use
30 12 2015 22:02:03.430:INFO [karma]: Karma v0.13.16 server started at http://localhost:9877/
30 12 2015 22:02:03.433:INFO [launcher]: Starting browser Chrome
30 12 2015 22:03:03.437:WARN [launcher]: Chrome have not captured in 60000 ms, killing.
30 12 2015 22:03:05.444:WARN [launcher]: Chrome was not killed in 2000 ms, sending SIGKILL.
30 12 2015 22:03:07.451:WARN [launcher]: Chrome was not killed by SIGKILL in 2000 ms, continuing.

I'll check on this again tomorrow...

[mjackson]

Hmm, looks like you already have an instance of Karma running. It may be confused.

[alanquigley]

Hi, thanks for the great library, wondering if a fix for this issue is on the horizon?

[ericclemmons]

If there were a way to run the tests purely in Node (and let CI do the integration tests), I could pick this back up.

Me thinks that it doesn't like that I have both Chrome & Chrome Canary, but I also haven't used Karma since my despicable Angular 1 days :)

[ljharb]

fwiw, I have both Chrome stable and canary installed and npm test works fine.

[ericclemmons]

Can you take over this then?

[ljharb]

@ericclemmons could you link me to the PR you have in progress (or create one if there isn't one yet)?

[everson]

Your proposal looks good.

I have been using expect my branch and it serving me good. Although I agree that most times we want to see only last call, I would like to see a clear information that it is checking only the last call. Perhaps even a different name e.g. toHaveBeenLastlyCalledWith.

I have updated #48 so that it is in sync with master and linting passes. I've been using expect from this branch for a while. (did not test after merging master).