fix circular objects in json reporter. Closes #2433

[jeversmann]

In #2433, @boneskull recommended using jsonStringify() from ./lib/utils, but that function isn't exported by the utils module, and more importantly it isn't actually circular-reference safe.

Instead, I took the custom replacer from this StackOverflow post and added it to the reporter's invocation of JSON.stringify.

The test for this issue is mostly a copy of the 1st json reporter test. It has fewer asserts because there's no way to test for the change to the object that is made by the replacer, but the test does fail without the additional code.

[Munter]

Nice one. I'm not a big fan of the duplication though. Should this be put in its own reusable util function?

[jeversmann]

There's no duplication here? This one reporter is the only spot I'm aware of with this problem.

[ScottFreeCode]

IMHO if the version in our util component isn't actually circular-safe we should fix it (or make it a reference to a dependency that's safe), and if it isn't exported it should be, so other components such as reporters can just call on the util function.

[boneskull]

hmm, yes, it seems canonicalize() is circular-safe, which is what I thought I was pointing you to. oops. but that function is for displaying diffs.

[boneskull]

It would seem a public API to output JSON--while avoiding circular references--would be helpful to reporter implementations. That's tangential to #2433.

Looking more closely at the JSON reporter, it already purports to kill circular references. The bug is that it doesn't actually succeed in doing this.

The JSON stream reporter works because it doesn't attempt to "stringify" an Error--it just yanks the message and stack properties out.

I think what this PR should do is actually very fine-tuned to this bug. @jeversmann Can you instead modify errorJSON() or clean() to skip objects it's already seen?

I don't know what the "best" implementation would look like here, but we should retain as much of the Error as is feasible. We shouldn't take the easy way out and do what the JSON stream reporter does. That would would be a breaking change, and not a bug fix.

[jeversmann]

I moved the circular object logic into a helper function that's called in clean so it behaves according to the comment above it.

The 'gotcha' here is that Error objects don't JSON.stringify correctly, so formatting test.err is a two-step process which might behave strangely if a test throws an object that isn't an instanceof Error but contains one or more Errors as children. The suggested fix seems to be defining Error.prototype.toJSON but that seems like overkill unless the current method causes problems for anyone.

[ScottFreeCode]

...might behave strangely if a test throws an object that isn't an instanceof Error but contains one or more Error s as children.

I thought Mocha doesn't accept non-Error failures in the first place?

[jeversmann]

The problem in #2433 deals specifically with an object whose prototype chain contains Error, but the test I wrote to provoke the circular parsing problem just throws a regular object with a circular reference in it.

I can modify the test to build the thrown object from an Error but the code to handle printing it shouldn't change.

[ScottFreeCode]

Yeah, I'm not too concerned with having a test that focuses on the circular reference thing without worrying about rigging up a "real" Error; I just wasn't sure how the comment about handling the case of Error as a member of non-Error objects was relevant. Sorry for any confusion!

[stale]

I am a bot that watches issues for inactivity.
This issue hasn't had any recent activity, and I'm labeling it stale. In 14 days, if there are no further comments or activity, I will close this issue.
Thanks for contributing to Mocha!

[jeversmann]

@boneskull This is still labeled as pr-needs-work, but I think I've addressed the feedback.
The bot is right that this PR has become stale, but #2433 is still around, so I'd rather merge this in.

[boneskull]

@jeversmann thanks for the heads-up. someone will take a look at this, maybe even me!

[xyleen]

Hey! I've stumbled upon this situation myself and the proposed commits seem to resolve the issue for me.

Do you plan on merging this pull request sometime? Is this PR missing anything that prevents it from being mergable?

@boneskull - could you address those questions? Thanks!

[boneskull]

continued in #3318. thanks

[boneskull]

landed in 741b0bd