Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

assert.propEqual not guarded against circular structures #411

Open
amb26 opened this Issue Feb 4, 2013 · 3 comments

Comments

Projects
None yet
5 participants
Contributor

amb26 commented Feb 4, 2013

As a result of #343 we got a new "propEqual" method which is usefully capable of comparing object trees in a "constructor-blind" manner, using a weaker semantic than QUnit.deepEqual. However, this method is not up to the standard of the rest of the framework - in particular, QUnit.deepEqual itself offers support for

  1. correctly comparing cyclic structures without bombing the stack (#100 - recently improved by #397)
  2. not being confused by primitive arguments such as strings - these are compared properly and highlighted in the difference output.

"propEqual" was implemented using a utility function "objectValues" which performs a clone of the arguments before they are dispatched to QUnit.deepEqual, and it is deficiencies in this algorithm which are responsible for deficiencies in propEqual as compared to deepEqual.

  1. is caused by the cloning operation itself bombing out on cyclic structures, and
  2. is caused by the lack of a check for primitive arguments before copying - strings are copied as if they were arrays. The comments at the head of objectValues are clear about the latter restriction, but this contract limitation is not clear when passed up to propEqual itself.

I enclose a screenshot of the results of comparing two string using propEqual. deepEqual by contrast shows a suitable result.
propEqualStrings

Owner

Krinkle commented May 7, 2013

  1. The circular structure should be detected and guarded against, indeed.
  2. Primitive values being compared like objects is by design. Perhaps it should throw a warning instead. But treating them as strings instead of objects would be masking an error by the test author. I'd like to get this removed from deepEqual as well.

@ghost ghost assigned Krinkle May 7, 2013

Owner

jzaefferer commented Nov 6, 2014

@Krinkle want to look into fixing this again?

@Krinkle Krinkle self-assigned this Nov 6, 2014

Contributor

BraulioVM commented Mar 9, 2015

@Krinkle I started working on a patch for this, I hope you don't mind (I just wanted to contribute fixing a bug and saw that this one was open yet).

I started implementing a fix considering what Krinkle said in his first comment in this issue, but while I was writing tests for the new propEqual method I realised that I was not actually sure about what the method should return in certain cases. For example:

function fn1() { return "fn1"; }
function fn2() { return "fn2"; }
var first = { a : fn1 };
var second = { a: fn2 };


assert.propEqual(first, second, "Should they be equal?");
assert.propEqual(fn1, fn2, "Should they be equal here?");

Using the old propEqual, both would be right. I am not sure we wanted to keep the same behaviour as that may not be what the test author would expect. Should we keep it?

Furthermore, Krinkle said we should throw a warning whenever the test author supplied a primitive object. I have been inspecting the codebase and can't seem to find what the best mechanism would be for throwing such a warning. Any ideas?

Thank you very much

@Krinkle Krinkle added the assert label Oct 30, 2015

@Krinkle Krinkle removed the in progress label Apr 3, 2017

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