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.

Sign up for GitHub

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

Jump to bottom

test: add test for a vm indexed property #23318

Closed
wants to merge 4 commits into from
Closed

test: add test for a vm indexed property #23318

wants to merge 4 commits into from

Conversation

conectado
Copy link
Contributor

This commit adds a test for a vm with a single indexed property

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@conectado
test: add test for a vm indexed property
4e2f2e8 
Adds a single test for a vm with an indexed property.
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Oct 7, 2018
devsnek
devsnek approved these changes Oct 7, 2018
View reviewed changes
Copy link
Member

@devsnek devsnek left a comment

Choose a reason for hiding this comment

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

Specifically this test adds coverage for IndexedPropertyGetterCallback and IndexedPropertyDeleterCallback

@devsnek devsnek added the vm Issues and PRs related to the vm subsystem. label Oct 7, 2018
@devsnek
Copy link
Member

devsnek commented Oct 7, 2018

@nodejs/vm

jdalton
jdalton reviewed Oct 7, 2018
View reviewed changes
test/parallel/test-vm-context-property-forwarding.js Outdated

const x = vm.createContext({ get a() { return 5; } });

assert.strictEqual(x.a, 5);
Copy link
Member

Choose a reason for hiding this comment

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

Does IndexedPropertyGetterCallback apply for properties returning index values or indexed properties?

Reflect.defineProperty(x, '1', { get() { return 2 } })

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just tested that scenario and in fact, x[1] === 2. If that's the question. I'm not sure I quite understand it.

jdalton
jdalton suggested changes Oct 7, 2018
View reviewed changes
Copy link
Member

@jdalton jdalton left a comment

Choose a reason for hiding this comment

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

This test doesn't look like its testing index-like property names and is instead checking properties with index-like values. I'm not entirely sure what this test is trying to cover.

@conectado
Copy link
Contributor Author

conectado commented Oct 7, 2018 •
edited
Loading

@jdalton How about now? We are accessing indexed property ctx2[1].

@jdalton
Copy link
Member

jdalton commented Oct 8, 2018

Could you add a comment above the test explaining which component it's testing, e.g. IndexedPropertyGetterCallback.

@conectado
Copy link
Contributor Author

@jdalton done

@refack
Copy link
Contributor

refack commented Oct 8, 2018

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

@refack refack added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 8, 2018
@danbev
Copy link
Contributor

danbev commented Oct 11, 2018

Landed in b409eaa.

@danbev danbev closed this Oct 11, 2018
danbev pushed a commit that referenced this pull request Oct 11, 2018
@conectado @danbev
test: add test for a vm indexed property
b409eaa 
Adds a single test for a vm with an indexed property.

PR-URL: #23318
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: John-David Dalton <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
targos pushed a commit that referenced this pull request Oct 12, 2018
@conectado @targos
test: add test for a vm indexed property
e34761f 
Adds a single test for a vm with an indexed property.

PR-URL: #23318
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: John-David Dalton <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
jasnell pushed a commit that referenced this pull request Oct 17, 2018
@conectado @jasnell
test: add test for a vm indexed property
3c4d316 
Adds a single test for a vm with an indexed property.

PR-URL: #23318
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: John-David Dalton <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
MylesBorins pushed a commit that referenced this pull request Oct 30, 2018
@conectado @MylesBorins
test: add test for a vm indexed property
e865418 
Adds a single test for a vm with an indexed property.

PR-URL: #23318
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: John-David Dalton <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
@codebytere codebytere mentioned this pull request Nov 27, 2018
Closed
rvagg pushed a commit that referenced this pull request Nov 28, 2018
@conectado @rvagg
test: add test for a vm indexed property
ace066c 
Adds a single test for a vm with an indexed property.

PR-URL: #23318
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: John-David Dalton <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 29, 2018
@conectado @MylesBorins
test: add test for a vm indexed property
6493e01 
Adds a single test for a vm with an indexed property.

PR-URL: #23318
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: John-David Dalton <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
@codebytere codebytere mentioned this pull request Nov 29, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jdalton jdalton jdalton approved these changes

@refack refack refack approved these changes

@jasnell jasnell jasnell approved these changes

@devsnek devsnek devsnek approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. test Issues and PRs related to the tests. vm Issues and PRs related to the vm subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@conectado @devsnek @jdalton @refack @danbev @jasnell @nodejs-github-bot