This repository has been archived by the owner. It is now read-only.

assert.deepEqual() does not stop on circular references #207

Closed
akidee opened this Issue Jul 14, 2010 · 9 comments

Comments

Projects
None yet
3 participants
@akidee

akidee commented Jul 14, 2010

The script will not output 'All OK':

var b = {};
b.b = b;

var c = {};
c.b = c;

assert.deepEqual(b, c);

sys.puts('All OK');

@strager

This comment has been minimized.

Show comment Hide comment
@strager

strager Jul 22, 2010

I had the same problem in my test suite. I edited node.js to fix the problem : http://github.com/strager/node/commit/56dbd80eb883641e0fa734bd9f91be5d680feb54

strager commented Jul 22, 2010

I had the same problem in my test suite. I edited node.js to fix the problem : http://github.com/strager/node/commit/56dbd80eb883641e0fa734bd9f91be5d680feb54

@akidee

This comment has been minimized.

Show comment Hide comment
@akidee

akidee Jul 23, 2010

Better: set a non-enumerable property $id on the object, and add the id to an object that belongs to the equal call. So the check, if an object was already checked, will be faster.

akidee commented Jul 23, 2010

Better: set a non-enumerable property $id on the object, and add the id to an object that belongs to the equal call. So the check, if an object was already checked, will be faster.

@strager

This comment has been minimized.

Show comment Hide comment
@strager

strager Jul 23, 2010

@akidee, I think this would cause problems. I prefer a "hands-off" approach; don't touch the object, but just examine it. If you can do some performance analysis and it proves your method to be significantly faster or my method to be very flawed, we could reconsider, but I prefer correctness and fool-proof-ness over performance when writing test cases. (I REALLY don't want bugs or gatcha's in the assertion framework I'm using!)

strager commented Jul 23, 2010

@akidee, I think this would cause problems. I prefer a "hands-off" approach; don't touch the object, but just examine it. If you can do some performance analysis and it proves your method to be significantly faster or my method to be very flawed, we could reconsider, but I prefer correctness and fool-proof-ness over performance when writing test cases. (I REALLY don't want bugs or gatcha's in the assertion framework I'm using!)

@akidee

This comment has been minimized.

Show comment Hide comment
@akidee

akidee Jul 23, 2010

I appreciate a "hands-off" approach. I use my method for fast validation of objects. But you are right: in a test case you want to be fool-proof. So it would be good to get your patch pushed to the main branch.

akidee commented Jul 23, 2010

I appreciate a "hands-off" approach. I use my method for fast validation of objects. But you are right: in a test case you want to be fool-proof. So it would be good to get your patch pushed to the main branch.

@strager

This comment has been minimized.

Show comment Hide comment
@strager

strager Jul 23, 2010

Alright ... Do I have to wait for a committer to see this or should I e-mail or something? (I don't github that much.)

strager commented Jul 23, 2010

Alright ... Do I have to wait for a committer to see this or should I e-mail or something? (I don't github that much.)

@akidee

This comment has been minimized.

Show comment Hide comment
@akidee

akidee Jul 23, 2010

You have made a fork of node, so simply send ry a pull request (button on the top right hand side)

akidee commented Jul 23, 2010

You have made a fork of node, so simply send ry a pull request (button on the top right hand side)

@akidee

This comment has been minimized.

Show comment Hide comment
@akidee

akidee Nov 8, 2010

Any success?

akidee commented Nov 8, 2010

Any success?

@ry

This comment has been minimized.

Show comment Hide comment
@ry

ry Mar 30, 2011

appears to be working in current version v0.4.3. Reopen if broke.

ry commented Mar 30, 2011

appears to be working in current version v0.4.3. Reopen if broke.

@ry

This comment has been minimized.

Show comment Hide comment
@ry

ry Mar 30, 2011

Add test for circular refs in deepEquals

Closed by 6394ba2.

ry commented Mar 30, 2011

Add test for circular refs in deepEquals

Closed by 6394ba2.

@ry ry closed this Mar 30, 2011

coolaj86 pushed a commit that referenced this issue Apr 15, 2011

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.