[policy, no patch] Fail testbot results on ESLint errors beginning March 2

Since we have d8 regression and patch jobs separate from the other jenkins jobs, this would be a configuration option that could be set easily.

http://cgit.drupalcode.org/drupalci_testbot/tree/src/DrupalCI/Plugin/Bui...

Im in agreement with this change, can do it easily, and am basically looking for core committer signoff. This would be for Core only, contrib would *not* fail tests for eslinting standards non-compliance.

I'm +1 - but I'm not a release manager. I think it makes sense to block patches on this. It means that by the time things get to rtbc committers have one less thing to think about and developers become more aware of our coding standards. This benefit also extends to the needs review stage when people can concentrate on reviewing what a patch does rather than coding standard nits.

It's a yes from me.

Yeah let's! This is a really good idea and as a bonus will let us test the water on how coding standards checking integration in the issue queue will go and what the bumps might be.

My only concern is possible unknown side-effects of the deployment. Core has these upcoming important windows:

Now through Mar. 2: Prep for 8.3.0-RC1
Mar. 15: Security release window
Mar. 21-26: Major sprint at Drupal Developer Days
Week of Apr. 3: 8.3.0
April 24-28: DrupalCon Baltimore

So if we can avoid disrupting CI around those windows, then I think this is a great idea.

(Making a small revision to the summary as coding standards are not a gate.)

Didn't realize this was actually in the core queue already.

+1 It seems right after 8.3.0 drops could be the time to implement.

The only thing that needs to happen to implement this change is to set a variable to true in the jenkins jobs - the implementation is already there, and we're already reporting on eslint errors, so, in this case the risk is relatively insignificant.

The only way I can see this potentially being impactful is if a patch had errors and was still committed, thus failing branch tests.

Given that possibility it would be sensible to mark a tests as failed for eslint errors only during d8 core patch testing, and allow the regression tests (nightlies/on commit) to pass in spite of having a coding standard issue. Coding standards are something we want to keep on top of, but don't necessarily need it to be an "all hands on deck who broke the build" kind of issue.

As an aside, we've launched the first part of coding standards: making them visible on the build results pages: https://www.drupal.org/pift-ci-job/607613

In that case it shouldn't be committed at all, because an issue will be filled to fix the error as soon as it's seen.

A 100% fail rate in HEAD will get reverted pretty quickly, so I'm not too concerned about that. We have something disruptive scheduled for Mar. 3 already (maybe the array patch) that will toast a lot of the core issue queue, so it might be better to turn it on a different day for easier debugging. Mar. 2 would be fine I think.

@Mixologic, do you need anything else from core maintainers here? Else I think we can mark this fixed as a policy issue. (Core doesn't need to track the implementation/deployment.)

I've got all the information/consensus I was looking for and will deploy this on the 2nd. I'll update this and mark it fixed once it's deployed, just in case there's any timing questions.

This has been deployed.

One caveat: since eslinting happens *before* simpletest, I've configured it such that it will fail fast and immediately stop the build if there is an eslint error. If this turns out to be onerous, we can always change it back, but ideally we aren't burning 2 hours of testbot time so that we can run a test that has eslinting errors, and then run it again without.

@Mixologic I don't think the fail fast logic is right here - since if you also have test fail you'll want to know. And fix both things.

Im trying to figure out the best way to continue to add testing without adding excessive costs. Each new assessment adds new failure mechanisms that can potentially increase the amount of tests that we need to run. So in the case of a patch where the only thing wrong is eslinting, we'd end up running two full testruns - the first time with the failed coding standard, the second time with the passing coding standard.

The fail fast method would return almost immediately (https://www.drupal.org/node/2542050#comment-11963053 took 44 seconds to build), which would be annoying at first, but the hope is that developers would adjust their local toolchain to avoid those failures in the first place.

If we get to the point where we fail on csslint and/or phpcs, we would definitely only want to fail the build after running all coding standards checks (ie. we definitely do not want the devs to have to run a coding standards patch hurdle race)

In any case, I'd like to try it with the fail fast first to see how it goes, as that is definitely the most cost effective way for us to continue adding functionality. If it proves to be onerous / disruptive we can adjust it to run the whole tests.