Don't follow prototype chain in diff

[loldrup]

Hi

When a test case fails, the 'actual' and 'expected' values gets logged to screen. When these values are objects with many properties/functions on their prototype chains, the logged text gets huge and (always?) pretty irrelevant. Is there a way to tell QUnit not to follow prototype chains?

Here's an example of a way too big logging:

"Expected:

{
  "add": function add( a, b ){
    [code]
  },
  "addScalar": function addScalar( a ){
    [code]
  },
  "addScaledVector": function addScaledVector( a, b ){
    [code]
  },
  "addVectors": function addVectors( a, b ){
    [code]
  },
  "angleTo": function angleTo( a ){
    [code]
  },
  "applyAxisAngle": function applyAxisAngle( a, b ){
    [code]
  },
  "applyEuler": function applyEuler( a ){
    [code]
  },
  "applyMatrix3": function applyMatrix3( a ){
    [code]
  },
  "applyMatrix4": function applyMatrix4( a ){
    [code]
  },
  "applyProjection": function applyProjection( a ){
    [code]
  },
  "applyQuaternion": function applyQuaternion( a ){
    [code]
  },
  "ceil": function ceil(){
    [code]
  },
  "changeBasis": function changeBasis( a, b, c, d ){
    [code]
  },
  "clamp": function clamp( a, b ){
    [code]
  },
  "clampLength": function clampLength( a, b ){
    [code]
  },
  "clampScalar": function clampScalar( a, b ){
    [code]
  },
  "clone": function clone(){
    [code]
  },
  "constructor": function Vector3( a, b, c ){
    [code]
  },
  "copy": function copy( a ){
    [code]
  },
  "cross": function cross( a, b ){
    [code]
  },
  "crossVectors": function crossVectors( a, b ){
    [code]
  },
  "distanceTo": function distanceTo( a ){
    [code]
  },
  "distanceToManhattan": function distanceToManhattan( a ){
    [code]
  },
  "distanceToSquared": function distanceToSquared( a ){
    [code]
  },
  "divide": function divide( a ){
    [code]
  },
  "divideScalar": function divideScalar( a ){
    [code]
  },
  "dot": function dot( a ){
    [code]
  },
  "equals": function equals( a ){
    [code]
  },
  "floor": function floor(){
    [code]
  },
  "fromArray": function fromArray( a, b ){
    [code]
  },
  "fromAttribute": function fromAttribute( a, b, c ){
    [code]
  },
  "fromBufferAttribute": function fromBufferAttribute( a, b, c ){
    [code]
  },
  "getColumnFromMatrix": function getColumnFromMatrix( a, b ){
    [code]
  },
  "getComponent": function getComponent( a ){
    [code]
  },
  "getPositionFromMatrix": function getPositionFromMatrix( a ){
    [code]
  },
  "getScaleFromMatrix": function getScaleFromMatrix( a ){
    [code]
  },
  "isVector3": true,
  "length": function length(){
    [code]
  },
  "lengthManhattan": function lengthManhattan(){
    [code]
  },
  "lengthSq": function lengthSq(){
    [code]
  },
  "lerp": function lerp( a, b ){
    [code]
  },
  "lerpVectors": function lerpVectors( a, b, c ){
    [code]
  },
  "max": function max( a ){
    [code]
  },
  "min": function min( a ){
    [code]
  },
  "multiply": function multiply( a, b ){
    [code]
  },
  "multiplyScalar": function multiplyScalar( a ){
    [code]
  },
  "multiplyVectors": function multiplyVectors( a, b ){
    [code]
  },
  "negate": function negate(){
    [code]
  },
  "normalize": function normalize(){
    [code]
  },
  "project": function project( a ){
    [code]
  },
  "projectOnPlane": function projectOnPlane( a ){
    [code]
  },
  "projectOnVector": function projectOnVector( a ){
    [code]
  },
  "reflect": function reflect( a ){
    [code]
  },
  "rejectOnVector": function rejectOnVector( a ){
    [code]
  },
  "round": function round(){
    [code]
  },
  "roundToZero": function roundToZero(){
    [code]
  },
  "set": function set( a, b, c ){
    [code]
  },
  "setComponent": function setComponent( a, b ){
    [code]
  },
  "setEulerFromQuaternion": function setEulerFromQuaternion(){
    [code]
  },
  "setEulerFromRotationMatrix": function setEulerFromRotationMatrix(){
    [code]
  },
  "setFromCylindrical": function setFromCylindrical( a ){
    [code]
  },
  "setFromMatrixColumn": function setFromMatrixColumn( a, b ){
    [code]
  },
  "setFromMatrixPosition": function setFromMatrixPosition( a ){
    [code]
  },
  "setFromMatrixScale": function setFromMatrixScale( a ){
    [code]
  },
  "setFromSpherical": function setFromSpherical( a ){
    [code]
  },
  "setLength": function setLength( a ){
    [code]
  },
  "setScalar": function setScalar( a ){
    [code]
  },
  "setX": function setX( a ){
    [code]
  },
  "setY": function setY( a ){
    [code]
  },
  "setZ": function setZ( a ){
    [code]
  },
  "sub": function sub( a, b ){
    [code]
  },
  "subScalar": function subScalar( a ){
    [code]
  },
  "subVectors": function subVectors( a, b ){
    [code]
  },
  "toArray": function toArray( a, b ){
    [code]
  },
  "transformDirection": function transformDirection( a ){
    [code]
  },
  "unproject": function unproject( a ){
    [code]
  },
  "x": 0,
  "y": 0,
  "z": 0
}"

