Skip to content

test: handle undefined default_configuration - #30465

Closed
codebytere wants to merge 1 commit into
nodejs:masterfrom
codebytere:handle-unset-defaultconfiguration
Closed

test: handle undefined default_configuration#30465
codebytere wants to merge 1 commit into
nodejs:masterfrom
codebytere:handle-unset-defaultconfiguration

Conversation

@codebytere

@codebyterecodebytere commented Nov 13, 2019

Copy link
Copy Markdown
Member

This PR adds a check for process.config.target_defaults being undefined, as it is Electron's case, and ensures that the buildType is set to a default value of Release should it not be defined.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-botnodejs-github-bot added the test Issues and PRs related to the tests. label Nov 13, 2019
@BridgeARBridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 13, 2019
@nodejs-github-bot

nodejs-github-bot commented Nov 13, 2019

Copy link
Copy Markdown
Collaborator

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

trivikr
trivikr reviewed Nov 13, 2019
View reviewed changes
Comment threadtest/common/index.js Outdated
@codebytere
codebytereforce-pushed the handle-unset-defaultconfiguration branch from fe70071 to 7b72997CompareNovember 13, 2019 21:48
@codebytere

codebytere commented Nov 15, 2019•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

@trivikr your suggestion failed lint:

/home/travis/build/nodejs/node/test/common/index.js
122:4 error '?' should be placed at the end of the line operator-linebreak
123:4 error ':' should be placed at the end of the line operator-linebreak

are we fine for me to move back to the previous format?

I could also do this:

constbuildType=process.config.target_defaults ?
process.config.target_defaults.default_configuration :
'Release';

i'm fairly un-opinionated on this front ¯\_(ツ)_/¯

@trivikr

trivikr commented Nov 15, 2019

Copy link
Copy Markdown
Member

Sure, this format works

constbuildType=process.config.target_defaults ?
process.config.target_defaults.default_configuration :
'Release';

I'd posted my review as a nit, suggesting it's optional.

@codebytere
codebytereforce-pushed the handle-unset-defaultconfiguration branch from 7b72997 to 74cf1cdCompareNovember 16, 2019 16:32
@nodejs-github-bot

nodejs-github-bot commented Nov 16, 2019

Copy link
Copy Markdown
Collaborator

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

@codebytere

codebytere commented Nov 18, 2019

Copy link
Copy Markdown
MemberAuthor

Landed in 70281ce

codebytere added a commit that referenced this pull request Nov 18, 2019
@codebytere
test: handle undefined default_configuration
70281ce
PR-URL: #30465
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@codebytere
codebytere deleted the handle-unset-defaultconfiguration branch November 18, 2019 17:24
BridgeAR pushed a commit that referenced this pull request Nov 19, 2019
@codebytere@BridgeAR
test: handle undefined default_configuration
c464ede
PR-URL: #30465
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BridgeARBridgeAR mentioned this pull request Nov 19, 2019
Merged
targos pushed a commit that referenced this pull request Dec 1, 2019
@codebytere@targos
test: handle undefined default_configuration
376ec01
PR-URL: #30465
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BethGriggsBethGriggs mentioned this pull request Dec 9, 2019
Closed
MylesBorins pushed a commit that referenced this pull request Dec 17, 2019
@codebytere@MylesBorins
test: handle undefined default_configuration
e4a1cff
PR-URL: #30465
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BethGriggsBethGriggs mentioned this pull request Dec 23, 2019
Merged
codebytere added a commit to electron/electron that referenced this pull request Jan 8, 2020
@codebytere
Remove default config patch
ae7507d
nodejs/node#30465
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnelljasnelljasnell approved these changes
@addaleaxaddaleaxaddaleax approved these changes
@cjihrigcjihrigcjihrig approved these changes
@richardlaurichardlaurichardlau approved these changes
@BridgeARBridgeARBridgeAR approved these changes
@trivikrtrivikrtrivikr approved these changes
+1 more reviewer
@devnexendevnexendevnexen approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.testIssues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@codebytere@nodejs-github-bot@trivikr@jasnell@addaleax@cjihrig@devnexen@richardlau@BridgeAR