Travis CI should run jshint

[jorbin]

Now that jshint is running properly, let's add it to our travis config so that we can find out if we ever screw it up.

[bpetty]

Are you sure you want Travis to send notifications every time a commit is made that violates jshint rules rather than just notifications when the code actually fails to function properly?

Not everyone realizes this at first, but there is a line you want to draw here. Add too much into Travis, and everyone gets used to (and starts to ignore) build failure notifications. I'd also hate to see committers getting into the habit of committing first, and relying on Travis to run the tests for them after the fact rather than running the checks before they commit.

Travis mostly helps ease the pain of testing across multiple environments while pin-pointing exactly which commit broke it at the same time. I don't think we really need that to check for jshint violations though. You already know the location, and the test doesn't need to be run across multiple build environments. It's simple enough to run on your local dev machine because you only need to check in one environment, and it currently only takes 16 seconds to run.

[jorbin]

Replying to bpetty:

Are you sure you want Travis to send notifications every time a commit is made that violates jshint rules rather than just notifications when the code actually fails to function properly?

One of the things jshint gives us is an indication that our code might not function properly. All The ES3 warnings tell us of potential problems in some of the browsers we support.

Not everyone realizes this at first, but there is a line you want to draw here. Add too much into Travis, and everyone gets used to (and starts to ignore) build failure notifications. I'd also hate to see committers getting into the habit of committing first, and relying on Travis to run the tests for them after the fact rather than running the checks before they commit.

I agree there is a line to draw, but I think that drawing it at only unit tests is drawing too short. Unit testing failures should be a show stopper, that causes the developers that worked on the breaking change to go back and fix it. JSHint is the same way. When it doesn't pass, we should figure out why and fix it.

Travis mostly helps ease the pain of testing across multiple environments while pin-pointing exactly which commit broke it at the same time. I don't think we really need that to check for jshint violations though. You already know the location, and the test doesn't need to be run across multiple build environments. It's simple enough to run on your local dev machine because you only need to check in one environment, and it currently only takes 16 seconds to run.

I would love to figure out a way to not have to run jshint for each of the configurations since you are correct that it doesn't need to be run across multiple build environments. I'm not sure if Travis supports that, but I imagine we can code around that if you really think it is a show stopper. Of course, as you point out we are only adding ~16 seconds to the travis build.

[jorbin]

We recently had multiple commits to fix jshnit errors [ #27033 #27032 ]. I think that this proves that it is beneficial to be notified sooner and thus it makes sense to run these with our unit tests.

Let's make a decision this cycle rather then putting this off.

[nacin]

In 27131:

Various JSHint fixes. see #26446.

[nacin]

In 27267:

Add jshint to Travis CI config.

props jorbin.
fixes #26446.