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 test for a vm indexed property #23318

Closed
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

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
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


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.

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

@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

@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
Adds a single test for a vm with an indexed property.

PR-URL: #23318
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
targos pushed a commit that referenced this pull request Oct 12, 2018
Adds a single test for a vm with an indexed property.

PR-URL: #23318
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
jasnell pushed a commit that referenced this pull request Oct 17, 2018
Adds a single test for a vm with an indexed property.

PR-URL: #23318
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
MylesBorins pushed a commit that referenced this pull request Oct 30, 2018
Adds a single test for a vm with an indexed property.

PR-URL: #23318
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@codebytere codebytere mentioned this pull request Nov 27, 2018
rvagg pushed a commit that referenced this pull request Nov 28, 2018
Adds a single test for a vm with an indexed property.

PR-URL: #23318
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 29, 2018
Adds a single test for a vm with an indexed property.

PR-URL: #23318
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@codebytere codebytere mentioned this pull request Nov 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

7 participants