nodejs / node

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

Jump to bottom

node-api: refactor to cctest v8impl::Reference #38970

Closed

legendecas wants to merge 3 commits into nodejs:master from legendecas:node-api/internal-test

Member

legendecas commented Jun 8, 2021 •

edited

Refactor node_api.cc and js_native_api_v8.cc to expose internal representatives with node_api_internals.h and js_native_api_v8.h respectively. In that way we can test internal codebase with cctest and verify hard-to-reproduce code paths.

With the new test suite, we can verify crashes fixed by #38899 in a stable way (i.e. the test crash every time without #38899)

/cc @nodejs/node-api

legendecas added the node-api label

legendecas requested review from gabrielschulhof and mhdawson

github-actions bot added lib / src needs-ci labels

legendecas force-pushed the legendecas:node-api/internal-test branch from e339fce to 72528c7

legendecas marked this pull request as ready for review

legendecas force-pushed the legendecas:node-api/internal-test branch 2 times, most recently from 95b92e8 to 2e97174


          node-api: cctest on v8impl::Reference

d6054a3

legendecas force-pushed the legendecas:node-api/internal-test branch from bbf5e1f to d6054a3

legendecas added the request-ci label

github-actions bot removed the request-ci label

nodejs-github-bot commented Jun 11, 2021

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

mhdawson reviewed

src/js_native_api_v8.h Outdated

  
      @@ -3,6 +3,7 @@
    
        // This file needs to be compatible with C compilers.

        #include <string.h>  // NOLINT(modernize-deprecated-headers)

        #include "gtest/gtest_prod.h"

mhdawson Jun 11, 2021
Member

In Node-api meeting @gabrielschulhof suggested this should be in "js_native_api_v8_internals.h" instead.


          fixup! node-api: cctest on v8impl::Reference

88c29c8

Member

mhdawson commented Jun 16, 2021

@legendecas I think we should leave the LICENCE file in src/gtest/LICENSE in addition to the reference you added. That seems consistent with the copy in test/cctest/gtest.


          fixup! review suggestion

2f5e43a

legendecas added the request-ci label

mhdawson approved these changes

Member

mhdawson left a comment

LGTM

nodejs-github-bot commented Jun 17, 2021 •

edited by legendecas

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

00:07:31 not ok 714 parallel/test-worker-cleanexit-with-moduleload
00:07:31 ---
00:07:31 duration_ms: 5.795
00:07:31 severity: crashed
00:07:31 exitcode: -11
00:07:31 stack: |-

Member Author

legendecas commented Jun 17, 2021

So it seems like the label bot is not working...

legendecas removed the request-ci label

nodejs-github-bot commented Jun 17, 2021

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

Member Author

legendecas commented Jun 23, 2021 •

edited

@gabrielschulhof hey👋 , it will be great if you can take a look at this. thanks!

gabrielschulhof approved these changes

Contributor

gabrielschulhof left a comment

LGTM, but did you check that this crashes when the fixes we made earlier are absent?

Member Author

legendecas commented Jul 5, 2021

@gabrielschulhof yeah, I did run the test against the build without the fix patch it does crash every time.

Member Author

legendecas commented Jul 5, 2021

Landed in 392213a

legendecas closed this

legendecas deleted the legendecas:node-api/internal-test branch

legendecas added a commit that referenced this pull request


          node-api: cctest on v8impl::Reference

392213a

PR-URL: #38970
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Gabriel Schulhof <gabrielschulhof@gmail.com>

ejose19 added a commit to ejose19/node that referenced this pull request


          node-api: cctest on v8impl::Reference

6fccab7

PR-URL: nodejs#38970
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Gabriel Schulhof <gabrielschulhof@gmail.com>

targos added a commit that referenced this pull request


          node-api: cctest on v8impl::Reference

63f8702

PR-URL: #38970
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Gabriel Schulhof <gabrielschulhof@gmail.com>

targos mentioned this pull request

v16.5.0 release proposal #39373

Merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment