Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 1 commit into from

Conversation

AnnaMag
Copy link
Member

@AnnaMag AnnaMag commented Mar 15, 2017

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

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Mar 15, 2017
@AnnaMag
Copy link
Member Author

AnnaMag commented Mar 15, 2017

cc/ @fhinkel

@mscdex mscdex added the vm Issues and PRs related to the vm subsystem. label Mar 15, 2017
assert.deepStrictEqual(Object.keys(result), Object.keys(descriptor));
for (const prop of Object.keys(result)) {
assert.strictEqual(result[prop], descriptor[prop]);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@AnnaMag AnnaMag Mar 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with or without the suggestion :)

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

Copy link
Member

@fhinkel fhinkel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member Author

AnnaMag commented Mar 16, 2017

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
Copy link
Member

fhinkel commented Mar 16, 2017

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
Copy link
Member

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
Copy link
Member

fhinkel commented Mar 16, 2017

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

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

AnnaMag commented Mar 16, 2017

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

@fhinkel
Copy link
Member

fhinkel commented Mar 16, 2017

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
Copy link
Member

fhinkel commented Mar 20, 2017

Thanks! Landed in 7ef2d90.

@fhinkel fhinkel closed this Mar 20, 2017
fhinkel pushed a commit that referenced this pull request Mar 20, 2017
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>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Mar 20, 2017
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>
jungx098 pushed a commit to jungx098/node that referenced this pull request Mar 21, 2017
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>
MylesBorins pushed a commit that referenced this pull request Apr 18, 2017
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>
MylesBorins pushed a commit that referenced this pull request Apr 19, 2017
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>
@MylesBorins MylesBorins mentioned this pull request Apr 19, 2017
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests. vm Issues and PRs related to the vm subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants