-
Notifications
You must be signed in to change notification settings - Fork 350
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
fix(build): Support running the test suite under Windows #1255
Changes from 4 commits
9c2d1fc
0fe03f8
1877201
84c5eb7
f23d5ca
1d2e171
3f517e9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
root = true | ||
|
||
# Unix-style newlines with a newline ending every file | ||
[*] | ||
end_of_line = lf | ||
insert_final_newline = true |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
* text=auto eol=lf |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -40,7 +40,7 @@ export function writeCSSJSFile(rootPath, originalPath, destinationPath, contents | |
|
||
export function getRelativeImportPath(from, to) { | ||
const parsedTo = path.parse(to); | ||
const newImportPath = path.normalize(path.join(relative(from, parsedTo.dir), parsedTo.base).replace(/\\/g, '')); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not quite sure why we've removed backslashes here. From what I can see, it didn't have any effect on Linux/OS X, and completely broke paths on Windows. Am I missing something? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i'm honestly not sure... but just ensuring you can run There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (rebuilding styles) |
||
const newImportPath = path.normalize(path.join(relative(from, parsedTo.dir), parsedTo.base)); | ||
return newImportPath.startsWith('.') ? newImportPath : `./${newImportPath}`; | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,9 +1,29 @@ | ||
import { EOL as SYSTEM_EOL } from 'os'; | ||
import prettier from 'prettier'; | ||
import { defineInlineTest } from 'jscodeshift/dist/testUtils'; | ||
import transform from './pf3-pf4'; | ||
|
||
const prettierConfig = prettier.resolveConfig.sync(process.cwd()); | ||
const pretty = src => prettier.format(src, { parser: 'babylon', ...prettierConfig }); | ||
/** | ||
* Codemod outputs should follow the EOL pattern of the target codebase, not | ||
* Patternfly's. Patternfly currently enforces LF as line ending, independent of | ||
* the OS building the codebase. | ||
* | ||
* JSCodeShift produces OS-dependent line endings however, LF for Unix based | ||
* systems, and CRLF for Windows based ones. This is also likely what most | ||
* projects would like to see the codemod prduce. | ||
* | ||
* To make sure we both adhere to Patternfly's conventions, and compare the | ||
* correct OS-specific line endings during the test cases' assertions, we store | ||
* expected values with LF line endings, and convert them into the OS-specific | ||
* ones at runtime using prettier. | ||
*/ | ||
const PRETTIER_EOL = SYSTEM_EOL === '\r\n' ? 'crlf' : 'cr'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. With all the extra effort it takes to work around line ending issues, maybe it would be a better approach to disable the line ending lint rule, and let Git ensure that they are correctly set? If Git's There are likely ways to force non-LF endings into the code base with this setup, but that risk may be a worthy tradeoff for the added flexibility? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @dana Thoughts? |
||
const prettierConfig = { | ||
...prettier.resolveConfig.sync(process.cwd()), | ||
parser: 'babel', | ||
endOfLine: PRETTIER_EOL | ||
}; | ||
const pretty = src => prettier.format(src, prettierConfig); | ||
|
||
global.console.log = jest.fn(); | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -149,7 +149,7 @@ module.exports = (file, api, options) => { | |
|
||
return prettier | ||
? prettier.format(transformedSource, { | ||
parser: 'babylon', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why did this need to change from babylon to babel? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I also changed it in #1541 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @dlabaj I had to upgrade Prettier to 1.16.x to support the The babylon parser was deprecated with Prettier 1.16.0 and replaced with the bable parser. In my understanding, it's the same thing, just a different name. |
||
parser: 'babel', | ||
...prettier.resolveConfig.sync(process.cwd()) | ||
}) | ||
: transformedSource; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,9 +1,29 @@ | ||
import { EOL as SYSTEM_EOL } from 'os'; | ||
import prettier from 'prettier'; | ||
import { defineInlineTest, runInlineTest } from 'jscodeshift/dist/testUtils'; | ||
import transform from './pf3-pf4'; | ||
|
||
const prettierConfig = prettier.resolveConfig.sync(process.cwd()); | ||
const pretty = src => prettier.format(src, { parser: 'babylon', ...prettierConfig }); | ||
/** | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i am still seeing some tests fail locally in this file on Mac...happy to give you the output if needed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @priley86 The Travis build is failing too. Looks like that's related to the Prettier upgrade -- some files are missing some formatting it seems. See https://travis-ci.org/patternfly/patternfly-react/builds/486997667#L5198 -- are the issues you are running into the same ones? I disabled the linting locally, since I get mostly noise due to #1256. Looks like it's the PF3 packages that are causing trouble, I should be able to lint them on my machine without problems. I'll take a look! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, i think you've captured it. Seems you just need a few more snapshot updates in react-codemods... |
||
* Codemod outputs should follow the EOL pattern of the target codebase, not | ||
* Patternfly's. Patternfly currently enforces LF as line ending, independent of | ||
* the OS building the codebase. | ||
* | ||
* JSCodeShift produces OS-dependent line endings however, LF for Unix based | ||
* systems, and CRLF for Windows based ones. This is also likely what most | ||
* projects would like to see the codemod prduce. | ||
* | ||
* To make sure we both adhere to Patternfly's conventions, and compare the | ||
* correct OS-specific line endings during the test cases' assertions, we store | ||
* expected values with LF line endings, and convert them into the OS-specific | ||
* ones at runtime using prettier. | ||
*/ | ||
const PRETTIER_EOL = SYSTEM_EOL === '\r\n' ? 'crlf' : 'cr'; | ||
const prettierConfig = { | ||
...prettier.resolveConfig.sync(process.cwd()), | ||
parser: 'babel', | ||
endOfLine: PRETTIER_EOL | ||
}; | ||
const pretty = src => prettier.format(src, prettierConfig); | ||
|
||
global.console.log = jest.fn(); | ||
|
||
|
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.
This upgrade is necessary to enable the
endOfLine
config option. (See https://prettier.io/docs/en/options.html#end-of-line)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.
if upgrading prettier...please run
yarn prettier
to update all formatting across the project w/ this PR. It seems we also need some updates now in.prettierignore
now that i try this locally, but i think it can be done in a different PR 😄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.
@priley86 -- It looks like the upgrade changed a few minor things around how prettier likes to indent stuff.
The linter is picking up those changes in the PF3 packages. There seem to also be a lot of formatting issues in the PF4 packages, which aren't reported right now during the linting step due to #1256.
My suggestion is to fix the formatting in PF3, which only affects around 6 files and should fix the linter errors during the build, and leave fixing the formatting in PF4 for when #1256 is tackled. Touching all those files now will probably draw the wrath of everyone with an open PR on me. 😅
Makes sense to you?
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.
with the conversion to typescript/tslint for pf4 upcoming, i don't know how much time we invest on those eslint rules for pf4 - @dgutride can probably weigh this. Regarding prettier though - i think we should just ensure running
yarn prettier
is consistent for now (certainly realize the other issue exists). I think the pf4 team will have to weigh this. Runningyarn prettier
to format our code now is low effort though ;)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.
f23d5ca fixes the formatting for the pf3 stuff. The build is still broken, but less badly now I suppose, trying to figure out what else is wrong with it...
The linting is not failing anymore at least. If you want me to any other formatting, I'm happy to!