Make WordPress Core

Opened 7 weeks ago

Last modified 5 weeks ago

#65950 assigned defect (bug)

Make sure all scripts and tasks provided to developers are always functional

Reported by: afercia Owned by: afercia
Priority: normal Milestone: Awaiting Review
Component: Build/Test Tools Version:
Severity: normal Keywords: has-patch
Cc: Focuses:

Description

Related: #65923 and #65939

Not all the NPM scripts and Grunt tasks provided by Core are included in the automated testing pipeline. Some of them are provided as sort of 'developers utility' as in: tools that developers can use for plugin development or to perform tasks on some parts of Core during development. For example: grunt jshint:plugins and npm run lint:jsdoc.

Given that they are not included in any automated testing, they can break at any time with little chances Core contributors would notice that promptly.

Turns out that:

  • lint:jsdoc is broken since 5 months, when ESLint ​was upgraded to version 10.
  • grunt jshint:plugins (assuming it is still useful to some extent) fails with any plugin that uses minified / uglified JavaScript files or a node_modules directory, which is pretty the norm today.

Once they are fixed in the related tickets, WordPress should make sure they are also tested so that any future breakage is detected automatically.

Re: lint:jsdoc:
I don't see a reason why it is not included in the precommit tasks. By adding it there, it would also be automatically tested in CI.

Re: grunt jshint:plugins:
I'm not sure how this could be included in some sort of automated testing. Still, it's a script provided by Core and we should make sure that it works. Otherwise, we should consider to remove it.

I didn't go any deeper with checking all the scripts and tasks in package.json and in the Gruntfile. There mey be other scripts / tasks that aren't actually tested in any way it would be worth double checking.

Change History (8)

#1 @iamchitti
7 weeks ago

I checked both on current trunk. You're right about both.

Worth adding about grunt jshint:plugins: it isn't just untested, it's tested in a way that hides the problem. reusable-coding-standards-javascript.yml runs npm run grunt jshint, which runs every target, plugins included. It passes only because src/wp-content/plugins/ in CI holds nothing but hello.php and index.php. So CI stays green while the task is broken for anyone with real plugins in their checkout.

On lint:jsdoc: ESLint 10 no longer reads the old eslintrc format, so both .eslintrc-jsdoc.js and .eslintignore are now ignored. wp-scripts falls back to its own config and lints everything, including src/wp-includes/js and src/wp-admin/js. I ran it for 20+ minutes and got no output at all. It needs a flat eslint.config.js.

Does this split sound right?

  • #65939 — move to a flat config
  • #65923 — decide whether jshint:plugins gets fixed or dropped
  • here - add lint:jsdoc to precommit:js as you suggest, and have the CI job call jshint:corejs directly

Happy to take the patches if that works. I also looked at the other scripts that never run in CI, but that's probably better saved until this part is settled.

#2 @afercia
7 weeks ago

It passes only because src/wp-content/plugins/ in CI holds nothing but hello.php and index.php

@iamchitti good finding. So it appears that on CI running jshint:plugins which is included in npm run grunt jshint is totally useless and misleading.

#65939 and #65923 already have a PR. A review there would be appreciated.

here - add lint:jsdoc to precommit:js as you suggest,

Yes but that needs to happen after all the errors get fixed otherwise it will block CI. See #65951 and the PR linked there.

#3 @iamchitti
7 weeks ago

Thanks @afercia for clearing up sequencing.

Good point on #65951, I'd missed that. So the order here is: #65939 lands, then #65951 fixes the errors, then lint:jsdoc can go into precommit:js. I'll leave that parked until then rather than propose it early. Note - ​https://github.com/WordPress/wordpress-develop/pull/13251 doesn't touch precommit:js itself, so it still needs doing as a follow-up.

The CI change is independent of all that, so I can open a patch now: have reusable-coding-standards-javascript.yml call grunt jshint:corejs instead of grunt jshint, so it stops implying plugins is covered. Shall I put that on this ticket?

This ticket was mentioned in ​PR #13336 on ​WordPress/wordpress-develop by ​@afercia.


6 weeks ago
#4

  • Keywords has-patch added

Trac ticket: https://core.trac.wordpress.org/ticket/65950

[!IMPORTANT]
Do not merge before ​https://github.com/WordPress/wordpress-develop/pull/10743 otherwise the tests on CI will fail for any pull request.

Adds a Grunt task to run the lint:jsdoc as part of the precommit task.

Needs ​https://github.com/WordPress/wordpress-develop/pull/10743 to be merged first.

Test instructions:

  • Make a temporary change to any admin JS file.
  • From the terminal, run grunt precommit.
  • The task should detect that only JS files have been modified and only run the JS tests and sub-tasks, see precommit:js.
  • Observe lint:jsdoc runs as part of precommit:js..
  • Note that until ​https://github.com/WordPress/wordpress-develop/pull/10743 isn't merged yet, lint:jsdoc will report several violations.

## Use of AI Tools

#5 @afercia
6 weeks ago

  • Owner set to afercia
  • Status new → assigned

​@afercia commented on ​PR #13336:


5 weeks ago
#6

Now that all the remaining improvements to the Customizer JS documentation have been committed in Core-40831 running npm run lint:jsdoc on trunk reports 0 errors. It's a good time to make it run on CI.

Further improvements to the JavaScript inline documentation and the JSDoc configuration will be addressed in Core-66033 for the WordPress 7.2 release cycle.

#7 @afercia
5 weeks ago

In 63464:

Build/Test Tools: Integrate lint:jsdoc into the Core development workflow.

This changeset adds a lint:jsdoc Grunt task and includes it in the precommit:js task list.

The existing lint:jsdoc NPM script was restored to a working configuration in #65939. It broke with the upgrade of ESLint to version 10 in the @wordpress/scripts package in the Gutenberg repository. See ​https://github.com/WordPress/gutenberg/pull/76654.

By including lint:jsdoc into CI, future potential breakages will be detected soon. Also, this change aims to reinforce the long-term goal of improving the JavaScript inline documentation that was started years ago in the ​WordPress JavaScript Docs Coding Standards initiative. See ​https://make.wordpress.org/core/handbook/docs/inline/js/.
Future improvements will be part of ongoing maintenance work. See for example #66033.

Developed in ​https://github.com/WordPress/wordpress-develop/pull/13336

Props afercia, westonruter.
See #65939, #65950, #66033.

Note: See TracTickets for help on using tickets.