Fix test quality issues: null assertion, misleading test name, arrow-function this, semver comparison, and changed_db flag

[Copilot]

Several correctness and clarity issues in test utilities and middleware tests, ranging from a risky non-null assertion to a semantically wrong version-range comparison in the D1 mock.

middleware.test.ts

• ETag null guard: replace etag! with a preceding expect(etag).toBeTruthy() assertion and etag as string cast, making the failure explicit rather than a silent runtime error.
• Misleading test name: rename "should set Vary header when returning empty results" → "should not set Vary header when GitHub API fails" — the test hits the 503 code path where the app never sets Vary, so the name and toBeNull() assertion now match both each other and the actual application behaviour.

test/utils/d1-mock.ts

• Remove this alias: drop const mockDb = this + its eslint-disable comment; convert bind and all D1PreparedStatement methods to arrow functions so this is the MockD1Database instance throughout via lexical closure.

• Semver comparison: replace string >=/<= version comparison (wrong for e.g. "0.10.0" <= "0.2.0") with proper numeric component comparison:

function compareVersions(a: string, b: string): number {
  const aParts = a.split(".").map((p) => parseInt(p, 10));
  const bParts = b.split(".").map((p) => parseInt(p, 10));
  // compare element-by-element numerically …
}

• changed_db flag in DELETE: was unconditionally true; changed to wasDeleted to accurately reflect whether a row was actually removed.

Original prompt

Please apply the following diffs and create a pull request.
Once the PR is ready, give it a title based on the messages of the fixes being applied.

