Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix(build): Support running the test suite under Windows #1255

Merged
merged 7 commits into from
Mar 11, 2019
Merged

fix(build): Support running the test suite under Windows #1255

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
9c2d1fc
Check out and create new files with Unix style LF line endings
TimoStaudinger Jan 25, 2019
0fe03f8
Fix react-tokens test utilities for Windows
TimoStaudinger Jan 28, 2019
1877201
Apply correct line endings to expected codemod outputs during testing
TimoStaudinger Jan 28, 2019
84c5eb7
Generate correct style sheet import statements during react-core build
TimoStaudinger Jan 28, 2019
f23d5ca
Fix formatting of Patternfly 3 packages
TimoStaudinger Feb 1, 2019
1d2e171
Merge branch 'master' into fix/test-suite-on-windows
dlabaj Mar 11, 2019
3f517e9
bump yarn.lock
redallen Mar 11, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .editorconfig
View file
Open in desktop
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
1 change: 1 addition & 0 deletions .gitattributes
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
* text=auto eol=lf
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@
"minimist": "^1.2.0",
"npmlog": "^4.1.2",
"plop": "^2.0.0",
"prettier": "^1.14.2",
"prettier": "^1.16.1",
Copy link
Author

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)

Copy link
Member

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 😄

Copy link
Author

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?

Copy link
Member

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

Copy link
Author

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!

"prettier-eslint": "^8.8.1",
"prop-types": "^15.6.1",
"raf": "^3.4.0",
Expand Down
25 changes: 12 additions & 13 deletions ...y-3/patternfly-react-extensions/src/components/CatalogTileView/CatalogTileViewCategory.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,19 +93,18 @@ class CatalogTileViewCategory extends React.Component {
<div className={classes} {...props} ref={this.handleRef}>
<div className="catalog-tile-view-pf-category-header">
<span className="catalog-tile-view-pf-category-header-title">{title}</span>
{!viewAll &&
numShown < totalItems && (
<span className="pull-right">
<Button
bsStyle="link"
className="catalog-tile-view-pf-category-view-all pull-right"
style={{ marginRight: rightSpacerWidth }}
onClick={onViewAll}
>
{`${viewAllText} (${totalItems})`}
</Button>
</span>
)}
{!viewAll && numShown < totalItems && (
<span className="pull-right">
<Button
bsStyle="link"
className="catalog-tile-view-pf-category-view-all pull-right"
style={{ marginRight: rightSpacerWidth }}
onClick={onViewAll}
>
{`${viewAllText} (${totalItems})`}
</Button>
</span>
)}
</div>
<div className="catalog-tile-view-pf-category-body">
{catalogTiles}
Expand Down
43 changes: 21 additions & 22 deletions packages/patternfly-3/patternfly-react/src/components/Filter/__mocks__/mockFilterExample.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,28 +277,27 @@ export class MockFilterExample extends React.Component {
{this.renderInput()}
</Filter>
</div>
{activeFilters &&
activeFilters.length > 0 && (
<Toolbar.Results>
<Filter.ActiveLabel>Active Filters:</Filter.ActiveLabel>
<Filter.List>
{activeFilters.map((item, index) => (
<Filter.Item key={index} onRemove={this.removeFilter} filterData={item}>
{item.label}
</Filter.Item>
))}
</Filter.List>
<a
href="#"
onClick={e => {
e.preventDefault();
this.clearFilters();
}}
>
Clear All Filters
</a>
</Toolbar.Results>
)}
{activeFilters && activeFilters.length > 0 && (
<Toolbar.Results>
<Filter.ActiveLabel>Active Filters:</Filter.ActiveLabel>
<Filter.List>
{activeFilters.map((item, index) => (
<Filter.Item key={index} onRemove={this.removeFilter} filterData={item}>
{item.label}
</Filter.Item>
))}
</Filter.List>
<a
href="#"
onClick={e => {
e.preventDefault();
this.clearFilters();
}}
>
Clear All Filters
</a>
</Toolbar.Results>
)}
</div>
);
}
Expand Down
13 changes: 6 additions & 7 deletions ...y-react/src/components/LoginPage/components/LoginCardComponents/LoginCardSocialColumns.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,13 +60,12 @@ class LoginCardSocialColumns extends React.Component {
return null;
}
const { expend, width } = this.state;
const expendButton = width > 768 &&
links.length > numberOfButtonsToShow && (
<Button bsStyle="link" bsClass="btn btn-link login-pf-social-toggle" onClick={this.toggleExpend}>
{expend ? 'Less' : 'More'} &nbsp;
<Icon name={`angle-${expend ? 'up' : 'down'}`} />
</Button>
);
const expendButton = width > 768 && links.length > numberOfButtonsToShow && (
<Button bsStyle="link" bsClass="btn btn-link login-pf-social-toggle" onClick={this.toggleExpend}>
{expend ? 'Less' : 'More'} &nbsp;
<Icon name={`angle-${expend ? 'up' : 'down'}`} />
</Button>
);

