test: add known_issues test for #5350 #10319

AnnaMag · 2016-12-17T16:16:00Z

Checklist

make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

tests

Description of change

A test addressing subproblem of #5350
inherited properties flattened in the vm.runInNewContext
is added to the known_issues directory.
It will be fixed with the 5.5 V8 API changes

#5350 is a consequence of the above as flattening of sandbox
inherited properties happens first inside the vm context
and is subsequently translated onto the outer sandbox.
assert.strictEqual(sandbox.hasOwnProperty('propBase'), false);

Fishrock123 · 2016-12-17T16:20:41Z

@AnnaMag btw, to get the list to show up as a checklist you need to remove the white space. :D

e.g. - [x]

Fishrock123

I think make lint should catch those nits? I forget.

CI: https://ci.nodejs.org/job/node-test-pull-request/5454/

Fishrock123 · 2016-12-17T16:21:35Z

test/known_issues/test-vm-inherited_properties.js

+
+const context = vm.createContext(sandbox);
+
+const result = vm.runInContext("this.hasOwnProperty('prop_base');", context);


single quotes please!

Is this the proper way to write it?
'this.hasOwnProperty("propBase");'
I thought that double quotes are used in JS to avoid escaping: http://standardjs.com/rules.html
Could you clarify this for me, please? Thanks!

Oh uh... good question. We don't use Standard though (for legacy reasons).

@Trott thoughts? I'd prefer template strings tbh but I think our linter may warn about that currently...

Ok. I asked for future reference.
I used template strings here: https://github.com/nodejs/node/pull/10272/files
and looks like the linter was ok with it

In our lint rules, double quotes are acceptable if being used to avoid escaping single quotes. There are a few examples of it in the code base, but not many.

