FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

test: add test for a vm indexed property by conectado · Pull Request #23318 · nodejs/node · GitHub

/ node Public

test: add test for a vm indexed property - #23318

Closed
conectado wants to merge 4 commits into
nodejs:masterfrom
conectado:master
ConversationCommits (4)ChecksFiles changed
Closed

test: add test for a vm indexed property#23318
conectado wants to merge 4 commits into
nodejs:masterfrom
conectado:master

Conversation

conectado commented Oct 7, 2018

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

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

devsnek left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Specifically this test adds coverage for IndexedPropertyGetterCallback and IndexedPropertyDeleterCallback

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

devsnek commented Oct 7, 2018

Copy link
Copy Markdown
Member

@nodejs/vm

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

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

assert.strictEqual(x.a, 5);

jdalton Oct 7, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

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

conectado Oct 7, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 suggested changes Oct 7, 2018
View reviewed changes

jdalton left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 commented Oct 7, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

jdalton commented Oct 8, 2018

Copy link
Copy Markdown
Member

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

conectado commented Oct 8, 2018

Copy link
Copy Markdown
Contributor Author

@jdalton done

refack commented Oct 8, 2018

Copy link
Copy Markdown
Contributor

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

refack added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Oct 8, 2018

danbev commented Oct 11, 2018

Copy link
Copy Markdown
Contributor

Landed in b409eaa.

danbev closed this Oct 11, 2018
danbev pushed a commit that referenced this pull request Oct 11, 2018
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 <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
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 <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
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 <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
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 <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 mentioned this pull request Nov 27, 2018
Closed
rvagg pushed a commit that referenced this pull request Nov 28, 2018
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 <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
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 <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 mentioned this pull request Nov 29, 2018
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell approved these changes
+3 more reviewers
jdalton jdalton approved these changes
refack refack approved these changes
devsnek devsnek approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, 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

Back | FazBrowse Home | New Git URL