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

test: partition N-API tests #24557

Closed

Conversation

gabrielschulhof
Copy link
Contributor

Partition test/addons-napi into test/js-native-api and test/node-api to
isolate the Node.js-agnostic portion of the N-API tests from the
Node.js-specific portion.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Nov 21, 2018
@gabrielschulhof gabrielschulhof added test Issues and PRs related to the tests. node-api Issues and PRs related to the Node-API. labels Nov 21, 2018
@gabrielschulhof
Copy link
Contributor Author

@bghgary would this help test your implementation of N-API?

test/README.md Outdated Show resolved Hide resolved
@bghgary
Copy link
Contributor

bghgary commented Nov 21, 2018

would this help test your implementation of N-API?

I scanned the changes, but I'm not familiar with the build/test infrastructure. It looks like it will help. Does this build without my PR? I have been curious why the tests don't catch the header issue that I'm having.

@gabrielschulhof
Copy link
Contributor Author

@bghgary I guess you're right. We won't really know whether the tests in test/js-native-api will build/run until we try to build/run them out-of-tree. The idea is that nothing in any of the subdirectories of test/js-native-api refers to node_api.h, but only test/js-native-api/entry_point.c, which basically turns each test into a N-API addon.

@gabrielschulhof
Copy link
Contributor Author

... and then a different test/js-native-api/entry_point.c can be defined for out-of-tree testing. Each test exports a symbol napi_value Init(napi_env env, napi_value exports); which the entry point can call to get the bindings in an object.

@bghgary
Copy link
Contributor

bghgary commented Nov 22, 2018

turns each test into a N-API addon

That would be great!

@refack
Copy link
Contributor

refack commented Nov 22, 2018

Kinda RSLGTM, if it passes CI it's good (binary make is better at grokking this then me).
https://ci.nodejs.org/job/node-test-pull-request/18860/

@gabrielschulhof
Copy link
Contributor Author

@gabrielschulhof
Copy link
Contributor Author

OK, I don't get it. What's wrong with

{
  "targets": [
    {
      "target_name": "test_constructor",
      "sources": [
        "test_constructor.c",
        "../entry_point.c"
      ]
    },
    {
      "target_name": "test_constructor_name",
      "sources": [
        "test_constructor_name.c",
        "../entry_point.c"
      ]
    }
  ]
}

It can't seem to find entry_point.c. Yet other tests use the same notation, and, when building those, it can find entry_point.c just fine 😕

@refack
Copy link
Contributor

refack commented Nov 23, 2018

"../entry_point.c"

I think it's a GYP limitation. It's not allowed to look outside of the scope of the root .gyp file. Unless you use a CLI parameter called --depth.
You might be able to workaround it by using a base .gyp file in /test/js-native-api and play with including spesific.gypi. Or the other way around.
But I'm just guessing, haven't tried it.

