-
Notifications
You must be signed in to change notification settings - Fork 30k
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: improve UV_THREADPOOL_SIZE
tests on .env
#49213
Conversation
|
Looking at the error messages, this is likely due to some platforms not running the node-api tests, either not at all or not on the same machine that runs this test. Does that sound right @richardlau? If that's the case, perhaps just skip the test if the file is missing. |
Yes, that is correct. If the test needs the addon from |
I agree that that makes sense, but FWIW, there is precedent for skipping parts of tests when a shared library is missing: node/test/addons/openssl-test-engine/test.js Lines 34 to 38 in 9cd70f4
|
@tniessen In that particular case I think it's because the engine is not compiled on all platforms: node/test/addons/openssl-test-engine/binding.gyp Lines 8 to 11 in 9cd70f4
That is not the case for |
e4c08f4
to
e24414c
Compare
Thank you @StefanStojanovic. I've applied your suggestions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Commit Queue failed- Loading data for nodejs/node/pull/49213 ✔ Done loading data for nodejs/node/pull/49213 ----------------------------------- PR info ------------------------------------ Title test: improve `UV_THREADPOOL_SIZE` tests on `.env` (#49213) Author Yagiz Nizipli (@anonrig) Branch anonrig:test-dotenv-improve -> nodejs:main Labels test, cli, needs-ci Commits 1 - test: improve `UV_THREADPOOL_SIZE` tests on `.env` Committers 1 - Yagiz Nizipli PR-URL: https://github.com/nodejs/node/pull/49213 Reviewed-By: Chemi Atlow ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/49213 Reviewed-By: Chemi Atlow -------------------------------------------------------------------------------- ⚠ Commits were pushed since the last approving review: ⚠ - test: improve `UV_THREADPOOL_SIZE` tests on `.env` ℹ This PR was created on Thu, 17 Aug 2023 14:19:12 GMT ✔ Approvals: 1 ✔ - Chemi Atlow (@atlowChemi): https://github.com/nodejs/node/pull/49213#pullrequestreview-1585715138 ✔ Last GitHub CI successful ℹ Last Full PR CI on 2023-11-01T01:47:03Z: https://ci.nodejs.org/job/node-test-pull-request/55373/ - Querying data for job/node-test-pull-request/55373/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncuhttps://github.com/nodejs/node/actions/runs/6748150235 |
Landed in 4dbb017 |
PR-URL: nodejs#49213 Reviewed-By: Chemi Atlow <[email protected]> Reviewed-By: Geoffrey Booth <[email protected]>
PR-URL: #49213 Reviewed-By: Chemi Atlow <[email protected]> Reviewed-By: Geoffrey Booth <[email protected]>
PR-URL: #49213 Reviewed-By: Chemi Atlow <[email protected]> Reviewed-By: Geoffrey Booth <[email protected]>
PR-URL: #49213 Reviewed-By: Chemi Atlow <[email protected]> Reviewed-By: Geoffrey Booth <[email protected]>
Follow up after @tniessen's pull request #49165