Merge pull request 'fix(lint): bring the backend test suites into scope (#298)' (#299) from fix/298-lint-backend-tests into main

Reviewed-on: #299

backend/eslint.config.mjs

@@ -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',
+    },
   }
 );

backend/package.json

@@ -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",

backend/tests/integration/setup/testDb.ts

@@ -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 '!'`
   );
 }
 

backend/tests/integration/uploadValidation.integration.test.ts

@@ -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'

backend/tsconfig.test.json

@@ -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"]
+}