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

Fix circular JSON for console reporter #421

Merged
merged 1 commit into from Aug 10, 2015

Conversation

@Marsup
Copy link
Member

Marsup commented Aug 9, 2015

Lab crashed silently with circular JSON in deep expectations.

@Marsup Marsup added the bug label Aug 9, 2015
@@ -2,6 +2,7 @@

var Tty = require('tty');
var Diff = require('diff');
var StringifySafe = require('json-stringify-safe');

This comment has been minimized.

Copy link
@hueniverse

This comment has been minimized.

Copy link
@Marsup

Marsup Aug 9, 2015

Author Member

That's not what I want. Hoek only provides an error string instead, it's not because it's circular it cannot be diffed.

@@ -143,8 +144,8 @@ internals.Reporter.prototype.end = function (notebook) {
if (test.err.actual !== undefined &&
test.err.expected !== undefined) {

var actual = JSON.stringify(test.err.actual, null, 2);
var expected = JSON.stringify(test.err.expected, null, 2);
var actual = StringifySafe(test.err.actual, null, 2);

This comment has been minimized.

Copy link
@geek

geek Aug 10, 2015

Member

Thoughts on falling back to StringifySafe if the normal stringify fails? (moll/json-stringify-safe#4)

I know we aren't super concerned with perf in testing, but it does seem reasonable to only use StringifySafe when needed.

This comment has been minimized.

Copy link
@Marsup

Marsup Aug 10, 2015

Author Member

@geek stringify safe internally uses JSON.stringify with a custom replacer, not sure it's worth doing part of the job twice (worst case scenario), anyway would you mind updating your benchmarks with the newer version ? Note that it can still be hugely improved (see my whining here about it moll/json-stringify-safe#12).

This comment has been minimized.

Copy link
@geek

geek Aug 10, 2015

Member

Nice, thank you.

@geek geek added this to the 5.15.1 milestone Aug 10, 2015
geek added a commit that referenced this pull request Aug 10, 2015
Fix circular JSON for console reporter
@geek geek merged commit 88c2970 into hapijs:master Aug 10, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Marsup Marsup deleted the Marsup:circular-console branch Dec 18, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.