-
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
fix(build): Support running the test suite under Windows #1255
Conversation
PatternFly-React preview: https://1255-pr-patternfly-react-patternfly.surge.sh |
Pull Request Test Coverage Report for Build 4256
💛 - Coveralls |
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.
@timosta Does pf-react environment work on Windows? Is this all that's needed for support to run tests?
@dlabaj Not quite, there's some other issues that still need some tweaking. I'm trying to resolve those missing pieces right now, but overall it seems to be fairly straightforward to make it work. |
@@ -51,7 +51,7 @@ | |||
"minimist": "^1.2.0", | |||
"npmlog": "^4.1.2", | |||
"plop": "^2.0.0", | |||
"prettier": "^1.14.2", | |||
"prettier": "^1.16.1", |
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. Running yarn 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!
* 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 comment
The 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 core.autocrlf
option is set correctly, it should make sure that all files have LF line endings after check-in, but devs can work with the OS-specific line endings locally.
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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
@dana Thoughts?
@@ -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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
i'm honestly not sure... but just ensuring you can run yarn clean; yarn build; yarn start:pf4
and we verifying we still have styles loading correctly should hopefully verify this...
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.
(rebuilding styles)
@dlabaj The tests are now running fine on Windows for me with the proposed changes. I'd appreciate a review and feedback! |
By default, files will be checked out and created with CRLF line endings under Windows. This is rejected by the repository's ESLint rules.
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 comment
The 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 comment
The 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 comment
The 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... react-codemods
is not yet used in the wild (still a P.O.C.) so definitely not a risk right now. Just need to ensure tests pass. I will try to pull your branch again and build locally on Mac after Travis is happy 😸
@@ -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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
console.warn node_modules/prettier/index.js:440
{ parser: "babylon" } is deprecated; we now treat it as { parser: "babel" }.
I also changed it in #1541
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.
@dlabaj I had to upgrade Prettier to 1.16.x to support the endOfLine
parameter.
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.
@redallen - can you take a look, too? |
Checked it out locally, tests should pass after updating |
I haven't had the time to look into the build failure yet myself unfortunately, but I'll try to find time to fix it over the next one or two days. @redallen -- You're saying that merging master into this should help, right? Or is there anything specific around the lockfile that needs to be updated? |
No. Run Edit: It should just change |
Codecov Report
@@ Coverage Diff @@
## master #1255 +/- ##
==========================================
+ Coverage 83.6% 83.61% +0.01%
==========================================
Files 551 552 +1
Lines 5744 5750 +6
Branches 12 12
==========================================
+ Hits 4802 4808 +6
Misses 940 940
Partials 2 2
Continue to review full report at Codecov.
|
I'll merge with one more reviewer. |
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
Running the linter and test suite under Windows currently fails. The changes proposed by this PR should resolve this.
What
linebreak-style
ESLint rule, which requires Unix style LF line breaks. A.gitattributes
file was added to prevent automatic conversion to Windows style linebreaks during checkout, and a.editorconfig
file was added to instruct IDEs to create new files with Unix style linebreaks. (See b0d83fc).css.js
files created during the build were inserted without slashes between path segments, resulting in failed imports. This is corrected, and the import path segments are now correctly separated by slashes. (See b63f8c0)🎉