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: failing behaviour on sandboxed Proxy #11671

Closed
wants to merge 1 commit into from

Conversation

AnnaMag
Copy link
Member

@AnnaMag AnnaMag commented Mar 3, 2017

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

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

AnnaMag commented Mar 3, 2017

cc/ @fhinkel

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

fhinkel commented Mar 3, 2017

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

fhinkel commented Mar 3, 2017

@fhinkel
Copy link
Member

fhinkel commented Mar 6, 2017

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

AnnaMag commented Mar 6, 2017

Done, @fhinkel

@fhinkel
Copy link
Member

fhinkel commented Mar 6, 2017

@jasnell
Copy link
Member

jasnell commented Mar 6, 2017

Unrelated failure in CI

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

vm.runInContext('', context);
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Great idea. Done & thanks!

const context = vm.createContext(sandbox);


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

@fhinkel fhinkel Mar 7, 2017

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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



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

Choose a reason for hiding this comment

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

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.

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

jasnell commented Mar 15, 2017

@jasnell
Copy link
Member

jasnell commented Mar 17, 2017

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

jasnell pushed a commit that referenced this pull request Mar 17, 2017
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
Copy link
Member

jasnell commented Mar 17, 2017

Landed in 6473737

@jasnell jasnell closed this Mar 17, 2017
italoacasas pushed a commit to italoacasas/node that referenced this pull request Mar 20, 2017
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>
jungx098 pushed a commit to jungx098/node that referenced this pull request Mar 21, 2017
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>
MylesBorins pushed a commit that referenced this pull request Apr 18, 2017
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>
MylesBorins pushed a commit that referenced this pull request Apr 19, 2017
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>
@MylesBorins MylesBorins mentioned this pull request Apr 19, 2017
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
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>
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.

5 participants