[{"message":"Using non-null assertion operator (!) is risky in tests. Consider using a proper null check or assertion to ensure etag is defined before using it.","fixFiles":[{"filePath":"test/services/versions/v1/middleware.test.ts","diff":"diff --git a/test/services/versions/v1/middleware.test.ts b/test/services/versions/v1/middleware.test.ts\n--- a/test/services/versions/v1/middleware.test.ts\n+++ b/test/services/versions/v1/middleware.test.ts\n@@ -145,13 +145,14 @@\n       const response1 = await app.request(\"/versions/v1\", {}, env, ctx1);\n       await waitOnExecutionContext(ctx1);\n       const etag = response1.headers.get(\"ETag\");\n+      expect(etag).toBeTruthy();\n \n       const ctx2 = createExecutionContext();\n       const response2 = await app.request(\n         \"/versions/v1\",\n         {\n           headers: {\n-            \"If-None-Match\": etag!\n+            \"If-None-Match\": etag\n           }\n         },\n         env,\n"}]},{"message":"Test description is misleading - it's testing error handling when GitHub API fails (503 status), not 'empty results'. The test should be renamed to reflect what it's actually testing.","fixFiles":[{"filePath":"test/services/versions/v1/middleware.test.ts","diff":"diff --git a/test/services/versions/v1/middleware.test.ts b/test/services/versions/v1/middleware.test.ts\n--- a/test/services/versions/v1/middleware.test.ts\n+++ b/test/services/versions/v1/middleware.test.ts\n@@ -197,7 +197,7 @@\n       expect(cacheControl).toBeTruthy();\n     });\n \n-    it(\"should set Vary header when returning empty results\", async () => {\n+    it(\"should not set Vary header when GitHub API fails\", async () => {\n       (vi.mocked(ghRequest) as MockGitHubRequest).mockRejectedValueOnce(\n         new Error(\"GitHub API Error\")\n       );\n"}]},{"message":"This assertion seems incorrect - if the test is about 'Vary header when returning empty results', it should verify the Vary header is present, not null. The test logic doesn't match its description.","fixFiles":[{"filePath":"test/services/versions/v1/middleware.test.ts","diff":"diff --git a/test/services/versions/v1/middleware.test.ts b/test/services/versions/v1/middleware.test.ts\n--- a/test/services/versions/v1/middleware.test.ts\n+++ b/test/services/versions/v1/middleware.test.ts\n@@ -207,7 +207,7 @@\n       await waitOnExecutionContext(ctx);\n \n       expect(response.status).toBe(503);\n-      expect(response.headers.get(\"Vary\")).toBeNull();\n+      expect(response.headers.get(\"Vary\")).toBeTruthy();\n     });\n   });\n \n"}]},{"message":"Capturing 'this' in a variable to work around closure issues is unnecessary. Use arrow functions for the bind method's return object methods instead to preserve the correct 'this' context.","fixFiles":[{"filePath":"test/utils/d1-mock.ts","diff":"diff --git a/test/utils/d1-mock.ts b/test/utils/d1-mock.ts\n--- a/test/utils/d1-mock.ts\n+++ b/test/utils/d1-mock.ts\n@@ -36,11 +36,8 @@\n   }\n \n   prepare(query: string): D1PreparedStatement {\n-    // eslint-disable-next-line @typescript-eslint/no-this-alias\n-    const mockDb = this;\n-\n     return {\n-      bind(...params: unknown[]) {\n+      bind: (...params: unknown[]) => {\n         return {\n           async all(): Promise<D1Result<DatabaseAlert>> {\n             // Simulate getting alerts by version\n@@ -82,7 +78,7 @@\n             // Simulate getting alert by ID\n             if (query.includes(\"WHERE id = ?\")) {\n               const id = params[0];\n-              const alert = mockDb.alerts.find((a) => a.id === id);\n+              const alert = this.alerts.find((a) => a.id === id);\n               return {\n                 success: true,\n                 results: alert ? [alert] : [],\n@@ -119,7 +115,7 @@\n             // Simulate getting first result (for getAlertById)\n             if (query.includes(\"WHERE id = ?\")) {\n               const id = params[0] as string;\n-              const alert = mockDb.alerts.find((a) => a.id === id);\n+              const alert = this.alerts.find((a) => a.id === id);\n               return (alert as T | undefined) || null;\n             }\n \n"}]},{"message":"String comparison is used for version comparison, which will produce incorrect results (e.g., '0.10.0' < '0.2.0' in string comparison). Implement proper semantic version comparison using a library like semver or a custom comparison function that splits versions into numeric components.","fixFiles":[{"filePath":"test/utils/d1-mock.ts","diff":"diff --git a/test/utils/d1-mock.ts b/test/utils/d1-mock.ts\n--- a/test/utils/d1-mock.ts\n+++ b/test/utils/d1-mock.ts\n@@ -10,6 +10,45 @@\n   buttons?: string; // JSON string in database\n };\n \n+/**\n+ * Compare two semantic version strings (e.g. \"1.2.3\") numerically.\n+ * Returns -1 if a < b, 0 if a == b, 1 if...

---

🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.

[admdly]

Generated by GitHub Advanced Security tool as code quality fixes.

[cloudflare-workers-and-pages]

Deploying with    Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status	Name	Latest Commit	Preview URL	Updated (UTC)
✅ Deployment successful! View logs	api-worker	d5bf11c	Commit Preview URL Branch Preview URL	Mar 17 2026, 12:41 PM

[Copilot]

Pull request overview

This PR improves correctness and clarity in the test suite by removing risky assertions, aligning test names with actual behavior, and fixing logic errors in the D1 mock used by tests.

Changes:

• Harden ETag-related middleware test by asserting ETag presence before use (removing the non-null assertion).
• Rename a misleading middleware test to match the exercised 503/GitHub-failure code path.
• Fix D1 mock behavior by using numeric semantic version comparison, removing this aliasing via arrow functions, and making changed_db reflect whether a DELETE actually removed a row.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
test/utils/d1-mock.ts	Fixes semver range filtering, this handling in prepared statements, and DELETE metadata accuracy in the mock D1 implementation.
test/services/versions/v1/middleware.test.ts	Makes ETag usage safer and renames a test to accurately describe the GitHub API failure behavior being asserted.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.