(Just providing the answer to the question asked. I'm fine with whatever you do in this case as long as the linter accepts it.)

Fishrock123 · 2016-12-17T16:21:44Z

test/known_issues/test-vm-inherited_properties.js

+const assert = require('assert');
+
+const base = {
+  prop_base: 1,


could we drop the trailing comma?

AnnaMag · 2016-12-17T16:25:45Z

cc/ @fhinkel

From https://nodejs.org/api/vm.html#vm_vm_createcontext_sandbox
Outside of scripts run by the vm module, sandbox will remain unchanged.
Should sandbox always reflect changes happening in the vm context (even if changes do not cause unintended behavior)?

cjihrig · 2016-12-17T16:37:50Z

test/known_issues/test-vm-inherited_properties.js

+const assert = require('assert');
+
+const base = {
+  prop_base: 1,


Can you use camelCase for JavaScript variables please.

cjihrig · 2016-12-17T16:38:09Z

test/known_issues/test-vm-inherited_properties.js

+};
+
+const sandbox = Object.create(base, {
+  prop_sandbox: {value: 3}


camelCase here too please.

AnnaMag · 2016-12-17T16:55:14Z

@Fishrock123, @cjihrig Done- thanks!

italoacasas · 2016-12-18T01:07:28Z

CI: https://ci.nodejs.org/job/node-test-pull-request/5462/console

fhinkel · 2016-12-20T15:02:30Z

test/known_issues/test-vm-inherited_properties.js

@@ -0,0 +1,20 @@
+'use strict';
+// Ref: https://github.com/nodejs/node/issues/5350


We usually don't include Ref: when linking to issues. I can only find two tests that do that. (For consistency they probably shouldn't)

The known issue tests do.

I don't think it matters.

The more information possible the better, though.

I meant just get rid of Ref, not the link.

Yea. I meant the known issue tests (I think all of them) include Refs: at the top.

Never mind. Didn't realize that's the standard for known tests. Sorry for derailing the discussion.

AnnaMag · 2016-12-20T18:09:39Z

Yes, it seems to be included in all files in that dir. I assumed it was an agreed upon "standard". Of course, np to remove it.

fhinkel

LGTM

italoacasas · 2016-12-21T14:59:34Z

Landed: b73402d

PR-URL: #10319 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>

MylesBorins · 2017-01-23T02:30:44Z

this passes on v6 and fails on v4. Landed only on v6

PR-URL: #10319 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>

nodejs-github-bot added test Issues and PRs related to the tests. lts-watch-v6.x labels Dec 17, 2016

Fishrock123 suggested changes Dec 17, 2016

View reviewed changes

AnnaMag force-pushed the test-issue-5350 branch from b9b9655 to 559dcad Compare December 17, 2016 16:35

nodejs-github-bot added lts-watch-v6.x labels Dec 17, 2016

cjihrig requested changes Dec 17, 2016

View reviewed changes

AnnaMag force-pushed the test-issue-5350 branch from 559dcad to f829fb7 Compare December 17, 2016 16:44

nodejs-github-bot added lts-watch-v6.x labels Dec 17, 2016

test: add known_issues test for nodejs#5350

9c9c5a7

AnnaMag force-pushed the test-issue-5350 branch from f829fb7 to 9c9c5a7 Compare December 17, 2016 16:45

nodejs-github-bot added lts-watch-v6.x labels Dec 17, 2016

cjihrig approved these changes Dec 17, 2016

View reviewed changes

mscdex added the vm Issues and PRs related to the vm subsystem. label Dec 17, 2016

Fishrock123 approved these changes Dec 18, 2016

View reviewed changes

fhinkel reviewed Dec 20, 2016

View reviewed changes

fhinkel approved these changes Dec 21, 2016

View reviewed changes

italoacasas closed this Dec 21, 2016

evanlucas mentioned this pull request Jan 3, 2017

v7.4.0 release proposal #10589

Merged

MylesBorins added dont-land-on-v4.x and removed lts-watch-v4.x labels Jan 23, 2017

MylesBorins mentioned this pull request Jan 24, 2017

v6.10.0 proposal #10974

Merged

AnnaMag deleted the test-issue-5350 branch January 25, 2017 14:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: add known_issues test for #5350 #10319

test: add known_issues test for #5350 #10319

AnnaMag commented Dec 17, 2016 •

edited

Loading

Fishrock123 commented Dec 17, 2016

Fishrock123 left a comment

Fishrock123 Dec 17, 2016

AnnaMag Dec 17, 2016

Fishrock123 Dec 17, 2016

AnnaMag Dec 17, 2016

Trott Dec 18, 2016

Trott Dec 18, 2016

AnnaMag Dec 18, 2016

Fishrock123 Dec 17, 2016

AnnaMag commented Dec 17, 2016 •

edited

Loading

cjihrig Dec 17, 2016

cjihrig Dec 17, 2016

AnnaMag commented Dec 17, 2016

italoacasas commented Dec 18, 2016

fhinkel Dec 20, 2016

cjihrig Dec 20, 2016

Fishrock123 Dec 20, 2016

fhinkel Dec 20, 2016

cjihrig Dec 20, 2016

fhinkel Dec 21, 2016

AnnaMag commented Dec 20, 2016

fhinkel left a comment

italoacasas commented Dec 21, 2016

MylesBorins commented Jan 23, 2017 •

edited

Loading


		const context = vm.createContext(sandbox);

		const result = vm.runInContext("this.hasOwnProperty('prop_base');", context);

		@@ -0,0 +1,20 @@
		'use strict';
		// Ref: https://github.com/nodejs/node/issues/5350

test: add known_issues test for #5350 #10319

test: add known_issues test for #5350 #10319

Conversation

AnnaMag commented Dec 17, 2016 • edited Loading

Checklist

Affected core subsystem(s)

Description of change

Fishrock123 commented Dec 17, 2016

Fishrock123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AnnaMag commented Dec 17, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AnnaMag commented Dec 17, 2016

italoacasas commented Dec 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AnnaMag commented Dec 20, 2016

fhinkel left a comment

Choose a reason for hiding this comment

italoacasas commented Dec 21, 2016

MylesBorins commented Jan 23, 2017 • edited Loading

AnnaMag commented Dec 17, 2016 •

edited

Loading

AnnaMag commented Dec 17, 2016 •

edited

Loading

MylesBorins commented Jan 23, 2017 •

edited

Loading