fix(build): Support running the test suite under Windows by TimoStaudinger · Pull Request #1255 · patternfly/patternfly-react

TimoStaudinger · 2019-01-25T21:53:07Z

Running the linter and test suite under Windows currently fails. The changes proposed by this PR should resolve this.

What

Check out and create new files with Unix style line endings: By default, files are checked out and created with CRLF line endings on Windows. This is rejected by the 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)
Properly load generated files from fs mock when testing react-tokens generation: The generated files and their contents are compared against a snapshot. Only the last two segments of the file path are used to identify a file, to make the comparison independent of the absolute location in the file system. To correctly extract this piece of the file path, the test script needs to handle back slashes (used on Windows) as well as forward slashes (used on Linux/OS X). (See 024f77e)
Use OS-specific line endings for expected codemod outputs: Codemods should produce code which uses OS-specific line endings, which is already the case. However, it used to be compared against an expected value that always used LF line endings. To fix this, the expected values are formatted to use OS-specific line endings at runtime. (See 240f25c)
Generate proper stylesheet imports during react-core build: The generated file paths in react-core, which are used to import .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)

🎉

patternfly-build · 2019-01-25T22:21:20Z

PatternFly-React preview: https://1255-pr-patternfly-react-patternfly.surge.sh

coveralls · 2019-01-25T22:23:54Z

Pull Request Test Coverage Report for Build 4256

3 of 5 (60.0%) changed or added relevant lines in 3 files are covered.
3 unchanged lines in 3 files lost coverage.
Overall coverage increased (+0.03%) to 80.225%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
packages/patternfly-3/react-console/src/AccessConsoles/AccessConsoles.js	2	3	66.67%
packages/patternfly-4/react-styles/src/build/util.js	0	1	0.0%

Files with Coverage Reduction	New Missed Lines	%
packages/patternfly-4/react-core/src/layouts/Toolbar/ToolbarSection.js	1	84.62%
packages/patternfly-4/react-core/src/components/Modal/ModalBoxCloseButton.js	1	85.71%
packages/patternfly-4/react-table/src/components/Table/Table.js	1	84.21%

Totals
Change from base Build 4223:	0.03%
Covered Lines:	4632
Relevant Lines:	5424

💛 - Coveralls

dlabaj

@timosta Does pf-react environment work on Windows? Is this all that's needed for support to run tests?

TimoStaudinger · 2019-01-28T18:23:45Z

@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.

TimoStaudinger · 2019-01-28T18:32:51Z

    "npmlog": "^4.1.2",
    "plop": "^2.0.0",
-    "prettier": "^1.14.2",
+    "prettier": "^1.16.1",


This upgrade is necessary to enable the endOfLine config option. (See https://prettier.io/docs/en/options.html#end-of-line)

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 😄

@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?

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 ;)

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!

TimoStaudinger · 2019-01-28T18:36:56Z

+ * 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';


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?

I agree. @tlabaj @jschuler Thoughts?

@dana Thoughts?

