chore(lint): let the linter see tests/ and scripts/ - #920
Merged
Conversation
`npm run lint` was `eslint src`, so two whole trees were never linted. Across
the fleet that hid roughly 3,900 errors, none of which any CI leg had shown.
Most of it was the config, not the code, and the same two defects were in
every app.
**scripts/ had no config block at all.** These are CommonJS Node CLI checkers,
and flat config defaults every `.js` to ESM with browser-ish globals, so eslint
read the CommonJS wrapper itself as undefined identifiers: `require`,
`process`, `__dirname`, `__filename`, `module`. A `scripts/**` block now
declares the environment. Declaring beats suppressing here: `no-undef` is the
rule that catches a genuinely misspelled identifier, and dozens of fake
findings would bury a real one. A second block covers `scripts/**/*.mjs`, which
is ESM and needs Node's globals without the CommonJS wrapper.
**The tests block applied a non-TypeScript-aware rule to TypeScript.** It named
`tests/**/*.ts` while setting the CORE `no-unused-vars`, which v9 deliberately
turns off for `.ts` in favour of the `@typescript-eslint` version. The core
rule reads the parameter names inside a function TYPE as bindings, so
t?: (app: string, key: string) => string
reports `app` and `key` as unused variables, and every unused `catch (e)` in a
`.ts` spec reports twice. The block is split now: `.js`/`.mjs` on the core
rule, `.ts`/`.tsx` on the TypeScript one, same patterns on both.
Also: stale `eslint-disable` comments naming plugins eslint 10 no longer
registers, which are themselves errors ("Definition for rule ... was not
found"), and a rule that must not parse shell scripts.
The genuinely real findings were the useful part: dead locals, unused imports,
dead helper functions, unused `catch` bindings, extensionless relative imports,
and a handful of `== null` comparisons spelled out so they still match null AND
undefined.
Verified per app: `npm run lint` 0 errors over src + tests + scripts,
`prettier --check` clean, and the unit suite still green.
One finding here was a real defect rather than tidiness. `no-dupe-keys`
flagged this assertion:
expect(moderationItemTitle({ name: ' ', name: 'Real' })).toBe('Real')
The second `name` silently overwrites the first, so the object actually built
was `{ name: 'Real' }` and the test named "ignores blank/whitespace title
fields" never took the blank branch at all. It could not have failed for the
reason it claimed. It now passes `{ name: ' ', title: 'Real' }`, which does
exercise the fall-through.
`TITLE_FIELDS` carried the same slip, listing `'name'` twice, so the second
entry was dead. Removed. The suite is 5 passed.
Contributor
Quality Report — ConductionNL/stackiq @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| check-manifest | ✅ | ||||
| check-vue-demi | ✅ | ||||
| test-l10n | ✅ | ||||
| format | ✅ | ||||
| check-schema-l10n | ✅ | ||||
| check-l10n-js | ✅ | ||||
| composer | ✅ | ✅ 130/130 | |||
| npm | ✅ | ✅ 711/711 | |||
| app:check-code | ⏭️ | ||||
| info.xml | ✅ | ||||
| REUSE | ❌ | ||||
| PHPUnit | ✅ | ||||
| Newman | ⏭️ | ||||
| Playwright | ⏭️ deferred — runs on the promotion into beta/main, not on a pull request into development | ||||
| Hydra gates | ✅ |
Quality workflow — 2026-09-01 10:05 UTC
Download the full PDF report from the workflow artifacts.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
npm run lintwaseslint src, sotests/andscripts/were never linted at all. Across the fleet that hid roughly 3,900 errors, none of which any CI leg had ever shown.Most of it was the config, not the code, and the same two defects were in every app.
scripts/had no config block at allThese are CommonJS Node CLI checkers, and flat config defaults every
.jsto ESM with browser-ish globals, so eslint read the CommonJS wrapper itself as undefined identifiers:require,process,__dirname,__filename,module. Ascripts/**block now declares the environment, and a second block coversscripts/**/*.mjs, which is genuinely ESM and needs Node's globals without the CommonJS wrapper.Declaring beats suppressing:
no-undefis the rule that catches a genuinely misspelled identifier, and dozens of fake findings would bury a real one.The tests block applied a non-TypeScript-aware rule to TypeScript
It named
tests/**/*.tswhile setting the coreno-unused-vars, which v9 deliberately turns off for.tsin favour of the@typescript-eslintversion. The core rule reads parameter names inside a function type as bindings, soreported
appandkeyas unused variables, and every unusedcatch (e)in a.tsspec reported twice. The block is now split:.js/.mjson the core rule,.ts/.tsxon the TypeScript one, same patterns on both.Two more that made eslint report on things it should not have
eslint-disablecomments naming plugins eslint 10 no longer registers. An inline disable for an unregistered plugin is itself an error (Definition for rule "n/shebang" was not found), so these were findings about the suppression, not the code.no-irregular-whitespace/ shell files: eslint was parsing a.shscript that matched a**/*.test.*glob and reporting a syntax error about a file it should never have opened.The real findings
The useful part: dead locals, unused imports, dead helper functions, unused
catchbindings, extensionless relative imports, and== nullcomparisons spelled out so they still match null and undefined.Where a rule was genuinely wrong for the code, it got a reasoned exception rather than a silent one, e.g. a suite that compiles as CommonJS and must use
requirebecauseimport.metais a syntax error there.Verified
npm run lint(nowsrc tests scripts): 0 errorsprettier --check: cleancheck:manifest,check:l10n, parity guards) still run and report the same baselinesPart of a fleet-wide pass. The
lintscript is widened only now that the tree is actually clean, so a half-done app cannot report green.