test: correct the assertion for the ES6 object properties by AnnaMag · Pull Request #11862 · nodejs/node · GitHub

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

cc/ @fhinkel

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. 😄

@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 :)

LGTM with or without the suggestion :)

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

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!

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?

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]

}

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. 😄

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

I see where it was confusing. It is updated now. Thanks @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.

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.

Thanks! Landed in 7ef2d90.

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. PR-URL: #11862 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>

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. PR-URL: nodejs#11862 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>

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. PR-URL: #11862 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>

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. PR-URL: nodejs/node#11862 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>

nodejs-github-bot added the test Issues and PRs related to the tests. label Mar 15, 2017

AnnaMag force-pushed the test_accessors branch from ebaa73e to 40d4ec1 Compare March 15, 2017 10:30

mscdex added the vm Issues and PRs related to the vm subsystem. label Mar 15, 2017

addaleax reviewed Mar 15, 2017

View reviewed changes

addaleax approved these changes Mar 15, 2017

View reviewed changes

jasnell approved these changes Mar 15, 2017

View reviewed changes

cjihrig approved these changes Mar 16, 2017

View reviewed changes

fhinkel suggested changes Mar 16, 2017

View reviewed changes

AnnaMag force-pushed the test_accessors branch 3 times, most recently from 0d948bf to 808a93d Compare March 16, 2017 17:01

AnnaMag force-pushed the test_accessors branch from 808a93d to 5231270 Compare March 17, 2017 14:16

fhinkel approved these changes Mar 17, 2017

View reviewed changes

fhinkel closed this Mar 20, 2017

italoacasas mentioned this pull request Mar 20, 2017

v7.7.4 Release Proposal #11941

Merged

MylesBorins added the land-on-v6.x label Apr 18, 2017

MylesBorins mentioned this pull request Apr 19, 2017

v6.10.3 proposal #12498

Merged

test: correct the assertion for the ES6 object properties by AnnaMag · Pull Request #11862 · nodejs/node · GitHub

Uh oh!

Uh oh!

test: correct the assertion for the ES6 object properties - #11862

test: correct the assertion for the ES6 object properties#11862AnnaMag wants to merge 1 commit intonodejs:masternodejs/node:masterfrom AnnaMag:test_accessorsAnnaMag/node:test_accessorsCopy head branch name to clipboard

Conversation

AnnaMag commented Mar 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

AnnaMag commented Mar 15, 2017

Uh oh!

addaleax Mar 15, 2017

Choose a reason for hiding this comment

Uh oh!

AnnaMag Mar 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

fhinkel left a comment

Choose a reason for hiding this comment

Uh oh!

AnnaMag commented Mar 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fhinkel commented Mar 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

addaleax commented Mar 16, 2017

Uh oh!

fhinkel commented Mar 16, 2017

Uh oh!

AnnaMag commented Mar 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fhinkel commented Mar 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fhinkel commented Mar 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

test: correct the assertion for the ES6 object properties#11862
AnnaMag wants to merge 1 commit into
nodejs:masterfrom
AnnaMag:test_accessors

AnnaMag commented Mar 15, 2017 •
edited
Loading

AnnaMag Mar 15, 2017 •
edited
Loading

AnnaMag commented Mar 16, 2017 •
edited
Loading

fhinkel commented Mar 16, 2017 •
edited
Loading

AnnaMag commented Mar 16, 2017 •
edited
Loading

fhinkel commented Mar 16, 2017 •
edited
Loading

fhinkel commented Mar 20, 2017 •
edited
Loading