assert.deepStrictEqual() fails to compare ES6 style object properties

[AnnaMag]

• Version: v7.7.1
• Platform: Mac OSX X86_64
• Subsystem:

The following assertion fails when objects are property descriptors:

'use strict';
require('../common');
const assert = require('assert');

const x = {};

Object.defineProperty(x, 'prop', {
  get() {
    return 'foo';
  }
});

const y = {};

Object.defineProperty(y, 'prop', {
  get() {
    return 'foo';
  }
});

const descriptorx = Object.getOwnPropertyDescriptor(x, 'prop');
const descriptory = Object.getOwnPropertyDescriptor(y, 'prop');

assert.deepStrictEqual(x, y); // ok
assert.deepStrictEqual(descriptorx, descriptory); // throws an error

What seems to happen is that (in both cases) Object.getPrototypeOf(descriptor)
results in {}, which leads to the condition
https://github.com/nodejs/node/blob/master/lib/assert.js#L200
returning false. Same is true for assert.strictEqual()

[AnnaMag]

With the changes in the new V8 API, accessor properties are not flattened anymore (patch hopefully coming soon as we are working on it with @fhinkel :)) and descriptors can have attributes that are functions (getters/setters). E.g. the following test will not throw with the new API:
https://github.com/nodejs/node/blob/master/test/known_issues/test-vm-getters.js
but current assertions do not work for deep object comparison with functions as attributes.
That is, this will not work:
https://github.com/nodejs/node/blob/master/test/known_issues/test-vm-getters.js#L19

I wrote a piece of code that fixed the issue:

const aKeys = Object.keys(descriptory);
const bKeys = Object.keys(descriptorx);

var keya, keyb, vala, valb, i;

if (aKeys.length !== bKeys.length)
  return false;

aKeys.sort();
bKeys.sort();

for (i = 0; i < aKeys.length; i++) {
  keya = aKeys[i];
  keyb = bKeys[i];
  if (keya === keyb) {
      if (typeof descriptory[keya] === 'function' &&
          typeof descriptorx[keyb] === 'function') {
              vala = descriptory[keya].toString();
              valb = descriptorx[keyb].toString();
              if (vala !== valb) {return false;}
              else continue;
       }

      if (descriptory[keya] !== descriptorx[keyb]) return false; //different values
    } else  return false; // keys are different
}

Happy to contribute it to assert.js. I was not able to figure out how to weave it into the existing code, though.

[AnnaMag]

cc/ @fhinkel

[addaleax]

What seems to happen is that (in both cases) Object.getPrototypeOf(descriptor)
results in {}, which leads to the condition
https://github.com/nodejs/node/blob/master/lib/assert.js#L200
returning false.

Object.getPrototypeOf(descriptorx) === Object.getPrototypeOf(descriptory) yields true for me, so I don’t think that’s the problem.

You define two different functions as the get fields of the two descriptors, so I think this assert is correct here (descriptorx.get !== descriptory.get)?

[joyeecheung]

I think this again raises the issue on whether we should add support for new ES features in assert...(see #11113). Similar to descriptors, the assertions don't work on Maps, Sets either..

Also related: last CTC minutes on this https://github.com/nodejs/CTC/blob/master/meetings/2017-02-01.md#assert-enforce-type-check-in-deepstrictequal-node10282

[addaleax]

@AnnaMag If I understand your code correctly, you would say that assert.strictEqual(function a() {}, function a() {}) should not throw, even though those are distinct functions – correct?

[joyeecheung]

(Also just noticed the comments above https://github.com/nodejs/node/blob/master/lib/assert.js#L200 are not correct...at least not until #10282 is merged, uh..)

[AnnaMag]

@addaleax, correct... might not be in line with the overall design. What I am after is to have a way to test whether ES6 property descriptors work correctly in the vm.

After removing CopyProperties() and using the 5.5 V8 API, this test (among others)
https://github.com/nodejs/node/blob/master/test/known_issues/test-vm-getters.js
can be moved from /known_issues to /parallel.
With no assert in place it is tricky to show that the test does work :)

[joyeecheung]

@AnnaMag @addaleax Ah yes, those are not the same functions. This should work:

function foo() { return 'foo'; }

const x = {};

Object.defineProperty(x, 'prop', {
  get: foo
});

const y = {};

Object.defineProperty(y, 'prop', {
  get: foo
});

[joyeecheung]

@AnnaMag I think if the bug has been fixed, https://github.com/nodejs/node/blob/master/test/known_issues/test-vm-getters.js should pass now ? Because assert.strictEqual(result, descriptor) is equivalent to assert(result === descriptor)(assuming that is the desired behavior).

[AnnaMag]

@joyeecheung, exactly, it should pass :) Yet it does not, which is the very thing that made me check the code to find out why:

assert.js:81
  throw new assert.AssertionError({
  ^
AssertionError: { get: [Function: get],
  set: undefined,
  enumerable: false,
  configurable: false } deepStrictEqual { get: [Function: get],
  set: undefined,
  enumerable: false,
  configurable: false }

This is the same for deepStrictEqual and strictEqual.
Object.getPrototypeOf() is {} and, as I mentioned above, nothing is in reality being checked (no comparison of the attributes) as the condition https://github.com/nodejs/node/blob/master/lib/assert.js#L200
returns false and nothing is executed after that.

What also happens is that even if it did work, it would not compare the values of the accessors (only attribute keys), as (as far as I understand it), comparing functions in JS requires a hack, e.g. .toString().
I might be wrong, so please feel free to correct me. Thanks for the input!

[addaleax]

Object.getPrototypeOf() is {}

Ah – sorry, I think I understand how this relates to the vm module. Object.getPrototypeOf() is Object.prototype in both cases, but because we are comparing the results of Object.getOwnPropertyDescriptor() calls in two different contexts, the returned descriptor objects refer to different Object.prototypes.

Since the test is supposed to be a test for #2734, I think it would be okay to fix this up in the test itself (replacing result with Object.assign({}, result) in the assertion might work?).
But this is still an interesting point that I don’t think we considered in #11128.

[AnnaMag]

@joyeecheung, the code snippet with common definition of foo does not seem to work for the current master (nor for my build).
Thanks @addaleax, makes sense.

Summing up, https://github.com/nodejs/node/blob/master/test/known_issues/test-vm-getters.js is not a valid test then. Is PR a good idea here?

[addaleax]

Is PR a good idea here?

Looks like it, yes… I’m not sure but I don’t think we’ll want to turn the prototype-equals-prototype check into a deep comparison, and that’s the only alternative I can think of right now. Maybe @fhinkel or @joyeecheung have better ideas but fixing up the test seems like the best option to me.