Fix: env unit test fails on Windows #41070
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
t-hamano
requested review from
ntwb,
nerrad,
ajitbohra and
ryanwelcher
as code owners
t-hamano
commented
May 14, 2022
| plugins: [ | ||
| './relative', | ||
| '../parent', | ||
| `${ os.homedir() }/home`, |
There was a problem hiding this comment.
I use os.homeDir() because tilde (~) is not recognized as a home directory path in Windows.
t-hamano
commented
May 14, 2022
| @@ -653,24 +682,24 @@ describe( 'readConfig', () => { | |||
| expect( config.env.development.mappings ).toEqual( { | |||
| test1: { | |||
| basename: 'test1', | |||
| path: '/test1', | |||
| path: resolve( '/test1' ), | |||
There was a problem hiding this comment.
resolve may seem meaningless, but it is necessary to remove the drive letter (e.g. C:) on Windows OS.
There was a problem hiding this comment.
Could you add this as a comment in the code?
|
Size Change: 0 B Total Size: 1.24 MB ℹ️ View Unchanged
|
t-hamano
added
the
[Type] Automated Testing
noahtallen
reviewed
May 14, 2022
| @@ -251,17 +264,17 @@ describe( 'readConfig', () => { | |||
| pluginSources: [ | |||
| { | |||
| type: 'local', | |||
| path: expect.stringMatching( /^\/.*relative$/ ), | |||
| path: expect.stringMatching( /^(\/||\\).*relative$/ ), | |||
There was a problem hiding this comment.
Would it be easier to match based on path.sep for these tests?
There was a problem hiding this comment.
if we use variables( path.sep ) in a regular expression, we need to generate a regExp.
I used this method because I think newRregExp makes the code verbose.
Also, I could not figure out how to embed a non-escaping backslash in the regexp 😅
| @@ -653,24 +682,24 @@ describe( 'readConfig', () => { | |||
| expect( config.env.development.mappings ).toEqual( { | |||
| test1: { | |||
| basename: 'test1', | |||
| path: '/test1', | |||
| path: resolve( '/test1' ), | |||
There was a problem hiding this comment.
Could you add this as a comment in the code?
|
Thanks for the review! |
noahtallen
approved these changes
May 16, 2022
There was a problem hiding this comment.
These changes make sense to me. Only caveat is that it seems easy enough for something to cause a regression in this area, as we don't have a way to run CI tests in Windows (as far as I know). That said, hopefully these changes are enough to encourage any future devs to be mindful of including os-specific paths :)
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.
Part of #39388
What?
This PR fixes a problem that wp-env unit test fails on Windows OS.
This PR will allow Jest unit tests to be complete correctly on Windows OS.
Why?
The main cause is that the path separator of the
.wp-jsonvalues read viareadConfigis backslashes in Windows OS.The information on this variable is as follows:
See variable information
How?
I replaced forward slashes with OS delimiter and fixed other OS-dependent problems.
Testing Instructions
One test
npm run test-unit -- ./packages/env/lib/config/test/config.jsAll tests
npm run test-unitIf the tests fails, narrow down
maxWorkersas follows: