test: add node::MakeCallback() test coverage

[bnoordhuis]

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/

[trevnorris]

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.

[trevnorris]

Why pass x?

[bnoordhuis]

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

[trevnorris]

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

[kkoopa]

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

[bnoordhuis]

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.

[bnoordhuis]

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?

[trevnorris]

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

[bnoordhuis]

Thanks Trevor, landed in 3a091d2.

[MylesBorins]

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.

[MylesBorins]

/cc @jasnell

[bnoordhuis]

Not strictly necessary but wouldn't hurt.

[jasnell]

Landed in v4.x-staging in d94f4b8