[trentmwillis]

Currently, there is no way to prevent comparison of prototypes. In theory, however, we should be able to optimize our diffs and equality checks by only check an object's "own" properties if it is being compared with an object that has the same constructor (and therefore prototype chain).

I'd be open to a PR with the above.

[loldrup]

Don't you mean: IF it is being compared with an object that has the same constructor THEN we should only check an object's "own" properties

[trentmwillis]

Yep. Same thing, just different ordering of words.

[Krinkle]

Just for the record, QUnit's deep comparator does not needless follow prototype chains. It does a recursive comparison on each property. Only when a property isn't equal will it traverse the referenced object. This means that prototype chains, naturally, are not followed if they are the same.

However, this issue is specific to the logging of the results once a difference is determined. It seems in that case we always dump the full object, regardless of how deep the comparator had to traverse.

The "Diff" already makes it easier to spot the difference, but we still dump an entire object: Expected, Result, Diff.

QUnit.test('example', function (assert) {
  let i = 1000;
  let list = [];
  while(i--) { list.push(Math.random()); }
  assert.deepEqual(
    { a: 1, b: 3, list: list  },
    { a: 1, b: 2, list: list }
  );
});

Current display:

Adding a limit the "diff context" of maybe 5-10 lines should be an easy win.

--- Result
+++ Expected
  {
    "a": 1,
-   "b": 3,
+   "b": 2,
    "list": [
      0.14316515211776704,
      0.9565818836785518,

If we only displayed the diff, then I'd be worried about whether it is still easy to understand where the difference was found in the values. However, we already print the full objects in the "Expected" and "Result" section, that shouldn't be an issue.

We might actually want to consider somehow collapsing the non-Diff sections by default, or at least limiting their height on-screen (max-height with overflow-y auto? - undo height limit on hover/focus?)

Not sure what to do for the CLI reporter. Perhaps hide Expected/Actual by default there, and only display the diff?

Just a few ideas :)

[loldrup]

You suggest some ways for limiting the printed output. May I suggest that you simply never print prototypal properties, unless the difference was found on the prototype chain?

Another question: what's the rationale for ever including the prototype chain in the equality comparison? Prototype objects seems to me to be something that is decidedly external to an object. When we ask QUnit to compare two bananas, when will it then ever be relevant to also compare the two monkeys holding them, and the two jungles in which the monkeys live? Even if relevant use cases could be found, would it then still be a good idea to force this behaviour on all other use cases as well?

Den 07/07/2017 6.40 AM skrev "Timo Tijhof" notifications@github.com:

Just for the record, QUnit's deep comparator does not needless follow prototype chains. It does a recursive comparison on each property. Only when a property isn't equal will it traverse the referenced object. This means that prototype chains, naturally, are not followed if they are the same.

However, this issue is specific to the logging of the results once a difference is determined. It seems in that case we always dump the full object, regardless of how deep the comparator had to traverse.

The "Diff" already makes it easier to spot the difference, but we still dump an entire object – three times: Expected, Result, Diff.

QUnit.test('example', function (assert) { let i = 1000; let list = []; while(i--) { list.push(Math.random()); } assert.deepEqual( { a: 1, b: 3, list: list }, { a: 1, b: 2, list: list } ); });

Current display: [image: screen shot 2017-07-06 at 21 29 58] https://user-images.githubusercontent.com/156867/27943242-4fad79ce-6292-11e7-8d3f-8f26ba7f852e.png

Adding a limit the "diff context" of maybe 5-10 lines should be an easy win.

--- Result+++ Expected { "a": 1,- "b": 3,+ "b": 2, "list": [ 0.14316515211776704, 0.9565818836785518,

If we only displayed the diff, then I'd be worried about whether it is still easy to understand where the difference was found in the values. However, we already print the full objects in the "Expected" and "Result" section, that shouldn't be an issue.

We might actually want to consider somehow collapsing the non-Diff sections by default, or at least limiting their height on-screen (max-height with overflow-y auto? - undo height limit on hover/focus?)

Not sure what to do for the CLI reporter. Perhaps hide Expected/Actual by default there, and only display the diff?

Just a few ideas :)

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/qunitjs/qunit/issues/1209#issuecomment-313585174, or mute the thread https://github.com/notifications/unsubscribe-auth/AAP_NK1a5smyvYgiMW5p80psOKiJNKACks5sLbafgaJpZM4OO6qZ .

[loldrup]

Krinkle wrote: "The "Diff" already makes it easier to spot the difference"

For numeric values, I find this not to be true, because the diff will start mixing the individual digits of each number. Because of this, I rarely use the 'diff' display (as I mostly work with numbers). I instead look at the raw 'actual' and 'expected' values. Therefore, I would be sad if they were to be automatically hidden or cropped from now on. Would it make more sense to display the algebraic numeric difference? (as obtained by subtracting one number from the other)

Jon wrote: "May I suggest that you simply never print prototypal properties, unless the difference was found on the prototype chain?"

Allow me to elaborate on this statement: IF both 'expected' and 'actual' has a prototype, traverse them and show the individual differences that might be between them. IF there is no difference, I guess we can simply omit displaying both of the prototypes? IF only one of the objects has a prototype: As I understand it, this situation is currently interpreted thus: "Well, since one of the objects didn't have a prototype, the entire prototype chain of the other object can be considered to be one big difference on its own. So we display the whole thing!" While I understand that it can be appropriate to generate this view (because, well, there actually is a difference between the two objects here), perhaps in this case, the view of the prototype could be hidden by default..? A single line could note the user that the prototype is there and is "different". Then, when clicking that line, it expands to show the full prototype chain.

[loldrup]

Hmm, I have just learned(1) that even objects created from object literals have prototype objects (are there any object that doesn't have a prototype?)

This means that it will practically never happen that only one object has a prototype. This leaves us with two cases:

A. the two objects have similar prototypes. B. the two objects have differing prototypes.

Thus my preference boils down to this: the display of any differences in the prototype chain should default to being collapsed. One can then click it to expand its view.

(1): "The prototypes of the clones are always Object.prototype, which is the default for objects created via object literals:

Object.getPrototypeOf(clone1) === Object.prototype true Object.getPrototypeOf(clone2) === Object.prototype true Object.getPrototypeOf({}) === Object.prototype true" http://2ality.com/2016/10/rest-spread-properties.html

[rwjblue]

are there any object that doesn't have a prototype?

Yes. Object.create(null) returns an object without a prototype.

[Krinkle]

what's the rationale for ever including the prototype chain in the equality comparison?

Short answer: For equality, we don't. QUnit's deepEqual only recurses into a property if the property is not strictly equal to its pair in the other object. For example, if a.constructor === b.constructor, then QUnit will stop traversing there as far as the equality comparison is concerned. When it has to enter, it stops at the first difference.

JavaScript makes no native difference between data and code inheritence. new MyVector() is syntax sugar in JavaScript for var x = Object.create(MyVector.prototype); MyVector.call(x);. And, at least prior to ES6 classes, the conventional way to declare methods was to create a (by default, enumerable) property on a plain object, like MyVector.prototype.transformDirection = function or MyVector.prototype = { transformDirection: function }.

The visual diff is based on strings, currently formatted independently of each other. I'm open to improving this. Note that QUnit already excludes non-enumerable properties from the formatting.

For ES6 classes the diff naturally looks much cleaner. That's because class MyClass { foo() { return x; } } is syntax sugar for Object.defineProperty(MyClass.prototype, 'foo', {enumerable: false, value: function… }. The downside is that when QUnit finds a value with the wrong class, the assertion correctly fails, but there is no visual diff (yet).

As for why someone would use deepEqual. Note that assert.deepEqual is the only assertion that compares the prototype. It exists to check that an object is (also) of same type, instance of same class, or inherits a similarly shaped plain object. If you don't need that, you might prefer assert.propEqual or assert.propContains instead. Deep equality in other test frameworks behaves similarly, partly because CommonJS Unit Testing was based on QUnit.

Examples:

• Consider WordPress.getPost("Foo") not returning Post { name: Foo, id: 1 }, but User { name: Foo, id: 1 }, or plain { name: Foo, id: 1 }. Only one of these will satisfy instanceof Post, and only one of these will have obj.getPostId() as a method. deepEqual is thus an easy way to check obj instanceof and properties in one go.
• Plain objects, such configuration or options objects, often use inheritence in modern JavaScript. This became widespread after ES5/ES6 added utility methods to ease that, such as var options = Object.assign(Object.create(defaultOptions), { foo: 1 }). This in contrast to lodash.extend() and jQuery.extend(), which produce "flat" objects instead. Properties changing in the parent, are effectively the same as changing an own property.
• Consider eleven.getLocation(): Coord returning PrimaryDimensionCoord { lat, long } vs UpsideDownCoord { lat, long }