expect('word$').to.equal('xxx') failure message broken

[jurko-gospodnetic]

Executing expect('word$').to.equal('xxx') will give a broken failure message:

AssertionError: expected 'word to equal 'xxx' to equal 'xxx'

The final $ sign seems to be used like some internal placeholder and expanded incorrectly.

[keithamus]

Hey @jurko-gospodnetic thanks for the issue.

Well, this was a tricky one to figure out! The offending lines are here; https://github.com/chaijs/chai/blob/master/lib/chai/utils/getMessage.js#L45-L48. The reason this is happening is because String.prototype.replace gives special meaning to $ within its second argument. Specifically, $1 - $99, $', $ ,$&and$$are all special substrings which reflect parts of the Regular Expression. When we inspect a String (usingchai.util.inspect`) it returns it back wrapped in quotes:

> chai.util.inspect('foo$')
'`\foo$\'"

Of course, as I mentioned before, $' is a special substring, which reflects part of the Regular Expression. Here is an example of the problem fully reduced:

'x'.replace('x', '$\'')

You'd expect that to equal $' but instead it returns `` (an empty string).

The Fix

Okay, so I think the easiest way to fix this is to subvert the String behaviour, and give the replace function a different argument. We can give it a function that returns a String, and everything works exactly as you'd expect. So I'd recommend changing L45-48 from:

  msg = msg
    .replace(/#{this}/g, objDisplay(val))
    .replace(/#{act}/g, objDisplay(actual))
    .replace(/#{exp}/g, objDisplay(expected));

To:

  msg = msg
    .replace(/#{this}/g, function () { return objDisplay(val) })
    .replace(/#{act}/g, function () { objDisplay(actual) })
    .replace(/#{exp}/g, function () { objDisplay(expected) });

This should fix the problem outright. As part of this, I would expect to see tests as well - our test suite for getMessage is here; I'd expect to see the developer who PRs this add the extra test cases demonstrating that the resulting message works well with $s in the expected/actual strings.

@jurko-gospodnetic if you want to make a PR to fix this problem, that'd be awesome. Doing so will net you a place in our amazing hall of fame where all of our great contributors go 🤘.

[jurko-gospodnetic]

ROFL... omg... what an amazing developer pitch... 👍

Ok... can't resist - I'll take a look now. 😆

[jurko-gospodnetic]

Got pull request #570 up for this.

[jurko-gospodnetic]

Might want to remove the pull-request-wanted label as well.

[keithamus]

I keep them around for posterity - we can use them to track the issues that wanted a pr and got one 😄