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

AnnaMag · 2017-03-15T10:28:38Z

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 · 2017-03-15T10:29:19Z

cc/ @fhinkel

addaleax · 2017-03-15T15:45:49Z

test/known_issues/test-vm-getters.js

+assert.deepStrictEqual(Object.keys(result), Object.keys(descriptor));
+for (const prop of Object.keys(result)) {
+  assert.strictEqual(result[prop], descriptor[prop]);
+}


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

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 · 2017-03-16T12:11:30Z

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 · 2017-03-16T12:24:33Z

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 · 2017-03-16T12:28:13Z

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 · 2017-03-16T12:34:26Z

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

AnnaMag · 2017-03-16T17:06:45Z

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

fhinkel · 2017-03-16T19:53:11Z

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.

fhinkel · 2017-03-20T08:22:06Z

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 requested 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

AnnaMag commented Mar 15, 2017 •

edited

AnnaMag commented Mar 15, 2017

addaleax Mar 15, 2017

AnnaMag Mar 15, 2017 •

edited

addaleax left a comment

fhinkel left a comment

AnnaMag commented Mar 16, 2017 •

edited

fhinkel commented Mar 16, 2017 •

edited

addaleax commented Mar 16, 2017

fhinkel commented Mar 16, 2017

AnnaMag commented Mar 16, 2017 •

edited

fhinkel commented Mar 16, 2017 •

edited

fhinkel commented Mar 20, 2017 •

edited

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

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

Conversation

AnnaMag commented Mar 15, 2017 • edited

Checklist

Affected core subsystem(s)

AnnaMag commented Mar 15, 2017

addaleax Mar 15, 2017

Choose a reason for hiding this comment

AnnaMag Mar 15, 2017 • edited

Choose a reason for hiding this comment

addaleax left a comment

Choose a reason for hiding this comment

fhinkel left a comment

Choose a reason for hiding this comment

AnnaMag commented Mar 16, 2017 • edited

fhinkel commented Mar 16, 2017 • edited

addaleax commented Mar 16, 2017

fhinkel commented Mar 16, 2017

AnnaMag commented Mar 16, 2017 • edited

fhinkel commented Mar 16, 2017 • edited

fhinkel commented Mar 20, 2017 • edited

AnnaMag commented Mar 15, 2017 •

edited

AnnaMag Mar 15, 2017 •

edited

AnnaMag commented Mar 16, 2017 •

edited

fhinkel commented Mar 16, 2017 •

edited

AnnaMag commented Mar 16, 2017 •

edited

fhinkel commented Mar 16, 2017 •

edited

fhinkel commented Mar 20, 2017 •

edited