Make calledWith() assertions idempotent

[cincodenada]

Purpose (TL;DR)

Fix bug where checking calledWith() on a spy was not idempotent. Previously, if called repeatedly in such a way that it generated multiple exceptions for a given spy, it would repeatedly quote the called arguments array.

Background (Problem in detail)

The best explainer is the failing test I added before fixing the issue:

it("assert.calledWith message is idempotent", function () {
    this.obj.doSomething("hey");

    this.message("calledWith", this.obj.doSomething, "");
    this.message("calledWith", this.obj.doSomething, "");
    this.message("calledWith", this.obj.doSomething, "");
    assert.contains(
        this.message("calledWith", this.obj.doSomething, ""),
        '"hey"'
    );
});

Which results in this AssertionError:

AssertionError: [assert.contains] Expected 'expected doSomething to be called with arguments \n' +
`\u001b[31m'"\\\\"\\\\\\\\\\\\"\\\\\\\\\\\\\\\\\\\\\\\\\\\\"hey\\\\\\\\\\\\\\\\\\\\\\\\\\\\"\\\\\\\\\\\\"\\\\""'\u001b[0m \u001b[32m''\u001b[0m ` to contain '"hey"'
+ expected - actual

-expected doSomething to be called with arguments
-'"\\"\\\\\\"\\\\\\\\\\\\\\"hey\\\\\\\\\\\\\\"\\\\\\"\\""' ''
+"hey"

As you can see, the argument is quoted recursively four times, resulting in an absurd number of backslashes. This is because the arguments array was modified in place while generating the error message.

Obviously this is pretty edge-case - most of the time error messages will only be generated once, because they will throw and end the test. But there's one instance of working around this bug in the Sinon tests themselves (I removed, the workaround, now that it is fixed), and I ran into this while trying to make improvements to sinon-chai, where I am also asserting based on the contents of the error messages, and ran into unexpected deeply-quoted strings.

Solution

We just escape a copy of the arguments, instead of escaping the arguments in place. Again, since this is usually only called once, this should have no other downstream effects.

How to verify

1. Check out this branch
2. npm install
3. git checkout HEAD^ to get back to the broken state
4. Run the test suite, or npm run test-node -- -f idempotent if you're impatient
5. See that you get an overly-escaped argument in the errors
6. git checkout idempotent-calledWith
7. Run tests again, see that it is no longer over-escaped

Checklist for author

• [] npm run lint passes
• References to standard library functions are cached.

npm run lint doesn't pass, but none of the errors are related to my changes - they're mostly missing JSDoc comments, and those are the only ones in the files that I modified.

Backports

It looks like this was introduced in v9.2.0 with commit 3b1fa42, so this could be backported to 9 and 10 as well - I can open up PRs for that as well once this PR is vetted.

[fatso83]

I had to read over the code three times, concluding it was doing the same as before, before reading your description of the problem and understanding what the fix was! I blame it on my kids😴

Good report and nice, clean fix. Thanks!