test: failing behaviour on sandboxed Proxy #11671

AnnaMag · 2017-03-03T12:33:41Z

CopyProperties() causes sandboxed Proxy to throw error
despite no code being run. The CopyProperties() function
will be removed shortly with the updates to the V8 API.

Here, failing Proxy test case is moved to known_issues.

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)

test

AnnaMag · 2017-03-03T12:37:31Z

cc/ @fhinkel

fhinkel · 2017-03-03T12:51:57Z

I agree, that proxy should not throw. The fact that it does, is an implementation detail and should be a known issue rather than a test case.

fhinkel · 2017-03-03T12:53:23Z

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

fhinkel · 2017-03-06T07:55:33Z

Can you address the linter issues:

node/test/known_issues/test-vm-proxy-failure-CP.js
   4:7  error  'assert' is assigned a value but never used   no-unused-vars
  13:3  error  Expected indentation of 0 spaces but found 2  indent

Thank you!

AnnaMag · 2017-03-06T09:05:36Z

Done, @fhinkel

fhinkel · 2017-03-06T09:08:34Z

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

jasnell · 2017-03-06T21:52:51Z

Unrelated failure in CI

fhinkel · 2017-03-07T06:11:17Z

test/known_issues/test-vm-proxy-failure-CP.js

+const sandbox = new Proxy({foo: 'bar'}, handler);
+const context = vm.createContext(sandbox);
+
+vm.runInContext('', context);


Can you add an assert? I'd be nice to move this file from known_issue to test once the issue is addressed, and a test without an assert would be confusing.

Great idea. Done & thanks!

fhinkel · 2017-03-07T11:56:56Z

test/known_issues/test-vm-proxy-failure-CP.js

+const context = vm.createContext(sandbox);
+
+
+assert.throws(() => vm.runInContext('', context),


Don't we expect this to not throw?

We usually have an assert in the known issues, that asserts the correct behavior but is currently failing. So for this test, we want runInContext to not throw, but it currently does.

Yes, true. We should be able to move it from /known_issues as is. Thanks!

fhinkel · 2017-03-09T08:40:51Z

test/known_issues/test-vm-proxy-failure-CP.js

+
+
+assert.throws(() => vm.runInContext('', context),
+        /Sandbox throws in CopyProperties() despite no code being run/);


You're testing that the exception's error matches Sandbox throws..., which of course it doesn't because it is whoops. So the assert fails and it looks like the known issue passes. But we need to check that no exception is thrown, not that a made-up exception is not thrown.

Why don't we change the assert to
assert.doesNotThrow(() => vm.runInContext('', context));
and add a comment in the beginning of the test:
// Sandbox throws in CopyProperties() despite no code being run.

Done. Thanks so much for pointing it out.

CopyProperties() causes sandboxed Proxy to throw error when in fact no code has been run. The function will be removed with the updates to the V8 API. Here, failing Proxy test case is moved to known_issues.

jasnell · 2017-03-15T00:44:11Z

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

jasnell · 2017-03-17T16:43:15Z

Opened a tracking issue for the known-issue: #11902

CopyProperties() causes sandboxed Proxy to throw error when in fact no code has been run. The function will be removed with the updates to the V8 API. Here, failing Proxy test case is moved to known_issues. PR-URL: #11671 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>

jasnell · 2017-03-17T16:47:18Z

Landed in 6473737

CopyProperties() causes sandboxed Proxy to throw error when in fact no code has been run. The function will be removed with the updates to the V8 API. Here, failing Proxy test case is moved to known_issues. PR-URL: nodejs#11671 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>

CopyProperties() causes sandboxed Proxy to throw error when in fact no code has been run. The function will be removed with the updates to the V8 API. Here, failing Proxy test case is moved to known_issues. PR-URL: #11671 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>

CopyProperties() causes sandboxed Proxy to throw error when in fact no code has been run. The function will be removed with the updates to the V8 API. Here, failing Proxy test case is moved to known_issues. PR-URL: nodejs/node#11671 Reviewed-By: James M Snell <jasnell@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 3, 2017

fhinkel added the vm Issues and PRs related to the vm subsystem. label Mar 3, 2017

AnnaMag force-pushed the proxy_CP branch from 62ae6ad to d20f60d Compare March 6, 2017 09:04

jasnell approved these changes Mar 6, 2017

View reviewed changes

fhinkel requested changes Mar 7, 2017

View reviewed changes

AnnaMag force-pushed the proxy_CP branch from d20f60d to 1254525 Compare March 7, 2017 10:55

fhinkel requested changes Mar 9, 2017

View reviewed changes

AnnaMag force-pushed the proxy_CP branch from 1254525 to 6faa23f Compare March 9, 2017 13:02

test: failing behaviour on sandboxed Proxy

4a74437

CopyProperties() causes sandboxed Proxy to throw error when in fact no code has been run. The function will be removed with the updates to the V8 API. Here, failing Proxy test case is moved to known_issues.

AnnaMag force-pushed the proxy_CP branch from 6faa23f to 4a74437 Compare March 9, 2017 13:08

fhinkel approved these changes Mar 9, 2017

View reviewed changes

jasnell mentioned this pull request Mar 17, 2017

vm: known issue with CopyProperties throw on empty code #11902

Closed

jasnell closed this Mar 17, 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: failing behaviour on sandboxed Proxy #11671

test: failing behaviour on sandboxed Proxy #11671

AnnaMag commented Mar 3, 2017 •

edited

AnnaMag commented Mar 3, 2017

fhinkel commented Mar 3, 2017

fhinkel commented Mar 3, 2017

fhinkel commented Mar 6, 2017

AnnaMag commented Mar 6, 2017

fhinkel commented Mar 6, 2017

jasnell commented Mar 6, 2017

fhinkel Mar 7, 2017

AnnaMag Mar 7, 2017

fhinkel Mar 7, 2017 •

edited

AnnaMag Mar 9, 2017

fhinkel Mar 9, 2017

AnnaMag Mar 9, 2017

jasnell commented Mar 15, 2017

jasnell commented Mar 17, 2017

jasnell commented Mar 17, 2017

		const context = vm.createContext(sandbox);


		assert.throws(() => vm.runInContext('', context),



		assert.throws(() => vm.runInContext('', context),
		/Sandbox throws in CopyProperties() despite no code being run/);

test: failing behaviour on sandboxed Proxy #11671

test: failing behaviour on sandboxed Proxy #11671

Conversation

AnnaMag commented Mar 3, 2017 • edited

Checklist

Affected core subsystem(s)

AnnaMag commented Mar 3, 2017

fhinkel commented Mar 3, 2017

fhinkel commented Mar 3, 2017

fhinkel commented Mar 6, 2017

AnnaMag commented Mar 6, 2017

fhinkel commented Mar 6, 2017

jasnell commented Mar 6, 2017

fhinkel Mar 7, 2017

Choose a reason for hiding this comment

AnnaMag Mar 7, 2017

Choose a reason for hiding this comment

fhinkel Mar 7, 2017 • edited

Choose a reason for hiding this comment

AnnaMag Mar 9, 2017

Choose a reason for hiding this comment

fhinkel Mar 9, 2017

Choose a reason for hiding this comment

AnnaMag Mar 9, 2017

Choose a reason for hiding this comment

jasnell commented Mar 15, 2017

jasnell commented Mar 17, 2017

jasnell commented Mar 17, 2017

AnnaMag commented Mar 3, 2017 •

edited

fhinkel Mar 7, 2017 •

edited