Add precommit hook.

[STRML]

This would have prevented #194. Alternatively, the CI should fail the build on lint errors.

[bajtos]

the CI should fail the build on lint errors.

@rmg how can we tell our CI to fail the build when a pretest script fails?

[rmg]

@bajtos my concern is that will result in a lot of failed builds, so I would need to make the behaviour configurable per-project, which would take some tinkering.

[bajtos]

Is there any way how to change the script values in package.json so that Jenkins does not skip the linter?

For example, will the following server as a workaround?

  "scripts": {
    "test": "grunt jscs jshint mocha"
  }

(Assuming that we define a Grunt task called mocha).

Alternatively:

  "scripts": {
    "test": "grunt jscs jshint && mocha"
  },

On the other hand, if linter errors are not considered as build failures, then linter should not be run as pretest script and it should be provided e.g. as lint script instead. In other words, the current setup where CI build passes while npm test fails, is IMO broken. Packages relying on this behaviour should be fixed so that npm test makes the same checks as the CI build.

[rmg]

@bajtos the linter isn't "skipped", per se, it's just that the tests and linting commands needs to be modified to produce report files that Jenkins can understand. So the main problem with running the linter in the pretest is that the output isn't captured in any useful way.

All of our projects are linted (the linter is chosen based on the presence of .jscsrc, .eslintrc, or .jshintrc, checked in that order) and the results collected and tracked by Jenkins. This phase could easily be marked as required instead of optional, but it might result in a sharp increase in build failures across the board until they are fixed. A mechanism for changing that behaviour per-project would be nice, but I don't have time right now :-(.

[bajtos]

@rmg I think we are mixing two things here.

Here is what I need: CI build must fail whenever npm test run locally fails (IIUC we don't have this now). The console output of npm test needs to be logged somewhere for future inspection (already implemented). This is the same behaviour that Travis-CI provides and IMO it is not related to linters only. I don't mind reading linting errors from Jenkins console log instead of using Jenkins UI integration.

Here is what I don't need, but I understand others may have a different opinion: integration of linter with Jenkins UI, so that linter results (and failures) are displayed in the UI. The current situation, when npm test runs linter once, and then Jenkins job runs the linter again, is an acceptable workaround for me.

The only thing I need from our Jenkins job is to honour the project settings configured in package.json. When package.json says that npm test should fail on a linter error reported by a pretest script, then Jenkins should do exactly that. If linter errors should not fail the build, then the linter should be run from a different npm script, e.g. npm run lint. The project maintainer is in control and can decide.

Let's not waste our time on discussing changes we don't have time to implement. I have submitted #202 to enable Travis-CI integration, that will give us what we need for free.

[bajtos]

@STRML My concern about a pre-commit hook is that the tests take several seconds to run and thus the hook will cause a significant slowdown. I would much rather fix the CI to report linting errors.

[bajtos]

BTW it would be even more cool to have https://linthub.io/ and/or https://github.com/markstory/lint-review enabled, unfortunately they don't seem to support JSCS yet.

[ritch]

AFAICT we can close this PR

@STRML please ping me if we should re-open.

[rmg]

@bajtos that's a good point about having a pretest being an opt-in for the behaviour you want. I took a look at the build-framework code and it only took a few minutes to make it so that the pretest is required to pass if defined, and it is now in production.

[bajtos]

@rmg that's a good point about having a pretest being an opt-in for the behaviour you want. I took a look at the build-framework code and it only took a few minutes to make it so that the pretest is required to pass if defined, and it is now in production.

Awesome, thank you!