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

[HELP WANTED] Fix bugs in named interceptor test. #781

Closed
wants to merge 4 commits into from

Conversation

mkrufky
Copy link
Collaborator

@mkrufky mkrufky commented May 26, 2018

Copied, more or less, from commit 946377f

Returning a string in the enumerator was never allowed but wasn't
enforced until recently. In Node.js 10, it hits a CHECK in V8.

This builds, but the tests fail:

#
# Fatal error in , line 0
# Check failed: element->IsName().
#
#
#
#FailureMessage Object: 0x7ffd987f3690not ok test/js/namedinterceptors-test.js ................ 0/1
    Command: "/home/mk/.nvm/versions/node/v10.2.1/bin/node --expose-gc namedinterceptors-test.js"
    TAP version 13
    not ok 1 test/js/namedinterceptors-test.js
      ---
        exit:    ~
        signal:  SIGILL
        stderr:  |
          #
          # Fatal error in , line 0
          # Check failed: element->IsName().
          #
          #
          #
          #FailureMessage Object: 0x7ffd987f3690
        command: "/home/mk/.nvm/versions/node/v10.2.1/bin/node --expose-gc namedinterceptors-test.js"
      ...
    
    1..1
    # tests 1
    # fail  1
    

I'm not sure what else is needed but this is at least a starting point for fixing the Node 10 CI failure.

Copied, more or less, from commit 946377f

Returning a string in the enumerator was never allowed but wasn't
enforced until recently.  In Node.js 10, it hits a CHECK in V8.
@kkoopa
Copy link
Collaborator

kkoopa commented May 26, 2018

Seems like this was not the right fix, the array elements must be strings (or symbols) https://github.com/nodejs/node/blob/master/deps/v8/include/v8.h#L5134

@mkrufky
Copy link
Collaborator Author

mkrufky commented May 26, 2018

Well, it wasn't as simple as 946377f :-P

I tried, but I'm closing this.

@mkrufky mkrufky closed this May 26, 2018
@bnoordhuis
Copy link
Member

I suppose it must be some recent Node.js change because it wasn't failing for me two weeks ago when I fixed the other test. I'll have a look.

@bnoordhuis
Copy link
Member

#782. Note it didn't hit a CHECK for me but the Object.keys(interceptor)[0] === 'value' check was failing.

@kkoopa
Copy link
Collaborator

kkoopa commented May 26, 2018

I suppose it must be some recent Node.js change because it wasn't failing for me two weeks ago when I fixed the other test. I'll have a look.

Yes, I remember everything passing after the indexed interceptor test fix, which confused me now. Semver, schmemver... I am happy it is sorted out now.

@bnoordhuis
Copy link
Member

I'm reasonably sure it's caused by nodejs/node#20350, which I would say is a legitimate bug fix (but I signed off on it so of course I would say that :-))

@mkrufky
Copy link
Collaborator Author

mkrufky commented May 26, 2018

I don't think it was caused by a new regression - the named interceptor test was failing the moment we added node 10 to the matrix:
https://travis-ci.org/nodejs/nan/builds/377939805

...but it's passing now. Thanks :-)

@mkrufky mkrufky deleted the named-interceptor branch May 28, 2018 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants