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
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:jsdocis 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 anode_modulesdirectory, 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)
#2
@
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
@
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:jsdocruns as part ofprecommit:js.. - Note that until https://github.com/WordPress/wordpress-develop/pull/10743 isn't merged yet,
lint:jsdocwill report several violations.
## Use of AI Tools
@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.
@afercia commented on PR #13336:
5 weeks ago
#8
Committed in https://core.trac.wordpress.org/changeset/63464
![(please configure the [header_logo] section in trac.ini)](/chrome/site/your_project_logo.png)
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.ymlrunsnpm run grunt jshint, which runs every target,pluginsincluded. It passes only becausesrc/wp-content/plugins/in CI holds nothing buthello.phpandindex.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.jsand.eslintignoreare now ignored.wp-scriptsfalls back to its own config and lints everything, includingsrc/wp-includes/jsandsrc/wp-admin/js. I ran it for 20+ minutes and got no output at all. It needs a flateslint.config.js.Does this split sound right?
jshint:pluginsgets fixed or droppedlint:jsdoctoprecommit:jsas you suggest, and have the CI job calljshint:corejsdirectlyHappy 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.