Page MenuHomePhabricator

Run linters before starting longer running jobs
Open, Needs TriagePublic

Description

From the parent T225730: Reduce runtime of MW shared gate Jenkins jobs to 5 min

  • have a pre-flight job that runs the linters for the repository that triggers the patch and hold any other jobs until that one has completed

I'd propose we start with composer test, then think about npm linters afterwards.

Another optimization we could make is to add a flag for quibble to not clone the entire repository when doing lint checks. If we split out the linting step into a standalone service on toolforge or cloud services, quibble ask the service if the patch passes the lint phase, and the service would have a composer cache that can be used to install the relevant codesniffer library version for the patch.

There is some prior art for this in https://gitlab.wikimedia.org/kharlan/fix-suggester-bot/-/blob/main/src/MessageHandler/GerritStreamHandler.php; that code generates fix suggestions but it could be expanded to be more generic if we are interested to have an API for this.

So the model would be something like:

  1. quibble job for linting starts
  2. quibble job makes an HTTP request to lint-service
  3. the lint service should be able to respond within a couple seconds, because it just needs to download the touched files and run composer test -- {files} using the correct version of the codesniffer library, which is already on the server thanks to composer cache
  4. build fails or passes based on above

steps 2-3 could be done entirely in Quibble, but having as a service seems useful for other tools to build on (like Fix-Suggester-Bot

Event Timeline

Change 747617 had a related patch set uploaded (by Kosta Harlan; author: Kosta Harlan):

[integration/quibble@master] Add phpcs stage

https://gerrit.wikimedia.org/r/747617

steps 2-3 could be done entirely in Quibble, but having as a service seems useful for other tools to build on (like Fix-Suggester-Bot

Easy enough to do in Quibble, so I've done that in the patch here.

Should this task be stalled, pending the discussion in https://gerrit.wikimedia.org/r/c/integration/quibble/ /747617/comment/cbfc0d7b_83cb505e/ ? The question raised is that phpcs sniffs should not short-circuit and suppress other tests, because developers rely on this comprehensive feedback. Perhaps we can rewrite this task to "run strong linters ahead of any other test", and phpcs will be in limbo, e.g. run as a nonblocking background job.

Should this task be stalled, pending the discussion in https://gerrit.wikimedia.org/r/c/integration/quibble/ /747617/comment/cbfc0d7b_83cb505e/ ? The question raised is that phpcs sniffs should not short-circuit and suppress other tests, because developers rely on this comprehensive feedback. Perhaps we can rewrite this task to "run strong linters ahead of any other test", and phpcs will be in limbo, e.g. run as a nonblocking background job.

I guess it's a question of whether or not we want to free up more executors for CI. If that isn't a concern, then what we could do is incorporate inline commenting on the patch (perhaps with a -1 vote) from either Quibble or a bot when we see that phpcs fails. I have most of that already wired up in https://wikitech.wikimedia.org/wiki/Tool:Fix_Suggester_Bot

Change 747617 abandoned by Kosta Harlan:

[integration/quibble@master] Add phpcs-and-lint stage

Reason:

https://gerrit.wikimedia.org/r/747617