-
Notifications
You must be signed in to change notification settings - Fork 459
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: removed the usage of default_configuration. #1226
Conversation
test/common/index.js
Outdated
if (await checkBuildType(buildTypes.Debug)) { | ||
buildType = buildTypes.Debug; | ||
let buildType; | ||
const envBuildType = process.env.BUILD_TYPE; |
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.
const envBuildType = process.env.BUILD_TYPE; | |
const envBuildType = process.env.BUILD_TYPE || 'Release'; |
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.
Suggestion in meeting from @vmoroz how about process.env.NODE_API_BUILD_CONFIG
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
PR-URL: #1226 Reviewed-By: Michael Dawson <midawson@redhat.com Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Landed as 10ad762 |
PR-URL: nodejs/node-addon-api#1226 Reviewed-By: Michael Dawson <midawson@redhat.com Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
This PR remove the usage of
process.config.target_defaults.default_configuration
in all test suite.A new function
whichBuildType
has been introduced and it has the reponsability to check which type of buildnode-gyp
made so the test can refer to the right folders.Refs: