Skip to content

Teach CircleCI to run eslint for files modified in the topic branch #6625

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

Closed
ghost wants to merge 9 commits into from
Closed

Teach CircleCI to run eslint for files modified in the topic branch #6625

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
694e390
Make space-before-blocks an error
Jul 8, 2016
66ed5c6
Break the rules
Jul 8, 2016
170d212
Pass run-lint script the files in this branch
Jul 8, 2016
7d733b6
run-lint should be passed a list of files to run
Jul 8, 2016
f021dfa
Compare topic branch with origin/master
Jul 9, 2016
120cc4d
Revert "Make space-before-blocks an error"
Jul 9, 2016
488a5cc
Revert "Break the rules"
Jul 9, 2016
ed2f714
adapt _npm run lint_ command to new bin/run-lint API
Jul 9, 2016
1ff429e
Bring back bin/run-lint --ext parameters
Jul 9, 2016
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
10 changes: 5 additions & 5 deletions bin/run-lint
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
#!/usr/bin/env node
const child_process = require( 'child_process' );
const path = require( 'path' );
var args = [ '--cache', '--quiet', '--ext=.js', '--ext=.jsx' ];
if ( process.argv.length > 2 ) {
args = args.concat( process.argv.slice( 2 ) );
} else {
args = args.concat( [ '.' ] );
var args = [ '--cache', '--quiet', '--ext=.js', '--ext=.jsx' ];
if ( process.argv.length <= 2 ) {
process.exit( 0 );
}

args = args.concat( process.argv.slice( 2 ) );

const results = child_process.spawnSync( path.join( '.', 'node_modules', '.bin', 'eslint' ), args );

if ( results.stdout ) {
Expand Down
8 changes: 1 addition & 7 deletions circle.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,7 @@ test:
- NODE_ENV=test make client/config/index.js:
parallel: true
override:
- bin/run-lint :
parallel: true
files:
- client/**/*.js
- client/**/*.jsx
- server/**/*.js
- server/**/*.jsx
- bin/run-lint $(git diff --name-only $(git merge-base $(git rev-parse --abbrev-ref HEAD) origin/master)..HEAD *js *jsx)
Copy link
Member

@nb nb Jul 11, 2016

Choose a reason for hiding this comment

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

This way wouldn’t we run the checks only on the changed files? We still want to show problems in other files/lines, but just relegate errors to warnings.

Copy link
Author

@ghost ghost Jul 11, 2016

Choose a reason for hiding this comment

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

Exactly, that's the purpose of this PR.

We still want to show problems in other files/lines, but just relegate errors to warnings.

I am not sure how to integrate this idea at a CircleCI level.

Rationale for the approach I have chosen with this PR

The idea makes a lot of sense in our development machines, as it gives us flexibility: you can solve warnings on areas aside of you task or you can focus of sending good code that passes validation without being prevented to move forward due to problems in other areas. That's the developer's call (taking into account the time you have, urgency of the task, etc).

But, as far as I have understood how CircleCI is being used now in calypso, it gives us a binary answer: does the PR pass the quality checks or not? Is it red or green?. Facing that choice, I asked myself: should CircleCI fail or should pass for warnings in files the PR has not modified? I have came to the conclusion that it should not fail for the warnings in those files, as the reason we have this task in the first place is being able to move forward and produce new quality code meanwhile we migrate the old code to current practices. The current run-lint script adopts the same position by reporting only errors to CircleCI.

Conclusion

So: if we are going to report only as errors the rules we break in the files modified in the PR and only those errors would make CircleCI to fail, I think passing run-lint only the files modified in the PR is a sensible thing to do. From a CircleCI point of view, we do not need to compute the files we are not going to consider as errors. For free, we also have a potential gain in performance in doing so.

Am I missing something?

Copy link
Member

@nb nb Jul 12, 2016

Choose a reason for hiding this comment

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

What if there are errors in the other files? What if we changed the eslint configuration or we upgraded eslint or in some other way modified how the linter works?

Copy link
Member

@nb nb Jul 12, 2016

Choose a reason for hiding this comment

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

I am not sure how to integrate this idea at a CircleCI level.

I am sure you can find a way :)

Copy link
Author

@ghost ghost Jul 13, 2016

Choose a reason for hiding this comment

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

What if there are errors in the other files?

You mean JS errors, not rule-breaking, right? This approach moves us from linting the whole codebase each time to lint the specific diff's we want to merge. Granted that all changes go through this cycle of building first a PR, we should not have undetected errors.

One concern I have after reading @blowery question above about master merges, though, is that we might need also to execute run-lint script on that cases. That is something we can do to cover all "entry points" to the codebase - if CircleCI also run on master commits. I am on mobile and I cannot check that. Would be great to learn more about how calypso uses CircleCI: does it send reports on error to some places (slack, email, etc)?

what if we changed the eslint configuration or we upgraded ESLint or in some other way modified how the linter works?

As long as ESLint keeps accepting a list of files to check we are safe. Right now, I cannot see a way how this PR would be affected by the things you suggest - or any other changes of that kind, to that matter. I would love to be illustrated if I am missing something obvious. The point is, nevertheless, very relevant for the next step -linting only specific lines- as we are going to depend more on ESLint specifics. Thanks for the food for thought.

Copy link
Author

@ghost ghost Jul 13, 2016

Choose a reason for hiding this comment

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

I am sure you can find a way

Would give it a second thought.

One approach I have already thought was: why not show the warnings too in the CircleCI console but only make CircleCI to fail (by the exit code of the script) if there are any errors in the files modified.

Reason I didn't pursue that way: current run-lint script does not show the warnings, so I took that as an indicator that people look for the green/red state given by CircleCI and only goes to its console if there are some problems.

- MOCHA_FILE=./test-results-client.xml npm run test-client -- -R mocha-junit-reporter -t $CIRCLE_NODE_TOTAL -i $CIRCLE_NODE_INDEX:
Copy link
Member

@nb nb Jul 11, 2016

Choose a reason for hiding this comment

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

Are we going to lose the checks running in parallel?

Copy link
Author

@ghost ghost Jul 11, 2016

Choose a reason for hiding this comment

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

The files modifier accepts only globs, so we cannot use them to write git expressions. We could find a way to set parallelism up again by using env variables in run-lint script, though.

The reason I was OK switching off parallelism and not pursuing the second approach was: if we are going to run eslint only for the files modified in the branch, I guess they are a handful, not a lot. Parallelism, then, is not going to accelerate things that much and, in return, we have a clearer and more understandable solution (without that logic in the script). My assumptions about the size and number of files by PR may be totally wrong as I am still not very familiar with the common practices around here - and so the gains of parallelization in this specific case could be bigger than I thought. What do you think?

Copy link
Contributor

@blowery blowery Jul 12, 2016

Choose a reason for hiding this comment

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

I think that's fine, though how does this work for the master merges?

Copy link
Author

@ghost ghost Jul 13, 2016 •
edited by ghost
Loading

Choose a reason for hiding this comment

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

The run-lint script is not going to run eslint.

As the HEAD in the master branch is pointing to the same place than origin/master, the result of the git expression would be void. Ergo, the run-lint script would not run.

But your question makes me wonder: I am assuming calypso is using CircleCI to do quality-checks on branches/PR before they are integrated into master. Is it useful outside that? I mean, do you expect CircleCI to also run on master?

cc @blowery

Copy link
Member

@nb nb Jul 13, 2016

Choose a reason for hiding this comment

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

Yes, it’s important to run on master, too – you never know what will happen after the merge. Also, need to make sure we run in on direct commits to master (I think we have that disabled, but still, better safe than sorry).

parallel: true
- MOCHA_FILE=./test-results-server.xml npm run test-server -- -R mocha-junit-reporter -t $CIRCLE_NODE_TOTAL -i $CIRCLE_NODE_INDEX:
Expand Down
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 @@ -142,7 +142,7 @@
"test-client:watch": "nodemon -e js,jsx --exec npm run test-client",
"test-server:watch": "nodemon -e js,jsx --exec npm run test-server",
"test-test:watch": "nodemon -e js,jsx --exec npm run test-test",
"lint": "bin/run-lint",
"lint": "bin/run-lint .",
Copy link
Member

@nb nb Jul 11, 2016

Choose a reason for hiding this comment

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

Why did you change the way we call run-lint? I am not saying it’s wrong, just wondering why.

Copy link
Author

@ghost ghost Jul 11, 2016 •
edited by ghost
Loading

Choose a reason for hiding this comment

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

To make npm run lint run as before despite the changes I have made to run-lint script. See image of changes to run-lint script.

As the purpose of this PR is to run eslint only for files modified within the branch (see rationale in the comment above), what happens when the branch does not modify any JS/JSX file and the result of the git expression is void? Current run-lint is passed no args and it takes that as an indication that it should run for the whole codebase. But, what we actually want in this specific case (at least in CircleCI environment) is to prevent eslint from running. So, as I have changed the internal behaviour of the script for this reason, I needed to modify also the external calls to it.

Copy link
Member

@nb nb Jul 12, 2016

Choose a reason for hiding this comment

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

Thanks, given your general approach this change makes sense.

"css-lint": "stylelint 'client/**/*.scss' --syntax scss"
},
"devDependencies": {
Expand Down