Testing JS: eslint and codechecker js support.

open-lms-open-source / moodle-plugin-ci

Assist with running a Moodle plugin in Travis CI

https://blackboard-open-source.github.io/moodle-plugin-ci/

GNU General Public License v3.0

42 stars 37 forks source link

Testing JS: eslint and codechecker js support. #89

Closed kabalin closed 4 years ago

kabalin commented 5 years ago

This adds two features (both done by @marinaglancy):

Add moodle-plugin-ci eslint check which lints JS files on the plugin using ESLint. This is different to grunt eslint task, as the latter one suppress warnings and there is no control how many warnings are allowed). To fail on warnings use --max-warnings 0.
moodle-plugin-ci codechecker now supports --files option to specify type of files Codechecker should be run on. Possible values php (default), js and all. To fail on warnings use --max-warnings 0.

In addition to that, this PR contains change to ignore node_modules directory in tests. Some plugins may populate node_modules directory (following npm install), which in turn can affect tests related to JS validation. So, this directory can be safely be excluded from Finder output.

Test coverage is added. Fixes to local_travis fixture will follow (not commited intentionally to demonstrate eslint test output).

kabalin commented 5 years ago

Example of eslint output: https://travis-ci.org/blackboard-open-source/moodle-plugin-ci/jobs/507809859#L724

polothy commented 5 years ago

I remember specifically removing processing JS files from codechecker because that was the direction of core. Core was going to use Grunt and friends to handle all the JS stuff. Do you have a link or something that shows that core changed their minds on this?

stronk7 commented 5 years ago

Hi,

sweet stuff @kabalin and @marinaglancy !

I think that @polothy is correct here and, after a number of attempts... we decide to keep the codechecker only working against php files and let other tools (grunt in general, but eslint, stylelint and others in particulat) to do their job.

Main reason was that allowing codechecker to process both PHP and JS types was leading to a lot of false positives... because a good number of Sniffs in the standard... that only apply to PHP in our case, are configured to also process JS files by default. So it was agreed to always restrict uses to just PHP to avoid that noisy JS processing).

So, if that serves as reference for anything... for core... right now, our belowed CiBoT is doing):

Execute grunt... against whole codebase, just to see if there is anything (minified, complied..) missing/out-of-sync (there will be local changes if so, that we detect).
Remove all the files not belonging to the patch (just to save processing time!)
Execute eslint individualy (to report both warns and errors).
Execute others (stylelint, csslint for old branches...)...
Execute codechecker (only for --extensions=php).

So, as we have been chatting... I think that introducing a new job for specifically running eslint (or stylelint, or anything else) checks is great. Also the --max-warnings idea (so severity can be raised)... but i really would keep phpcs 100% out from any JS/CSS check... and would use the specific tool instead.

Ciao :-)

polothy commented 5 years ago

Hey Eloy, thanks for providing some background and clarity.

So, what would we gain from calling eslint directly vs Grunt? I was hoping Grunt would take care of all the assets stuff for us in regards to CI.

marinaglancy commented 5 years ago

afaik grunt does not show warnings where eslint does

polothy commented 5 years ago

What about this? https://github.com/moodle/moodle/blob/ec819146cce71d47e6d0b5403e9889d64de1db7b/Gruntfile.js#L114

In the end... a little hesitant about adding anything for JS/CSS beyond what is in Grunt just to insulate this tool from change.

stronk7 commented 5 years ago

Yesyes, you can run grunt --show-lint-warnings to get them, np with that.

Sorry if i was not clear enough above... with CiBoT we decided to go via the separated execution (eslint, stylelint...) because we needed them into some processable format (checkstyle, xunit...) to be able to aggregate all the information together, filter out lines not being part of a patch and convert everything to a human-readable format (those smurf.xml and smurf.html files) that are reported in the tracker.

Obviously that's not needed for moodle-plugin-ci, so just passing options to the underlying grunt tasks could be enough, agree.

Ciao :-)

polothy commented 5 years ago

@kabalin So, based on these comments, I think the only things I'd like to accept are:

The --max-warnings for codechecker command. That's a pretty cool idea.
If you want to try to figure out how to do --show-lint-warnings for the Grunt task. Depending on what the flag does, we may just want to always show the warning or perhaps when -v is passed, it shows them.

Otherwise, regarding eslint, I think it is pretty easy to add that to your Travis CI file yourself if that's what you really want. I'm open to counter arguments, but when I worked on MPCI 2.0, I migrated a lot of JS stuff over to Grunt in hopes that Grunt would provide an abstraction to the JS tooling that core uses in each version of Moodle.

kabalin commented 5 years ago

Thanks @polothy for the feedback, makes sense. Pinging @marinaglancy as she developed those features.

marinaglancy commented 4 years ago

I'm working on a new pull request that will use "grunt eslint --show-lint-warnings" and optionally also "grunt eslint --max-lint-warnings=0" added in https://tracker.moodle.org/browse/MDL-68323

marinaglancy commented 4 years ago

new PR https://github.com/blackboard-open-source/moodle-plugin-ci/pull/111 is ready that utilises grunt eslint for eslint warnings. Thanks for useful suggestions! @kabalin please close this PR. TIA

kabalin commented 4 years ago

Thanks @marinaglancy!