(Also if it's possible to create this whole thing as 1 addon with multiple test entry points, that would be great for test run performance, since compiling each addon has a very big time overhead)

@gabrielschulhof
Copy link
Contributor Author

@refack thanks! I hope I can "includes": [ "../js_native_api.gypi" ] from a given test. Let's see ...
CI: https://ci.nodejs.org/job/node-test-pull-request/18905/

@gabrielschulhof
Copy link
Contributor Author

Argh! No help. I'll have to try some other way.

@refack
Copy link
Contributor

refack commented Nov 24, 2018

Argh! No help. I'll have to try some other way.

Since I assume it's a GYP limitation, I'll dig into this as soon as possible.

@gabrielschulhof
Copy link
Contributor Author

@refack thanks! I don't understand why it's only happening on some platforms. I would imagine that the version of node-gyp is the same on all of them, given that node-gyp is taken from Node.js' own source tree. I guess this is some platform-specific behaviour. The other interesting thing is that the build seems to work on all non-Linux platforms.

@gabrielschulhof
Copy link
Contributor Author

@refack according to this gyp doc it should be possible to add files of the form ../path/to/source.c.

@gabrielschulhof
Copy link
Contributor Author

sigh tried sorting the files alphabetically, as strongly encouraged by the gyp doc shrug
CI: https://ci.nodejs.org/job/node-test-pull-request/18908/

@gabrielschulhof
Copy link
Contributor Author

Nope.

@refack
Copy link
Contributor

refack commented Nov 24, 2018

@refack according to this gyp doc it should be possible to add files of the form ../path/to/source.c.

BTW, check this out http://gyp3.org/docs/UserDocumentation.html#add-a-source-file-that-builds-on-all-platforms

@gabrielschulhof
Copy link
Contributor Author

Lessee if it's only one addon in test_constructor ...
CI: https://ci.nodejs.org/job/node-test-pull-request/18911/

@gabrielschulhof
Copy link
Contributor Author

OK, consolidating the two addons into one seems to have done it.

Trott added a commit to Trott/io.js that referenced this pull request Dec 5, 2018
Refs: nodejs#24557 (comment)

PR-URL: nodejs#24839
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
BridgeAR pushed a commit that referenced this pull request Dec 5, 2018
Partition test/addons-napi into test/js-native-api and test/node-api to
isolate the Node.js-agnostic portion of the N-API tests from the
Node.js-specific portion.

PR-URL: #24557
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
BridgeAR pushed a commit that referenced this pull request Dec 5, 2018
Refs: #24557 (comment)

PR-URL: #24839
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
BridgeAR pushed a commit that referenced this pull request Dec 5, 2018
Refs: #24557 (comment)

PR-URL: #24839
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
@BridgeAR BridgeAR mentioned this pull request Dec 5, 2018
4 tasks
BridgeAR pushed a commit that referenced this pull request Dec 7, 2018
Partition test/addons-napi into test/js-native-api and test/node-api to
isolate the Node.js-agnostic portion of the N-API tests from the
Node.js-specific portion.

PR-URL: #24557
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
BridgeAR pushed a commit that referenced this pull request Dec 7, 2018
Refs: #24557 (comment)

PR-URL: #24839
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
BridgeAR pushed a commit that referenced this pull request Dec 7, 2018
Refs: #24557 (comment)

PR-URL: #24839
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
BridgeAR pushed a commit that referenced this pull request Dec 7, 2018
Partition test/addons-napi into test/js-native-api and test/node-api to
isolate the Node.js-agnostic portion of the N-API tests from the
Node.js-specific portion.

PR-URL: #24557
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
BridgeAR pushed a commit that referenced this pull request Dec 7, 2018
Refs: #24557 (comment)

PR-URL: #24839
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
BridgeAR pushed a commit that referenced this pull request Dec 7, 2018
Refs: #24557 (comment)

PR-URL: #24839
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
@addaleax
Copy link
Member

addaleax commented Dec 9, 2018

@gabrielschulhof I think this patch is responsible for, or exacerbates, a race condition when running make -j4 test… Somehow the node binary and the addon tests end up compiling at the same time, which doesn’t work while the node binary itself is being linked?

refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
Partition test/addons-napi into test/js-native-api and test/node-api to
isolate the Node.js-agnostic portion of the N-API tests from the
Node.js-specific portion.

PR-URL: nodejs#24557
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
Refs: nodejs#24557 (comment)

PR-URL: nodejs#24839
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
Refs: nodejs#24557 (comment)

PR-URL: nodejs#24839
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
BethGriggs pushed a commit that referenced this pull request Feb 12, 2019
Refs: #24557 (comment)

PR-URL: #24839
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
BethGriggs pushed a commit that referenced this pull request Feb 12, 2019
Refs: #24557 (comment)

PR-URL: #24839
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
BethGriggs pushed a commit that referenced this pull request Feb 20, 2019
Refs: #24557 (comment)

PR-URL: #24839
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
BethGriggs pushed a commit that referenced this pull request Feb 20, 2019
Refs: #24557 (comment)

PR-URL: #24839
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
rvagg pushed a commit that referenced this pull request Feb 28, 2019
Refs: #24557 (comment)

PR-URL: #24839
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
rvagg pushed a commit that referenced this pull request Feb 28, 2019
Refs: #24557 (comment)

PR-URL: #24839
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. node-api Issues and PRs related to the Node-API. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants