[Meta] Run tests with phpcs fail

I agree 100%. Don't block innovation over a missing space.

+1

In practice, almost no patch gets committed without a round of review anyway. So there’s very little risk of coding standards violations to get committed. (It’d fail CI anyway, just not as early.)

I suspect part of the reason is CI cost: save expensive CPU time for patches that could be green. But does this really save time & money? 🤔 Because … just to get to the point of running phpcs takes ~5 minutes (a lot of Docker activity before then!). phpcs then takes seconds, sometimes a minute. If we fail at that time, like we do today, the overhead is the overwhelming majority.

Worse: this means the only thing a contributor can fix is … coding standards. Because that’s the only information you get. So the current setup often results in netmore CPU time than if phpcs didn’t have halt-on-fail: true!

I'm wondering what does DrupalCI output now when there are coding standard violations? Also how does it report if there are coding standard violations and test failures?

Excellent question, let's try.

Moving to NW assuming #7 was suppose to fail.

Sorry but no, this still needs review by core committers and I suspect the DA as well then we can see what we need to do to DrupalCI to make this fail. Eventually it'll be CNW but that'll be in a different issue queue.

From past conversations with the DA I believe there is no capacity to make changes to DrupalCI to support this or any other feature requests, and instead we should look to implementing it when we switch to GitLab CI.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

And in the meantime we have the needs review queue bot to pick up some of this.

Excluding this issue for the bot, since what we need is discussion, not more bots getting in the way :)

Wait a second, wasn't that very useful? This means we could commit the patch as is if we wanted and while the checks won't show the phpstan fail , the bot eventually will. It's not like RTBC patches get in within hours.

Sometimes patches do get in quickly when they are critical, or core committers are working together. I'd really prefer not to have false-positive green test runs on something that has actually failed checks.

Is the goal to be able to run tests with phpcs errors?

If that's the case wouldn't the issue still need to have those fixed before it can be marked RTBC. Resulting in needing to run the tests again couldn't that be costly?

The problem with the needs review queue bot is that it's not a reliable service. It runs on a laptop behind the TV of a relative's house and there are a few limitations on what it can do today with regard to selecting patches to tests, running against specific branches, how frequently it runs because we need to use d.o api responsibly, etc. It's all solvable with some time but it's not time I have at the moment.

I do see the value of having the code style gate not being a blocker to running the tests. Overall most of the linting stuff is easy to fix, spacing, permissions, etc. so that would make sense to assume that it'll be easy to fix at any point and now "slow down" patches for that. There are a few eslint rules that are not easy to get rid off if eslint complaints but we could say it's non-blocking only for phpcs stuff and a specific subset of eslint rules.

If we're doing a wishlist it could make sense to not halt test runs on lint failures for NR issues but fail test runs for RTBC issues (since by that point the patches should be in a ready to commit state)

@longwave if core committers are involved and the patch is critical, everything that is non blocking would still be run in the pre-commit hook no?

For me I’m kind of 50/50.

If you’re rock n rolling with an issue and want to test how it’s going you don’t want to be delayed by phpcs issues.

But if we run tests for phpcs issues we still would have to run with them fixed. So double the resources right? Could that be noticeably more costly?

It won't be double the resources, no, such minutiae can be fixed ongoing. A complex enough patch will need many rounds anyways.

What we have here is: submit patch, schedule a check back in an hour so to find the patch was not tested. Often over the lack of absolutely unnecessary doxygen which makes it doubly infuriating. There's already an issue over dropping some of that but it's a long way off before we don't need to add any fluff.

So should this be moved over to the gitlab queue if we aren't going to add here?

Is there a way of just running the checks after the tests? Then we could keep halt-on-fail, but the tests would fail first, and the checks would only even run if the tests pass?

Moved the check to the bottom. Guessing that could matter?

I messed up indentation. Lets try again.

Still, should it be?

  halt-on-fail: false

No then it would show green even though there were phpcs errors I believe.

But #22 shows it ran the tests but still failed. Not sure why it's aborted but that's promising I think?

This doesn't appear to have worked due to unintended side effects, see https://dispatcher.drupalci.org/job/drupal_patches/187500/consoleFull

The tests create a bunch of output that is useful for debugging. But running the checks after the tests means that this output is considered as changes to the repo, and so the files are spellchecked/linted/etc. The clues are:

01:02:11 core/scripts/dev/commit-code-check.sh --drupalci
01:26:14 core/scripts/dev/commit-code-check.sh: line 212: /usr/bin/yarn: Argument list too long
01:26:14 
01:26:14 CSpell: failed

Then a huge number of PHPStan failures, and then it starts checking Nightwatch's CSS output:

01:27:18 nightwatch_output/nightwatch-html-report/css/bootstrap.min.css
01:27:18  6:9       ✖  Expected no more than 1 declaration                                            declaration-block-single-line-max-declarations
01:27:18  6:1006    ✖  Expected "Roboto" to be "roboto"                                               value-keyword-case
01:27:18  6:1060    ✖  Expected "Arial" to be "arial"                                                 value-keyword-case
01:27:18  6:1171    ✖  Expected "SFMono-Regular" to be "sfmono-regular"                               value-keyword-case
01:27:18  6:1186    ✖  Expected "Menlo" to be "menlo"                                                 value-keyword-case
01:27:18  6:1192    ✖  Expected "Monaco" to be "monaco"                                               value-keyword-case

Finally this all seems to take too long (it is analysing Bootstrap JS), and times out:

01:27:19 Checking core/nightwatch_output/nightwatch-html-report/js/bootstrap.min.js
01:27:19 
02:11:11 Build timed out (after 110 minutes). Marking the build as aborted.
02:11:11 Build was aborted

Hadn't seen this issue but have an MR up to run unit tests (but not everything else) if phpcs fails on gitlab. It would be a YAML change to allow unit + kernel but not everything else too.

Thanks catch, I intended to relate to that issue but mucked it up.

[# https://www.drupal.org/project/drupal/issues/3386076] → now runs phpunit and kernel tests if cs/phpstan etc. fails, but blocks functional etc. I think that's a good compromise between fast feedback and wasted test runs on typos for otherwise ready patches, so going to close this as duplicate.