bug/JENKINS 44303 js lint or js test failures don't fail the build

[cliffmeyers]

Description

• I discovered this while bringing one of my branches up to date w/ master. In my local environment, running builds via Maven that include lint or js test failures don't fail the build. Perhaps a regression in js-builder.

Submitter checklist

• Link to JIRA ticket in description, if appropriate.
• Change is code complete and matches issue description
• Appropriate unit or acceptance tests or explanation to why this change has no tests
• Reviewer's manual test instructions provided in PR description. See Reviewer's first task below.
• Ran Acceptance Test Harness against PR changes.

Reviewer checklist

• Run the changes and verified the change matches the issue description
• Reviewed the code
• Verified that the appropriate tests have been written or valid explanation given

[cliffmeyers]

[INFO]   101 passing (362ms)
[INFO]   1 failing
[INFO] 
[INFO]   1) Pipelines pending state should continue to render existing data while a fetch is pending:
[INFO]      Error: all i do is fail.
[INFO]       at Context.<anonymous> (src/test/js/pipelines-spec.js:65:19)
[INFO] 
[INFO] 
[INFO] 
[INFO] [15:25:49] Mocha test failures. See console for details (or surefire JUnit report files in target folder).Error in plugin 'gulp-mocha'
[INFO] Message:
[INFO]     1 test failed.
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 01:19 min
[INFO] Finished at: 2017-05-11T15:25:50-04:00
[INFO] Final Memory: 54M/966M
[INFO] ------------------------------------------------------------------------

[kzantow]

eww...

[cliffmeyers]

I am doing a poor man's bisect now to find this.

[cliffmeyers]

Confirmed that the build succeeds in CI too. See linked build here: https://ci.blueocean.io/blue/organizations/jenkins/blueocean/detail/bug%2Fjs-lint-or-test-failures-dont-fail-build/1/pipeline/?start=0#step-18-log-0

Click "show complete log" and search for "all i do is fail"

@i386 @michaelneale

[kzantow]

😭

[cliffmeyers]

Stepping away for a bit but I'm going to try to straighten this out later on tonight. It looks like the upgraded version of gulp-runner is the cause of the issue.

[cliffmeyers]

This looks to be the underlying source of the issue:

jonjaques/gulp-runner@736dbf9#diff-168726dbe96b3ce427e7fedce31bb0bcL49

Which we were relying on here:
https://github.com/jenkinsci/js-builder/blob/master/cli.js#L42

[kzantow]

Wow, nice catch! Maybe we should be checking for a different event? Or I'm sure we can get that reverted in gulp-runner

[kzantow]

Maybe we can just change the js-builder code to:

gulp.on('close', function(code) {
      if (code !== 0) {
         die, die die

[cliffmeyers]

@kzantow I think that could be an acceptable fix on our end. We could also just pass a callback to the runner as that seems to be another way that gulp-runner would reliably report the error.

As far changes in gulp-runner, I'm not sure. I have opened two PR's a while back and there has not been much activity. The maintainer told me privately he's been tied up with some other things and hasn't had time. see: jonjaques/gulp-runner/pull/6 and jonjaques/gulp-runner/pull/7

(Ironically, the PR I filed was meant to address this kind of issue, but I opened it a few weeks back and didn't realize that my branch was quite out of date and using an older version of js-builder that had a much earlier version of gulp-runner that still had the event)

I still question whether we should even be using gulp-runner at all. We have a custom js-builder CLI that is firing off a separate process to run gulp and it's just making things more complex when they really shouldn't be. This approach also makes it very difficult to debug our gulp-based builds, which is important for be able to debug our JS tests. I think we should consider scrapping gulp-runner honestly. It's just another moving part that can break.

[cliffmeyers]

I'll make a quick fix in js-builder later on tonight and perhaps we can discuss larger changes as part of a larger issue.

[michaelneale]

good catch - I think @sophistifunk mentioned something like this a few weeks back but I wasn't able to see it. @cliffmeyers so what is the plan? nurse the things back to health on this PR ?

[cliffmeyers]

@michaelneale yep... small change to js-builder just to get failures working again, which will ultimately get pulled into this PR so master is fixed ASAP. This should fix the problem with the lint errors not failing the build either.

I may start another thread on the UX list where I discuss the pros and cons of js-builder CLI and Gulp Runner vs. invoking gulp directly. I've seen issues with this in #1002

[michaelneale]

@cliffmeyers ok - make sure you keep in touch with keith about his upcoming changes how things are run of course.

[sophistifunk]

That would explain all the lint errors I've been getting in other people's code recently.

[cliffmeyers]

Appears the issues with failing tests not failing the full build are fixed. Failing lint results however are not failing this build. Warnings and errors are written to console, yet the data object passed via gulp's pipe show zero errors and warnings. Bizarre...

[michaelneale]

@cliffmeyers this is still a WIP?

[cliffmeyers]

@michaelneale yes, I'm investigating this now still. It might help if you could take a few mins (hopefully < 15m) to repro it as well? It's a timing issue, so it might not be consistent across environments, although the issues seem to show up in CI so I am hoping they would for you locally as well.

Repro Steps

1. Check out 7adb682 - this is the commit before mobx karaoke landed.
2. Open Activity.jsx in blueocean-dashboard and add a snippet like var foo = null;.
3. Run mvn clean test -DcleanNode
4. Note that the build fails, as you expect.
5. Check out e8c0f65 - this is the commit where mobx karaoke landed.
6. Repeat steps 2 and 3.
7. Note that the build succeeds, but shouldn't.

[cliffmeyers]

(note that I obtained the above repro steps after making some other changes to how the gulp build runs while debugging this issue. I think that the inconsistent behavior will still occur though)

[michaelneale]

@cliffmeyers FWIW I was able to reproduce the above - switching to that later commit and it doesnt' fail!!!

[cliffmeyers]

lint issues should be resolved with jenkinsci/js-builder#18

[michaelneale]

@cliffmeyers ok - up to you when you close this one down. Just keep in mind if you fix a bunch of violations, that all open branches/forks will need to be udpated to master to ensure they pick up the same stuff otherwise they will think everything is fine, and we will have a mess of broken builds on master.

[cliffmeyers]

ATH actually passes now that some of js-builder's transitive deps were prevented from being upgraded by declining updates in shrinkwrap. I am not crazy about this approach but maybe it's a not-uncommon workflow? I will be adding a validation script in the next push that looks for the offending dependency and fails the build if it's found.

Will also be pushing some commits that introduce lint errors and JS test failures to ensure the build fails like it should.

[cliffmeyers]

Above commit failed due to edgeCases/folder. Pretty sure it's a spurious failure, so re-running.

[cliffmeyers]

Updating to master did the trick. Apparently the changes in #1063 were needed because the "try blue ocean" link was moved to the left? What I don't understand is how that change impacted my own branch. Was something changed in Jenkins core that is automatically pulled into the ATH image? @michaelneale if you have any input I'd love to understand this better.

[cliffmeyers]

I think this PR is finally ready to go.

[michaelneale]

🐝 LGTM (master has a few regression fixes, but don't seem related, so this should be safe to get in).

[michaelneale]

please do update this to master now BEFORE merging, as lots of stuff today, and it may pick up new violations... (before merging after it passes)