WordPress.org

Make WordPress Core

Opened 4 months ago

Closed 7 weeks ago

Last modified 6 weeks ago

#26446 closed enhancement (fixed)

Travis CI should run jshint

Reported by: jorbin Owned by: nacin
Milestone: 3.9 Priority: normal
Severity: normal Version:
Component: Build/Test Tools Keywords: has-patch commit
Focuses: javascript Cc:

Description

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.

Attachments (1)

travis.diff (878 bytes) - added by jorbin 4 months ago.

Download all attachments as: .zip

Change History (7)

jorbin4 months ago

comment:1 follow-up: bpetty4 months ago

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.

comment:2 in reply to: ↑ 1 jorbin4 months ago

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.

comment:3 jorbin2 months ago

  • Focuses javascript added
  • Keywords commit added
  • Milestone changed from Awaiting Review to 3.9

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.

comment:4 nacin2 months ago

In 27131:

Various JSHint fixes. see #26446.

comment:5 nacin7 weeks ago

  • Owner set to nacin
  • Resolution set to fixed
  • Status changed from new to closed

In 27267:

Add jshint to Travis CI config.

props jorbin.
fixes #26446.

comment:6 ircbot6 weeks ago

This ticket was mentioned in IRC in #wordpress-dev by rodrigoprimo. View the logs.

Note: See TracTickets for help on using tickets.