test: correct the assertion for the ES6 object properties

[AnnaMag]

test: fix assertion in a vm test

Prototypes are not strict equal when they are
from different contexts. Therefore, assert.strictEqual()
fails for objects that are created in different
contexts, e.g. in vm.runInContext().

Instead of expecting the prototypes to be equal,
only check the properties of the objects for equality.

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

Affected core subsystem(s)

test

[AnnaMag]

cc/ @fhinkel

[addaleax]

Have you checked whether using assert.deepStrictEqual(Object.assign({}, result), descriptor) instead works too? I am not a 100 % sure it’s more readable so feel free to ignore me. 😄

[AnnaMag]

@addaleax, I double-checked and your alternative does work! Upps...
IMHO, it is more elegant, so I'd choose to use it in the test. Not sure about readability, so maybe I'll hold off with the change until an extra view on that :)

[addaleax]

LGTM with or without the suggestion :)

CI: https://ci.nodejs.org/job/node-test-commit/8487/

[fhinkel]

According to #11803, the problem is that assert.strictEqual() is false on two functions (functions that look the same way but are different objects).

I replaced the last assert.strictEqual() in #11803 with the changes in this PR, and it fails:

AssertionError: [Function: get] === [Function: get]

Seems to me like this change does not fix that problem. Can you explain how the patch is checking something different for getters than before? Thanks!

[AnnaMag]

I believe that those are distinct issues...? This patch is not meant to fix possible 'shortcomings' of assert related to the new ES features. @addaleax 's comment in #11803 addresses the question of the failure of the current assert check (why Object.getPrototypeOf() is {}). The change in the code compares descriptor attributes directly, surpassing the issue of comparing object prototypes in different vm contexts. This is a test fix, not an improvement to the assert module.
Could you share the code you ran, please?

[fhinkel]

I thought the issue is about comparing two getter functions. Your commit message says accessors, which accessors do you mean?

This is the test I ran:

var 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

for (const prop of Object.keys(descriptorx)) {
   assert.strictEqual(descriptorx[prop], descriptory[prop]); // AssertionError: [Function: get] === [Function: get]

}

[addaleax]

Sorry, right, I should have looked at the commit message here 🙈 The problem is that result and descriptor have Object.prototypes from different contexts. The function objects compared by the test are actually the same (as in ===); so it’s the commit message that should be updated. 😄

[fhinkel]

Ah. Thanks! That's why I was looking for something different and this commit didn't make much sense to me 😃

[AnnaMag]

I see where it was confusing. It is updated now. Thanks @fhinkel 👍

[fhinkel]

I still find the commit message very confusing. Can you describe the change you made rather than what the test is testing? If the change is because of different prototypes (because they're coming from different contexts), why is that not mentioned in the commit message?

To me, mentioning ES6 style properties, flattening, and sandbox is confusing with respect to this commit.

How about something like this:

test: fix assertion in vm test

Prototypes are not strict equal when they are from different contexts. Therefore, assert.strictEqual() fails for objects that are created in different contexts, e.g., in vm.runInContext().

Instead of expecting the prototypes to be equal, only check the properties of the objects for equality.

[fhinkel]

Thanks! Landed in 7ef2d90.