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

test: add node::MakeCallback() test coverage by bnoordhuis · Pull Request #3478 · nodejs/node · GitHub

/ node Public

test: add node::MakeCallback() test coverage - #3478

Closed
bnoordhuis wants to merge 1 commit into
nodejs:masterfrom
bnoordhuis:makecallback-tests
Closed

test: add node::MakeCallback() test coverage#3478
bnoordhuis wants to merge 1 commit into
nodejs:masterfrom
bnoordhuis:makecallback-tests

Conversation

Copy link
Copy Markdown
Member

R=@trevnorris?

/cc @kkoopa - this is why you can't have Local<Value> as the receiver, it wouldn't pass these tests. :-)

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

bnoordhuis added the test Issues and PRs related to the tests. label Oct 21, 2015
mscdex added the c++ Issues and PRs that require attention from people who are familiar with C++. label Oct 22, 2015

Copy link
Copy Markdown
Contributor

Technically MakeCallback should only be called as the entry point into the JS stack. Otherwise it will attempt to resolve the nextTickQueue with each nested call.

Copy link
Copy Markdown
Contributor

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

Why pass x?

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

Oh, copy/paste error probably. I'll remove that.

Copy link
Copy Markdown
Contributor

Aside from my first comment and a question, tests LGTM.

kkoopa commented Oct 22, 2015

Copy link
Copy Markdown

@bnoordhuis I see, but what about your comment on changing behavior to use the function's context instead? Does it still make sense to do the Value-to-Object conversion here nodejs/nan#499 or should that be reverted as well?

Copy link
Copy Markdown
Member Author

what about your comment on changing behavior to use the function's context instead?

That doesn't work right when the function is created in another context (see the test at the bottom of the file.)

You could take the receiver's context if recv->IsObject() and the function's context otherwise but that's too confusing / implicit, IMO.

Copy link
Copy Markdown
Member Author

Technically MakeCallback should only be called as the entry point into the JS stack. Otherwise it will attempt to resolve the nextTickQueue with each nested call.

I don't think it matters for this test (no I/O, no timers) but how would you test it otherwise? I suppose I could move more logic to C++ land but on second thought, maybe this should be addressed in MakeCallback() instead: maintain a call depth counter and only flush the queue when it's zero. Thoughts?

Copy link
Copy Markdown
Contributor

Sorry. Let's not hold up this PR for an unrelated issue. For reference this has mainly been discussed in nodejs/nan#284 and nodejs/node-v0.x-archive#9245. I'm taking another stab at it, but something about the timing of when things are called throw me off.

LGTM

Copy link
Copy Markdown
Member Author

Thanks Trevor, landed in 3a091d2.

bnoordhuis closed this Oct 23, 2015
bnoordhuis added a commit that referenced this pull request Oct 23, 2015
PR-URL: #3478
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
bnoordhuis deleted the makecallback-tests branch October 23, 2015 16:45

Copy link
Copy Markdown
Contributor

Should this be LTS? Sorry if this ends up being noise, but I'm unsure if this additional test is tied to a change in v8 or if it would be a useful addition to the LTS test suite.

Copy link
Copy Markdown
Contributor

/cc @jasnell

Copy link
Copy Markdown
Member Author

Not strictly necessary but wouldn't hurt.

bnoordhuis added a commit that referenced this pull request Oct 26, 2015
PR-URL: #3478
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
bnoordhuis added a commit that referenced this pull request Oct 30, 2015
PR-URL: #3478
Reviewed-By: Trevor Norris <trev.norris@gmail.com>

jasnell commented Oct 30, 2015

Copy link
Copy Markdown
Member

Landed in v4.x-staging in d94f4b8

bnoordhuis added a commit that referenced this pull request Oct 30, 2015
PR-URL: #3478
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. test Issues and PRs related to the tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants


Back | FazBrowse Home | New Git URL