diff --git a/prds/6-e2e-test-forgejo-triggers.md b/prds/6-e2e-test-forgejo-triggers.md index 834d0ac..e4bc985 100644 --- a/prds/6-e2e-test-forgejo-triggers.md +++ b/prds/6-e2e-test-forgejo-triggers.md @@ -31,6 +31,18 @@ Systematically exercise each untested scenario on the live Forgejo instance (git - Ensure the workflow has the correct permissions for each trigger type - **Validation**: Workflow YAML includes all required `on:` triggers +- [x] **M1.5: Fix image-downloader test failures blocking CI** + + - **Problem**: All 18 `downloadCommentImages` tests failed on Forgejo CI (Linux Bun 1.2.21) while passing locally on macOS. This blocked all PRs. + - **Root cause**: `mock.module("fs/promises")` on Linux Bun poisons the entire mock tracking system — not just fs mocks, but ALL mock/spy assertions in the file (octokit parameter mocks, `spyOn(global, "fetch")`, etc.) stopped recording calls. The only passing test used a **negative** assertion (`not.toHaveBeenCalledWith`) which trivially passes when tracking is broken. + - **History of failed attempts**: + 1. Upstream uses `spyOn(fs, "mkdir")` — works on GitHub CI (Bun 1.2.12/ubuntu) but fails on Forgejo Linux (different `fs/promises` object references between test and source modules) + 2. `96ec04e` tried `_fsForTest` mutable export — abandoned + 3. `cff360d` tried dependency injection — replaced + 4. `dab92ef` tried `mock.module("fs/promises")` — works on macOS, poisons mock tracking on Linux + - **Fix**: Dependency injection via optional `fsDeps` parameter on `downloadCommentImages()`. Tests pass mock `mkdir`/`writeFile` directly — no module-level mocking at all. Production callers omit the parameter and get real `fs`. Zero `mock.module` usage in the test file. + - **Validation**: 797/797 tests pass locally, typecheck clean. Awaiting CI confirmation. + - [ ] **M2: Test PR review comment trigger** - Create a test PR on the repo @@ -89,6 +101,7 @@ Systematically exercise each untested scenario on the live Forgejo instance (git | Forgejo webhook payloads differ from expected | Log raw payloads, compare with GitHub format, adapt parsing | | Trigger type not supported by Forgejo Actions | Document as known limitation, skip gracefully | | Action creates unwanted branches/comments during testing | Use a dedicated test repo or clean up after each test | +| Bun mock.module breaks mock tracking on Linux | Use dependency injection for fs mocks, never mock.module | ## Success Criteria diff --git a/src/github/utils/image-downloader.ts b/src/github/utils/image-downloader.ts index 0b2c0c7..28f3018 100644 --- a/src/github/utils/image-downloader.ts +++ b/src/github/utils/image-downloader.ts @@ -2,6 +2,15 @@ import fs from "fs/promises"; import path from "path"; import type { Octokits } from "../api/client"; +/** Injectable fs operations for cross-platform test compatibility. + * On Linux Bun, mock.module("fs/promises") poisons the entire mock tracking + * system, and spyOn(fs, "mkdir") gives test/source different object refs. + * Dependency injection sidesteps all of this. */ +export interface FsDeps { + mkdir: typeof fs.mkdir; + writeFile: typeof fs.writeFile; +} + // Build regexes at call time, reading GITHUB_SERVER_URL from process.env // directly. The config.ts export freezes the value at module load time, // which can be wrong when the Forgejo CI runner injects its own @@ -63,11 +72,13 @@ export async function downloadCommentImages( owner: string, repo: string, comments: CommentWithImages[], + fsDeps?: FsDeps, ): Promise> { + const { mkdir, writeFile } = fsDeps ?? fs; const urlToPathMap = new Map(); const downloadsDir = "/tmp/github-images"; - await fs.mkdir(downloadsDir, { recursive: true }); + await mkdir(downloadsDir, { recursive: true }); const { imageRegex, htmlImgRegex } = buildImageRegexes(); @@ -217,7 +228,7 @@ export async function downloadCommentImages( const arrayBuffer = await imageResponse.arrayBuffer(); const buffer = Buffer.from(arrayBuffer); - await fs.writeFile(localPath, buffer); + await writeFile(localPath, buffer); console.log(`✓ Saved: ${localPath}`); urlToPathMap.set(originalUrl, localPath); diff --git a/test/image-downloader.test.ts b/test/image-downloader.test.ts index 7c453b9..1b61b47 100644 --- a/test/image-downloader.test.ts +++ b/test/image-downloader.test.ts @@ -9,22 +9,10 @@ import { setSystemTime, } from "bun:test"; -// Mock fs/promises at the module loader level. Bun hoists mock.module calls -// before imports, so the source module's `import fs from "fs/promises"` gets -// these mocks. This avoids cross-module jest.fn() tracking bugs on Linux Bun. -const mockMkdir = mock(() => Promise.resolve(undefined as any)); -const mockWriteFile = mock(() => Promise.resolve(undefined as any)); - -mock.module("fs/promises", () => ({ - default: { - mkdir: mockMkdir, - writeFile: mockWriteFile, - }, -})); - import { downloadCommentImages, type CommentWithImages, + type FsDeps, } from "../src/github/utils/image-downloader"; import type { Octokits } from "../src/github/api/client"; @@ -35,6 +23,14 @@ describe("downloadCommentImages", () => { let fetchSpy: any; let savedServerUrl: string | undefined; + // DI mocks for fs — avoids mock.module which poisons mock tracking on Linux Bun + const mockMkdir = mock(() => Promise.resolve(undefined as any)); + const mockWriteFile = mock(() => Promise.resolve(undefined as any)); + const fsDeps: FsDeps = { + mkdir: mockMkdir as any, + writeFile: mockWriteFile as any, + }; + beforeEach(() => { // Pin GITHUB_SERVER_URL so tests pass regardless of runner environment. // The image-downloader reads process.env directly at call time. @@ -88,7 +84,7 @@ describe("downloadCommentImages", () => { const mockOctokit = createMockOctokit(); const comments: CommentWithImages[] = []; - await downloadCommentImages(mockOctokit, "owner", "repo", comments); + await downloadCommentImages(mockOctokit, "owner", "repo", comments, fsDeps); expect(mockMkdir).toHaveBeenCalledWith("/tmp/github-images", { recursive: true, @@ -110,6 +106,7 @@ describe("downloadCommentImages", () => { "owner", "repo", comments, + fsDeps, ); expect(result.size).toBe(0); @@ -155,6 +152,7 @@ describe("downloadCommentImages", () => { "owner", "repo", comments, + fsDeps, ); expect(mockOctokit.rest.issues.getComment).toHaveBeenCalledWith({ @@ -217,6 +215,7 @@ describe("downloadCommentImages", () => { "owner", "repo", comments, + fsDeps, ); expect(mockOctokit.rest.pulls.getReviewComment).toHaveBeenCalledWith({ @@ -266,6 +265,7 @@ describe("downloadCommentImages", () => { "owner", "repo", comments, + fsDeps, ); expect(mockOctokit.rest.pulls.getReview).toHaveBeenCalledWith({ @@ -315,6 +315,7 @@ describe("downloadCommentImages", () => { "owner", "repo", comments, + fsDeps, ); expect(mockOctokit.rest.issues.get).toHaveBeenCalledWith({ @@ -365,6 +366,7 @@ describe("downloadCommentImages", () => { "owner", "repo", comments, + fsDeps, ); expect(mockOctokit.rest.pulls.get).toHaveBeenCalledWith({ @@ -418,6 +420,7 @@ describe("downloadCommentImages", () => { "owner", "repo", comments, + fsDeps, ); expect(fetchSpy).toHaveBeenCalledTimes(2); @@ -471,6 +474,7 @@ describe("downloadCommentImages", () => { "owner", "repo", comments, + fsDeps, ); expect(fetchSpy).toHaveBeenCalledTimes(1); // Only downloaded once @@ -506,6 +510,7 @@ describe("downloadCommentImages", () => { "owner", "repo", comments, + fsDeps, ); expect(result.size).toBe(0); @@ -548,6 +553,7 @@ describe("downloadCommentImages", () => { "owner", "repo", comments, + fsDeps, ); expect(result.size).toBe(0); @@ -579,6 +585,7 @@ describe("downloadCommentImages", () => { "owner", "repo", comments, + fsDeps, ); expect(result.size).toBe(0); @@ -652,6 +659,7 @@ describe("downloadCommentImages", () => { "owner", "repo", comments, + fsDeps, ); expect(result.get(url)).toBe( `/tmp/github-images/image-${1704067200000 + callIndex}-0${ext}`, @@ -698,6 +706,7 @@ describe("downloadCommentImages", () => { "owner", "repo", comments, + fsDeps, ); expect(fetchSpy).toHaveBeenCalledTimes(1); @@ -745,6 +754,7 @@ describe("downloadCommentImages", () => { "owner", "repo", comments, + fsDeps, ); expect(mockOctokit.rest.issues.getComment).toHaveBeenCalledWith({ @@ -811,6 +821,7 @@ describe("downloadCommentImages", () => { "owner", "repo", comments, + fsDeps, ); expect(fetchSpy).toHaveBeenCalledTimes(2); @@ -863,6 +874,7 @@ describe("downloadCommentImages", () => { "owner", "repo", comments, + fsDeps, ); expect(fetchSpy).toHaveBeenCalledTimes(2); @@ -911,6 +923,7 @@ describe("downloadCommentImages", () => { "owner", "repo", comments, + fsDeps, ); expect(fetchSpy).toHaveBeenCalledTimes(1); // Only downloaded once @@ -957,6 +970,7 @@ describe("downloadCommentImages", () => { "owner", "repo", comments, + fsDeps, ); expect(fetchSpy).toHaveBeenCalledTimes(1);