TimoStaudinger · 2019-01-28T21:24:41Z


 export function getRelativeImportPath(from, to) {
  const parsedTo = path.parse(to);
-  const newImportPath = path.normalize(path.join(relative(from, parsedTo.dir), parsedTo.base).replace(/\\/g, ''));


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?

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...

(rebuilding styles)

TimoStaudinger · 2019-01-28T21:33:16Z

@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.

priley86 · 2019-02-01T16:05:20Z


-const prettierConfig = prettier.resolveConfig.sync(process.cwd());
-const pretty = src => prettier.format(src, { parser: 'babylon', ...prettierConfig });
+/**


i am still seeing some tests fail locally in this file on Mac...happy to give you the output if needed.

@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!

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 😸

dlabaj · 2019-02-01T14:47:45Z


    return prettier
      ? prettier.format(transformedSource, {
-          parser: 'babylon',


Why did this need to change from babylon to babel?

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

@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.

dgutride · 2019-03-11T18:36:34Z

@redallen - can you take a look, too?

redallen · 2019-03-11T18:52:20Z

Checked it out locally, tests should pass after updating yarn.lock. I like the changes.

TimoStaudinger · 2019-03-11T18:57:18Z

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?

redallen · 2019-03-11T18:58:36Z

No. Run yarn install in the project root and then commit yarn.lock.

Edit: It should just change yarn.lock#16702 from prettier@^1.16.0 to prettier@^1.16.0, prettier@^1.16.1:. I did it for you.

codecov-io · 2019-03-11T19:22:36Z

Codecov Report

Merging #1255 into master will increase coverage by 0.01%.
The diff coverage is 60%.

@@            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

Flag	Coverage Δ
#patternfly3	`84.89% <75%> (ø)`	⬆️
#patternfly4	`81.39% <0%> (+0.05%)`	⬆️

Impacted Files	Coverage Δ
...rc/components/VerticalNav/VerticalNavItemHelper.js	`43.61% <ø> (ø)`	⬆️
...ponents/CatalogTileView/CatalogTileViewCategory.js	`90% <ø> (ø)`	⬆️
...ckages/patternfly-4/react-styles/src/build/util.js	`8.33% <0%> (ø)`	⬆️
...ents/LoginCardComponents/LoginCardSocialColumns.js	`87.5% <100%> (ø)`	⬆️
...react-console/src/AccessConsoles/AccessConsoles.js	`88.57% <66.66%> (ø)`	⬆️
...fly-4/react-core/src/components/Dropdown/Toggle.js	`58.62% <0%> (ø)`	⬆️
...atternfly-4/react-core/src/components/Tabs/Tabs.js	`95% <0%> (ø)`	⬆️
...fly-4/react-core/src/components/Tabs/TabContent.js	`100% <0%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 96435b9...3f517e9. Read the comment docs.

redallen · 2019-03-11T19:24:34Z

I'll merge with one more reviewer.

tlabaj

LGTM

tlabaj added PF4 labels Jan 28, 2019

dlabaj reviewed Jan 28, 2019

View reviewed changes

TimoStaudinger commented Jan 28, 2019

View reviewed changes

TimoStaudinger changed the title ~~WIP: fix(build): Support running the test suite under Windows~~ fix(build): Support running the test suite under Windows Jan 28, 2019

TimoStaudinger mentioned this pull request Jan 31, 2019

fix(build): Support running npm scripts under Windows #1251

Merged

TimoStaudinger added 4 commits January 31, 2019 10:39

Check out and create new files with Unix style LF line endings

9c2d1fc

By default, files will be checked out and created with CRLF line endings under Windows. This is rejected by the repository's ESLint rules.

Fix react-tokens test utilities for Windows

0fe03f8

Apply correct line endings to expected codemod outputs during testing

1877201

Generate correct style sheet import statements during react-core build

84c5eb7

dlabaj previously approved these changes Feb 1, 2019

View reviewed changes

priley86 reviewed Feb 1, 2019

View reviewed changes

Fix formatting of Patternfly 3 packages

f23d5ca

TimoStaudinger dismissed dlabaj’s stale review via f23d5ca February 1, 2019 16:37

tlabaj assigned dlabaj Mar 6, 2019

Merge branch 'master' into fix/test-suite-on-windows

1d2e171

dlabaj previously approved these changes Mar 11, 2019

View reviewed changes

dgutride requested a review from redallen March 11, 2019 18:36

bump yarn.lock

3f517e9

redallen dismissed dlabaj’s stale review via 3f517e9 March 11, 2019 19:00

redallen approved these changes Mar 11, 2019

View reviewed changes

tlabaj approved these changes Mar 11, 2019

View reviewed changes

redallen merged commit 851756e into patternfly:master Mar 11, 2019

TimoStaudinger deleted the fix/test-suite-on-windows branch March 11, 2019 20:02

This was referenced Mar 12, 2019

fix(codemod): fix tests failing due to prettier #1555

Merged

build(tests): add misc test build target to travis #1556

Merged

jschuler mentioned this pull request Mar 12, 2019

Need dev/build environment support for Windows #1557

Closed

Conversation

TimoStaudinger commented Jan 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

Uh oh!

patternfly-build commented Jan 25, 2019

Uh oh!

coveralls commented Jan 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 4256

💛 - Coveralls

Uh oh!

dlabaj left a comment

Choose a reason for hiding this comment

Uh oh!

TimoStaudinger commented Jan 28, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TimoStaudinger Jan 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TimoStaudinger commented Jan 28, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

priley86 Feb 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

redallen Mar 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dgutride commented Mar 11, 2019

Uh oh!

redallen commented Mar 11, 2019

Uh oh!

TimoStaudinger commented Mar 11, 2019

Uh oh!

redallen commented Mar 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Mar 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

redallen commented Mar 11, 2019

Uh oh!

tlabaj left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

TimoStaudinger commented Jan 25, 2019 •

edited

Loading

coveralls commented Jan 25, 2019 •

edited

Loading

TimoStaudinger Jan 28, 2019 •

edited

Loading

priley86 Feb 1, 2019 •

edited

Loading

redallen Mar 11, 2019 •

edited

Loading

redallen commented Mar 11, 2019 •

edited

Loading

codecov-io commented Mar 11, 2019 •

edited

Loading