jenkinsci / JenkinsPipelineUnit

Framework for unit testing Jenkins pipelines
MIT License
1.54k stars 394 forks source link

Add CodeNarc linting checks #237

Open unitysipu opened 4 years ago

unitysipu commented 4 years ago

There's a variety of potential bugs, code smells and style issues in the library and accompanying code examples that are trivial to spot and fix using a linter during development. Using the examples as they are in code that enforces linting require them to be fixed / refactored.

It helps tremendously to set a clean baseline for any future contributions and make the linting tool a prerequisite for merging, either by using a commit hook or making it part of tests to ensure good code hygiene / eliminating obvious mistakes or discrepancies in the library.

groovy-lint can be installed via npm i -g npm-groovy-lint

Documentation

Some of the rules may not apply and can be ignored by crafting a .groovylintrc.json and committing it into the repository root. It can be easily autogenerated using the vscode-groovy-lint vscode plugin.

npm-groovy-lint and an associated vscode plugin vscode-groovy-lint also have features to auto-fix trivial issues.

nre-ableton commented 4 years ago

I'm a fan of CodeNarc myself and use it heavily for other Groovy projects at my work. I'm unfamiliar with this particular wrapper, but as JenkinsPipelineUnit already has Travis CI in place, feel free to submit a PR to add linting support for it. Short of that, I think that the unit tests provide enough of a safety net for us to make releases of this library.

unitysipu commented 4 years ago

Fixing all the nags / tuning the rules for this repository is way too much commitment from me to do, there's so many when run with recommended rules. It's probably best for somebody who's familiar with the whole library to check them out and do all the appropriate fixes / ignores. While you may have good unit tests it's still valuable to have a clean linting result as it's easier to find genuine bugs from all the noise. I'm just reporting this as it's something I see as a user and a developer leveraging this library.

nre-ableton commented 4 years ago

I agree that fixing all of the violations is a lot to ask, but as you said:

Some of the rules may not apply and can be ignored by crafting a .groovylintrc.json and committing it into the repository root.

The initial PR could just add support for checking with CodeNarc and suppress all warnings. Over time, people could fix the violations and remove the suppressions. Likewise, I understand if you don't have the time to contribute such a PR, in which case we appreciate the feature request nonetheless.

nre-ableton commented 4 years ago

Also, I couldn't get CodeNarc 1.6 to run on the current master branch of JenkinsPipelineUnit, I get an exception:

ERROR org.codenarc.rule.AbstractRule - Error from [org.codenarc.rule.formatting.IndentationRule] processing source file [./src/main/groovy/com/lesfurets/jenkins/unit/RegressionTest.groovy]
java.lang.NullPointerException
    at java.util.regex.Matcher.getTextLength(Matcher.java:1283)
    at java.util.regex.Matcher.reset(Matcher.java:309)
    at java.util.regex.Matcher.<init>(Matcher.java:229)
    at java.util.regex.Pattern.matcher(Pattern.java:1093)
        ... snip ...

It seems that CodeNarc doesn't like the trait keyword. I'll file a separate issue there and see if this can be sorted out: https://github.com/CodeNarc/CodeNarc/issues/526.

nre-ableton commented 4 years ago

BTW I have changed the title of this issue, since the previous title makes it seem like some existing check is failing.