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: add known_issues test for #10223 #11024

Closed
wants to merge 1 commit into from

Conversation

AnnaMag
Copy link
Member

@AnnaMag AnnaMag commented Jan 26, 2017

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

tests

Description of change

Test addressing #10223 is added to the known_issues directory.
Writable attribute should be false by default.

In v6+ (reverted 524f693) it is set to true irrespective of whether
it was set to false or left out in Object.defineProperty call.
It will be fixed with the 5.5 V8 API changes

@nodejs-github-bot nodejs-github-bot added test Issues and PRs related to the tests. lts-watch-v6.x labels Jan 26, 2017
@AnnaMag
Copy link
Member Author

AnnaMag commented Jan 26, 2017

cc/ @fhinkel , @bnoordhuis

// Refs: https://github.com/nodejs/node/issues/10223

require('../common');
var vm = require('vm');
Copy link
Contributor

Choose a reason for hiding this comment

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

const?

Copy link
Member

Choose a reason for hiding this comment

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

This will be flagged by the linter. It additionally flags that util on line 7 is never used.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done, thanks

const context = vm.createContext({});

const code = `
Object.defineProperty(this, "foo", {value: 5});
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you use single quotes around "foo" on this line and the next please.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done, thanks

@mscdex mscdex added the vm Issues and PRs related to the vm subsystem. label Jan 26, 2017
Copy link
Member

@bnoordhuis bnoordhuis 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 a suggestion.


const code = `
Object.defineProperty(this, 'foo', {value: 5});
Object.getOwnPropertyDescriptor(this, 'foo').writable;
Copy link
Member

Choose a reason for hiding this comment

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

I'd return the descriptor and then check that .writable, .enumerable and .configurable are all false.

Copy link
Member Author

Choose a reason for hiding this comment

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

suggestion incorporated:)

Copy link
Member Author

@AnnaMag AnnaMag Jan 26, 2017

Choose a reason for hiding this comment

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

ah, I just added the check for 'writable'. As a way to compare it all at once, would that work?:

const code = `
   Object.defineProperty(this, 'foo', {value: 5});
   Object.getOwnPropertyDescriptor(this, 'foo');
`;

const desc = vm.runInContext(code, context);

const foo = {value: 5, writable: false, configurable: false, enumerable: true}
assert.strictEqual(desc, foo)

Copy link
Member

Choose a reason for hiding this comment

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

If you use assert.deepStrictEqual(), it should.

Copy link
Member Author

@AnnaMag AnnaMag Jan 27, 2017

Choose a reason for hiding this comment

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

I tried, but I don't think assert.deepStrictEqual() is working as intended...
I tested on a positive example and it throws an error:

const desc = vm.runInContext(code, context);
//desc: { value: 5, writable: true, enumerable: true, configurable: true }

const foo = {value: 5, writable: true, configurable: true, enumerable: true};
assert.deepStrictEqual(desc, foo); // throw new assert.AssertionError

@fhinkel
Copy link
Member

fhinkel commented Jan 30, 2017

Thanks! Landed in e8bb9e6.

@fhinkel fhinkel closed this Jan 30, 2017
fhinkel pushed a commit that referenced this pull request Jan 30, 2017
PR-URL: #11024
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
evanlucas pushed a commit that referenced this pull request Jan 31, 2017
PR-URL: #11024
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@italoacasas italoacasas mentioned this pull request Jan 31, 2017
jasnell pushed a commit that referenced this pull request Mar 8, 2017
PR-URL: #11024
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
jasnell pushed a commit that referenced this pull request Mar 8, 2017
PR-URL: #11024
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
PR-URL: #11024
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@MylesBorins MylesBorins mentioned this pull request Mar 9, 2017
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
PR-URL: #11024
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@MylesBorins MylesBorins mentioned this pull request Mar 9, 2017
@AnnaMag AnnaMag deleted the working_setter branch March 28, 2017 16:41
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.

9 participants