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

When with fails, report not only the expected, but the actual. #29

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ExploreMqt
Copy link

No description provided.

@keithamus
Copy link
Member

Hey @ExploreMqt thanks for the pr

You can use #{act} instead, and then pass it as another argument, which is more the chai convention. An example

@ExploreMqt
Copy link
Author

Thanks. Will do

-----Original Message-----
From: "Keith Cirkel" [email protected]
Sent: ‎7/‎11/‎2015 7:25 AM
To: "chaijs/chai-spies" [email protected]
Cc: "Jim Argeropoulos" [email protected]
Subject: Re: [chai-spies] When with fails, report not only the expected, butthe actual. (#29)

Hey @ExploreMqt thanks for the pr
You can use #{act} instead, and then pass it as another argument, which is more the chai convention. An example

Reply to this email directly or view it on GitHub.

@keithamus
Copy link
Member

Sorry to be a pain @ExploreMqt - could you also add some tests for this? It'd be nice to have some tests to back up the code is doing what it is meant to. Let me know if you need help with this.

@ExploreMqt
Copy link
Author

You are not a pain. I was just at a loss as to where to put said test

-----Original Message-----
From: "Keith Cirkel" [email protected]
Sent: ‎7/‎24/‎2015 5:09 PM
To: "chaijs/chai-spies" [email protected]
Cc: "Jim Argeropoulos" [email protected]
Subject: Re: [chai-spies] When with fails, report not only the expected, butthe actual. (#29)

Sorry to be a pain @ExploreMqt - could you also add some tests for this? It'd be nice to have some tests to back up the code is doing what it is meant to. Let me know if you need help with this.

Reply to this email directly or view it on GitHub.

@keithamus
Copy link
Member

Here all all the .with tests, some of them (like this one) look at the failure error message, and test what it says. You'd just need to extend these with what you'd expect your new error messages to say. So rather than

  }).should.throw(chai.AssertionError, /have been called with/);

You should write something like:

  }).should.throw(chai.AssertionError, /have been called with [4] but got [[1], [2], [3]]/);

@keithamus
Copy link
Member

Hey @ExploreMqt do you think you will have some time available to update this?

@ExploreMqt
Copy link
Author

I haven't figured out a satisfactory solution for myself. So not at this point

-----Original Message-----
From: "Keith Cirkel" [email protected]
Sent: ‎10/‎13/‎2015 3:14 PM
To: "chaijs/chai-spies" [email protected]
Cc: "Jim Argeropoulos" [email protected]
Subject: Re: [chai-spies] When with fails, report not only the expected, butthe actual. (#29)

Hey @ExploreMqt do you think you will have some time available to update this?

Reply to this email directly or view it on GitHub.

@robcolburn
Copy link
Contributor

@ExploreMqt,
I had similar issues, but a went further. I wonder if you could review my solution ( #52 )

@robcolburn
Copy link
Contributor

@keithamus,
I tried to go down the ${act} route but found it too limiting. Once you've passed the buck to ${act} you can give it guidance and it ultimately results which are less than helpful. Our ${act} isn't singular here, instead we are inspecting a history of calls to this function. Since a spied-on function may have been called several times, we are really asking if has been called "at least one" (default), or "every time" (with .always modifier.

In digging around I found many examples in chai's own code-base, as well as other plugins chai-properties that directly call _.inspect. This turns out to be helpful too because we can format the call history instead of just calling it an array (which is confusing in an output).

I wonder if you can review my solution here #52

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants