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 references on assertions before sending across the wire. #187

Closed
wants to merge 2 commits into from

Conversation

jamestalmage
Copy link
Contributor

We serialize and send copies of assertions errors across the wire on failure.
If assertionError.actual, or assertionError.expected have a circular reference, this causes a problem when process.send converts to JSON.

This replaces serialize-error with destroy-circular. I found one tiny bug with their implementation, but it seems good otherwise.

@jamestalmage
Copy link
Contributor Author

😠

Windows and Node 0.10 are quickly becoming annoying.

@jamestalmage jamestalmage force-pushed the same-with-circular branch 2 times, most recently from 97292fd to 42e8747 Compare November 11, 2015 08:37
@jamestalmage
Copy link
Contributor Author

It looks like AppVeyor is having some issues. Those pass just fine in my Windows VM. It's not making it through npm install on AppVeyor

add test to prove failed assertions do not choke on circular data structures

add test fixture with circular reference (to make sure it makes it across the wire)

switch from serialize-error to destroy-circular
@jamestalmage
Copy link
Contributor Author

What the heck? It was green after rebase! All I did was squash!

@jamestalmage
Copy link
Contributor Author

Crap. Adding the tap-spec reporter should not have made this pass.

We have intermittent test failures on CI that I can not reproduce locally at all.

@jamestalmage
Copy link
Contributor Author

@sindresorhus
I think this is ready for merge.

I can drop the tap-spec commits, but I wanted you to see the brittle test issue I'm having before I do. (and if it fails again when I revert to @2f16758, I may toss my computer out the window).

@sindresorhus
Copy link
Member

👍

@jamestalmage jamestalmage deleted the same-with-circular branch November 21, 2015 23:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants