Improves error message for toHaveBeenCalledWith #48
Conversation
| for(const arg of spy.calls) { | ||
| actualArguments = actualArguments + '- ' + JSON.stringify(arg.arguments) + '\n' | ||
| } | ||
| const message = 'spy was never called with:\n%s.\n\nIt was called with:\n' + actualArguments |
There was a problem hiding this comment.
I tried using the %s place holder instead of the concatenation here, but it did not interpreted the \n, outputting it literally without breaking the line.
There was a problem hiding this comment.
One of the drawbacks to computing this message ahead of time like this is that we may not actually need it. For example, if the assertion passes, we will have done all this computation for nothing (i.e. we won't ever actually have to show this message). That's the great thing about the way our assert function works–it doesn't do any work at all if the assertion passes. It would be nice to keep this behavior so we don't slow tests down.
Also, please keep with existing code style everywhere (spacing, etc.)
|
Hmm, not sure why the build failed. I just restarted it. |
|
I have updated PR to reflect your comments and rebased commits. Please take a look when you have a moment. |
| ) | ||
| const genMessage = () => { | ||
| const actualArguments = spy.calls | ||
| .map(call => `- ${JSON.stringify(call.arguments)}`) |
There was a problem hiding this comment.
json doesn't support functions, or null, and a few other JS types - is this the best method to represent the items?
you may want to look into the object-inspect module here.
|
@khronnuz there's still a merge commit in the history - would you mind rebasing this on latest master? |
|
@ljharb done. |
|
Thanks! It might be nice to add some tests too, sorry I didn't mention that earlier :-/ |
|
@ljharb I've added a test case. Please take a look if you are happy with the test case or need more examples. Also, please check if you like how I've removed the indentation from the expected message here 0a11d3b#diff-c0516c644bb1bfc36e0180d5855333e8R51 |
|
Seems good! I'm not sure if I like the indentation approach but I think it's totally reasonable for now. |
|
Is this enough? I feel like you need to get a complete-ish history of calls (may have been called multiple times) with detailed output of each. For instance, maybe you want to do: expect(someReduxStore.dispatch).toHaveBeenCalledWith({
type: 'ACTION',
mega: {death: {deep: [args, from, an, api: {of: stuff
});I ran a similar issues with Chai-spies issue is here: chaijs/chai-spies#29 This still wasn't quite enough, so I wrote my own dirty patch. Dirty because console logging isn't exactly kosher chai, and npm is not friendly about patching. |
|
In general I think our error messages leave a lot to be desired, but this is an ok first pass. I'd like to take a hard look at the way we're generating error messages soon and rework it to be more flexible. @khronnuz Would you mind rebasing on current master? Thanks. |
|
@mjackson done. |
|
Are there any plans to complete this PR? This would be a huge improvement over the current error messages. |
Often when trying test using spied on functions, I find it hard to debug because you cannot tell if the function was called at all or which arguments it was called with. In this PR I try to address both issues by:
spy was never called with xxx.Again, thanks for the library and I am happy to address any issues you have with my proposed changes.