fix(lint): bring the backend test suites into scope (#298)
The backend lint script covered src and scripts; the frontend's has always covered src and tests. So roughly sixty backend test files had never been linted at all. That was a documented deferral rather than an oversight — the config said so in as many words, because tsconfig.json includes only src and type-aware rules had no program to resolve the test files against. tsconfig.test.json is that program, exactly as frontend/tsconfig.test.json was for the same problem in #137. It is separate from tsconfig.json rather than a widening of it, because that one drives the build and emits to dist, and pulling the suite in would ship the tests. The files were already type-checked at run time by ts-jest; this adds nothing to that, only to what the linter can see. Pointing it at tests produced 77 warnings and no errors. Sixty of those were rules that cannot be true in a test, so they are switched off here rather than left to accumulate — #60's argument, that a gate nobody reads is not a gate, and that a rule which cannot be true is noise hiding the rules that can. Forty-one alone were hardcoded passwords, which are the entire point of a test and which this project's own rule says must live only in test paths, which is here. The rest were a stub server on http to a socket the test opened itself, an RFC 5737 documentation IP, os.tmpdir, Math.random for a run id, and sorting two arrays to compare them. What was left was signal, and it found a real one on the first run. testDb.ts cleaned up settings with LIKE 'email\_%', and in a JavaScript string that backslash does nothing: the pattern is 'email_%', and an underscore in SQL LIKE matches any single character. It meant "email plus any one character" rather than "email_". It deleted the right rows only because no other key begins with those letters followed by something else — a setting called emailing_enabled would have been swept away between suites, silently, in a file that never mentions it. It now uses an explicit ESCAPE clause. It also found five dead `const before: string[] = []` declarations in uploadValidation, left over from #228's redesign of that suite. The tests assert properly through filesSettlingTo; the variables did nothing. Seven warnings remain, all in routesAreWrapped and workflowGate, and all judgement calls about guard-test complexity rather than defects. Leaving them visible is the point of having lint here at all. Closes #298 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -50,9 +50,6 @@ export default tseslint.config(
|
||||
),
|
||||
|
||||
{
|
||||
// `tests/` is deliberately out of scope for now: tsconfig.json only includes
|
||||
// `src`, so type-aware linting has no program for the test files, and
|
||||
// widening it is a separate change with its own violation count.
|
||||
files: ['src/**/*.ts'],
|
||||
languageOptions: {
|
||||
globals: globals.node,
|
||||
@@ -71,5 +68,73 @@ export default tseslint.config(
|
||||
{ checksVoidReturn: { attributes: false } },
|
||||
],
|
||||
},
|
||||
},
|
||||
|
||||
{
|
||||
// The test suites, in scope since #298. They had never been linted at all:
|
||||
// this config said tests were out of scope because tsconfig.json includes
|
||||
// only `src`, and that stayed true for long enough that two defects lived
|
||||
// here undetected — a unit test that opened a real TLS connection to Gmail
|
||||
// on every run, and integration tests that mocked the shared pg pool and
|
||||
// made a suite unrunnable. Neither is something lint would necessarily have
|
||||
// caught, but neither was ever looked at.
|
||||
//
|
||||
// `project` rather than `projectService`, for the reason the frontend's
|
||||
// equivalent block records: the service resolves each file to the nearest
|
||||
// tsconfig.json, which for tests/ is the one that excludes them, and every
|
||||
// file then errors as not part of a project.
|
||||
files: ['tests/**/*.ts'],
|
||||
languageOptions: {
|
||||
globals: { ...globals.node, ...globals.jest },
|
||||
parserOptions: {
|
||||
project: ['./tsconfig.test.json'],
|
||||
tsconfigRootDir: import.meta.dirname,
|
||||
},
|
||||
},
|
||||
rules: {
|
||||
// The same rule src is held to, and it matters at least as much here.
|
||||
// An unawaited promise in a test does not fail the test — it passes,
|
||||
// having asserted nothing, and the failure surfaces later as a suite that
|
||||
// will not exit.
|
||||
'@typescript-eslint/no-floating-promises': 'error',
|
||||
'@typescript-eslint/no-misused-promises': [
|
||||
'error',
|
||||
{ checksVoidReturn: { attributes: false } },
|
||||
],
|
||||
|
||||
// Everything below is switched off for tests rather than left as a
|
||||
// warning, on #60's argument: bringing these files in scope produced 77
|
||||
// warnings, of which 60 were rules that cannot be true in a test. A rule
|
||||
// that cannot be true here is noise, and noise hides the rules that can.
|
||||
// What is left is signal — unused variables, useless escapes, a regex
|
||||
// worth a second look.
|
||||
|
||||
// 41 of the 77. Test credentials are the entire point of a test, and this
|
||||
// project's own rule is that they must live only in test paths — which is
|
||||
// here. Flagging them where they belong trains a reader to skip the rule
|
||||
// where they do not.
|
||||
'sonarjs/no-hardcoded-passwords': 'off',
|
||||
|
||||
// Stub servers and fixtures: `http://127.0.0.1:<port>`. There is no
|
||||
// transport to secure between a test and a socket it opened itself.
|
||||
'sonarjs/no-clear-text-protocols': 'off',
|
||||
|
||||
// 203.0.113.5 is TEST-NET-3, reserved by RFC 5737 for exactly this. A
|
||||
// documentation address is the correct thing to hardcode.
|
||||
'sonarjs/no-hardcoded-ip': 'off',
|
||||
|
||||
// `os.tmpdir()`, via mkdtemp, which is how these suites get a scratch
|
||||
// uploads directory they can delete afterwards.
|
||||
'sonarjs/publicly-writable-directories': 'off',
|
||||
|
||||
// Math.random for a run id. Nothing here is a secret; it only has to not
|
||||
// collide with a parallel worker.
|
||||
'sonarjs/pseudo-random': 'off',
|
||||
|
||||
// Sorting two string arrays to compare them is how several guards assert
|
||||
// set equality. The locale-aware comparator the rule wants would change
|
||||
// nothing except the reading.
|
||||
'sonarjs/no-alphabetical-sort': 'off',
|
||||
},
|
||||
}
|
||||
);
|
||||
|
||||
@@ -8,7 +8,7 @@
|
||||
},
|
||||
"scripts": {
|
||||
"build": "tsc",
|
||||
"lint": "eslint src scripts",
|
||||
"lint": "eslint src scripts tests",
|
||||
"start": "node dist/server.js",
|
||||
"dev": "tsx watch src/server.ts",
|
||||
"test": "npm run test:unit",
|
||||
|
||||
@@ -123,8 +123,16 @@ export async function resetDb(): Promise<void> {
|
||||
// Same reasoning, same failure, for the intake settings (#227): a ceiling of
|
||||
// 1 left behind by the ceiling suite makes every later submission refuse with
|
||||
// a 503, in files that never mention a ceiling.
|
||||
//
|
||||
// ESCAPE, and not a backslash, because the backslash that used to be here did
|
||||
// nothing. In a JavaScript string 'email\_%' is 'email_%', and `_` in SQL LIKE
|
||||
// matches any single character — so the pattern meant "email plus any one
|
||||
// character", not "email_". It happened to delete the right rows only because
|
||||
// no other key begins with those letters followed by something else; a
|
||||
// setting called emailing_enabled would have been swept away silently. Found
|
||||
// by no-useless-escape the first time lint was pointed at tests/ (#298).
|
||||
await testPool.query(
|
||||
`DELETE FROM admin_settings WHERE key LIKE 'email\_%' OR key LIKE 'intake\_%'`
|
||||
`DELETE FROM admin_settings WHERE key LIKE 'email!_%' ESCAPE '!' OR key LIKE 'intake!_%' ESCAPE '!'`
|
||||
);
|
||||
}
|
||||
|
||||
|
||||
@@ -151,8 +151,6 @@ describe('what it refuses', () => {
|
||||
});
|
||||
|
||||
it('leaves nothing on the volume when it refuses the content', async () => {
|
||||
const before: string[] = [];
|
||||
|
||||
await createItem().attach('images', HTML_BYTES, {
|
||||
filename: 'photo.jpg',
|
||||
contentType: 'image/jpeg'
|
||||
@@ -164,8 +162,6 @@ describe('what it refuses', () => {
|
||||
// A rejection must take the whole request with it. Accepting the good file
|
||||
// from a refused upload would leave a file behind that nothing references.
|
||||
it('discards the valid files from a request that also carried an invalid one', async () => {
|
||||
const before: string[] = [];
|
||||
|
||||
const res = await createItem()
|
||||
.attach('images', REAL_PNG, { filename: 'good.png', contentType: 'image/png' })
|
||||
.attach('images', HTML_BYTES, { filename: 'bad.jpg', contentType: 'image/jpeg' });
|
||||
@@ -209,8 +205,6 @@ describe('the uploads directory', () => {
|
||||
*/
|
||||
describe('what it leaves on disk', () => {
|
||||
it('removes the upload when the request is refused for its other fields', async () => {
|
||||
const before: string[] = [];
|
||||
|
||||
const res = await request(app)
|
||||
.post('/api/admin/items')
|
||||
.field('name', 'Photographed thing')
|
||||
@@ -226,8 +220,6 @@ describe('what it leaves on disk', () => {
|
||||
});
|
||||
|
||||
it('removes the upload when the tags field is refused', async () => {
|
||||
const before: string[] = [];
|
||||
|
||||
const res = await request(app)
|
||||
.post('/api/admin/items')
|
||||
.field('name', 'Photographed thing')
|
||||
@@ -243,8 +235,6 @@ describe('what it leaves on disk', () => {
|
||||
// The accepted case must not be swept up by the same cleanup: these files are
|
||||
// the ones the item now points at.
|
||||
it('keeps the upload when the request succeeds', async () => {
|
||||
const before: string[] = [];
|
||||
|
||||
const res = await createItem().attach('images', REAL_PNG, {
|
||||
filename: 'photo.png',
|
||||
contentType: 'image/png'
|
||||
|
||||
@@ -0,0 +1,9 @@
|
||||
{
|
||||
"//": "Exists so ESLint has a TypeScript program covering tests/. tsconfig.json includes only `src`, which is why tests were out of scope for linting until #298 — type-aware rules had nothing to resolve them against. Separate from tsconfig.json rather than widening its `include`, because that one drives `npm run build` and emits to dist/: pulling the suite into the build output would ship the tests. Mirrors frontend/tsconfig.test.json, which exists for the same reason (#137). The files are already type-checked at run time by ts-jest; this adds nothing to that, only to what the linter can see.",
|
||||
"extends": "./tsconfig.json",
|
||||
"compilerOptions": {
|
||||
"noEmit": true,
|
||||
"rootDir": "."
|
||||
},
|
||||
"include": ["tests", "src"]
|
||||
}
|
||||
Reference in New Issue
Block a user