const doubleColumn = links.length > 4 ? 'login-pf-social-double-col' : '';
const moreItems = expend || width < 768 ? this.getHiddenListItems() : null;
Expand Down
8 changes: 4 additions & 4 deletions ...patternfly-3/patternfly-react/src/components/Table/__mocks__/mockClientPaginationTable.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -295,8 +295,8 @@ export class MockClientPaginationTable extends React.Component {

if (checked) {
const updatedSelections = [...new Set([...currentRows.map(r => r.id), ...selectedRows])];
const updatedRows = rows.map(
r => (updatedSelections.indexOf(r.id) > -1 ? MockClientPaginationTable.selectRow(r) : r)
const updatedRows = rows.map(r =>
updatedSelections.indexOf(r.id) > -1 ? MockClientPaginationTable.selectRow(r) : r
);
this.setState({
// important: you must update rows to force a re-render and trigger onRow hook
Expand All @@ -307,8 +307,8 @@ export class MockClientPaginationTable extends React.Component {
} else {
const ids = currentRows.map(r => r.id);
const updatedSelections = selectedRows.filter(r => !(ids.indexOf(r) > -1));
const updatedRows = rows.map(
r => (updatedSelections.indexOf(r.id) > -1 ? r : MockClientPaginationTable.deselectRow(r))
const updatedRows = rows.map(r =>
updatedSelections.indexOf(r.id) > -1 ? r : MockClientPaginationTable.deselectRow(r)
);
this.setState({
rows: updatedRows,
Expand Down
47 changes: 23 additions & 24 deletions ...ages/patternfly-3/patternfly-react/src/components/Toolbar/__mocks__/mockToolbarExample.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -318,30 +318,29 @@ export class MockToolbarExample extends React.Component {
<h5>40 Results</h5>
</Toolbar.Results>
))}
{activeFilters &&
activeFilters.length > 0 && (
<Toolbar.Results>
<h5>40 Results</h5>
<Filter.ActiveLabel>Active Filters:</Filter.ActiveLabel>
<Filter.List>
{activeFilters.map((item, index) => (
<Filter.Item key={index} onRemove={this.removeFilter} filterData={item}>
label=
{item.label}
</Filter.Item>
))}
</Filter.List>
<a
href="#"
onClick={e => {
e.preventDefault();
this.clearFilters();
}}
>
Clear All Filters
</a>
</Toolbar.Results>
)}
{activeFilters && activeFilters.length > 0 && (
<Toolbar.Results>
<h5>40 Results</h5>
<Filter.ActiveLabel>Active Filters:</Filter.ActiveLabel>
<Filter.List>
{activeFilters.map((item, index) => (
<Filter.Item key={index} onRemove={this.removeFilter} filterData={item}>
label=
{item.label}
</Filter.Item>
))}
</Filter.List>
<a
href="#"
onClick={e => {
e.preventDefault();
this.clearFilters();
}}
>
Clear All Filters
</a>
</Toolbar.Results>
)}
</Toolbar>
);
}
Expand Down
35 changes: 17 additions & 18 deletions packages/patternfly-3/patternfly-react/src/components/VerticalNav/VerticalNavItemHelper.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -280,25 +280,24 @@ class BaseVerticalNavItemHelper extends React.Component {
<span className="list-group-item-value">{title}</span>
{showBadges && childBadgeComponents && <div className="badge-container-pf">{childBadgeComponents}</div>}
</a>
{childItemComponents &&
childItemComponents.length > 0 && (
<div className={`nav-pf-${nextDepth}-nav`}>
<div className="nav-item-pf-header">
{(pinnableMenus || isMobile) && (
<a
className={classNames(`${nextDepth}-collapse-toggle-pf`, {
collapsed: onPinnedPath
})}
onClick={this.pinNextDepth}
/>
)}
<span>{title}</span>
</div>
<NavContextProvider {...this.props} idPath={this.idPath()} item={navItem}>
<ListGroup componentClass="ul">{childItemComponents}</ListGroup>
</NavContextProvider>
{childItemComponents && childItemComponents.length > 0 && (
<div className={`nav-pf-${nextDepth}-nav`}>
<div className="nav-item-pf-header">
{(pinnableMenus || isMobile) && (
<a
className={classNames(`${nextDepth}-collapse-toggle-pf`, {
collapsed: onPinnedPath
})}
onClick={this.pinNextDepth}
/>
)}
<span>{title}</span>
</div>
)}
<NavContextProvider {...this.props} idPath={this.idPath()} item={navItem}>
<ListGroup componentClass="ul">{childItemComponents}</ListGroup>
</NavContextProvider>
</div>
)}
</ListGroupItem>
);

Expand Down
26 changes: 12 additions & 14 deletions packages/patternfly-3/patternfly-react/src/components/Wizard/Stories/WizardExample.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,20 +60,18 @@ export class WizardExample extends MockWizardBase {
<Icon type="fa" name="angle-right" />
</Button>
)}
{activeStepIndex === 2 &&
activeSubStepIndex === 0 && (
<Button bsStyle="primary" onClick={this.onNextButtonClick}>
Deploy
<Icon type="fa" name="angle-right" />
</Button>
)}
{activeStepIndex === 2 &&
activeSubStepIndex === 1 && (
<Button bsStyle="primary" onClick={this.close}>
Close
<Icon type="fa" name="angle-right" />
</Button>
)}
{activeStepIndex === 2 && activeSubStepIndex === 0 && (
<Button bsStyle="primary" onClick={this.onNextButtonClick}>
Deploy
<Icon type="fa" name="angle-right" />
</Button>
)}
{activeStepIndex === 2 && activeSubStepIndex === 1 && (
<Button bsStyle="primary" onClick={this.close}>
Close
<Icon type="fa" name="angle-right" />
</Button>
)}
</Wizard.Footer>
</Wizard>
</div>
Expand Down
35 changes: 16 additions & 19 deletions packages/patternfly-3/react-console/src/AccessConsoles/AccessConsoles.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,14 +59,12 @@ class AccessConsoles extends React.Component {

getConsoleForType(type) {
// To keep connection, render all consoles but hide those unused
return React.Children.map(
this.props.children,
child =>
this.state.keptConnection[getChildTypeName(child)] ? (
<div key={getChildTypeName(child)} hidden={!isChildOfType(child, type)}>
{child}
</div>
) : null
return React.Children.map(this.props.children, child =>
this.state.keptConnection[getChildTypeName(child)] ? (
<div key={getChildTypeName(child)} hidden={!isChildOfType(child, type)}>
{child}
</div>
) : null
);
}

Expand Down Expand Up @@ -105,17 +103,16 @@ class AccessConsoles extends React.Component {
)}
</Dropdown.Menu>
</Dropdown>
{this.state.type !== NONE_TYPE &&
this.state.type !== DESKTOP_VIEWER_CONSOLE_TYPE && (
<Checkbox
className="console-selector-pf-disconnect-switch"
inline
defaultChecked={this.props.disconnectByChange}
onChange={e => this.onChangeDisconnectBySwitchClick(e.target)}
>
{this.props.textDisconnectByChange}
</Checkbox>
)}
{this.state.type !== NONE_TYPE && this.state.type !== DESKTOP_VIEWER_CONSOLE_TYPE && (
<Checkbox
className="console-selector-pf-disconnect-switch"
inline
defaultChecked={this.props.disconnectByChange}
onChange={e => this.onChangeDisconnectBySwitchClick(e.target)}
>
{this.props.textDisconnectByChange}
</Checkbox>
)}
</Col>
</FormGroup>
</Form>
Expand Down
2 changes: 1 addition & 1 deletion packages/patternfly-4/react-styles/src/build/util.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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, ''));
Copy link
Author

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?

Copy link
Member

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

Copy link
Member

Choose a reason for hiding this comment

The 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}`;
}

Expand Down
2 changes: 1 addition & 1 deletion packages/patternfly-4/react-tokens/build/generateTokens.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ test('keeps variable reference if computing fails', () => {
function getOutputs() {
return outputFileSyncMock.mock.calls.reduce((acc, call) => {
const [filePath, content] = call;
const splitPath = filePath.split('/');
const splitPath = filePath.split(/[/\\]/);
const name = splitPath.slice(-2).join('/');
return {
...acc,
Expand Down
24 changes: 22 additions & 2 deletions packages/react-codemods/transforms/pf3-pf4.button.test.js
View file
Open in desktop
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';
Copy link
Author

@TimoStaudinger TimoStaudinger Jan 28, 2019 •
edited
Loading

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. @tlabaj @jschuler Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

Expand Down
2 changes: 1 addition & 1 deletion packages/react-codemods/transforms/pf3-pf4.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ module.exports = (file, api, options) => {

return prettier
? prettier.format(transformedSource, {
parser: 'babylon',
Copy link
Contributor

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?

Copy link
Contributor

@redallen redallen Mar 11, 2019 •
edited
Loading

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

Copy link
Author

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.

parser: 'babel',
...prettier.resolveConfig.sync(process.cwd())
})
: transformedSource;
Expand Down
Loading