Increased specificity of Windows platform check #2262
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Daniel15
requested changes
Dec 15, 2016
| @@ -68,7 +68,7 @@ export async function run( | |||
| for (const action of actions) { | |||
| const cmd = scripts[action]; | |||
| if (cmd) { | |||
| const isWin = /win/.test(process.platform); | |||
| const isWin = /win32/.test(process.platform); | |||
There was a problem hiding this comment.
Could this just be process.platform === 'win32'? Looks like that's what Node.js itself does, so it should be safe: https://github.com/nodejs/node/blob/db1087c9757c31a82c50a1eba368d8cba95b57d0/lib/os.js#L6
There was a problem hiding this comment.
Certainly. I noticed another section in execute-lifecycle that does just that.
Daniel15
reviewed
Dec 15, 2016
| @@ -131,3 +131,11 @@ export async function run<T, R>( | |||
| throw new Error(`${err && err.stack} \nConsole output:\n ${out}`); | |||
| } | |||
| } | |||
| export const setPlatform = (platform : string) : string => { | |||
| const previous = process.platform; | |||
| Object.defineProperty(process, 'platform', { | |||
There was a problem hiding this comment.
I wish Node.js had a decent dependency injection framework, this is super hacky 😛
There was a problem hiding this comment.
I'm going to redo that. It's not effective at all 😰 I originally was pondering just passing platform into run so the test could directly send it, but, to your point, that seems corny.
Daniel15
pushed a commit
that referenced
this pull request
Dec 20, 2016
The original test asserts that window cmd line formatting is properly applied or not applied, for only the target platform the tests are executed on - that will have to be good enough. It is not possible to test both branches from one platform because the path module employed in the test alters its export based on process.platform in Node bootstrap, well before one can mock it.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Extended regex to match only Windows. Previous regex was matching Darwin in addition to Windows.
yarn run test > all tests pass.
#2261