Skip to content

fix(tests): replace dynamic-import + resetModules with static imports in announcement tests #3118

@bokelley

Description

@bokelley

Context

PR #3098 (Closes #3092) reduced announcement test flakiness under --pool=threads by adding vi.resetModules() to beforeEach in all 7 affected files and bumping the test timeout to 10s. Under heavy multi-workspace Conductor load, 2–4 tests in tests/announcement/announcement-backlog.test.ts still flake intermittently:

FAIL  tests/announcement/announcement-backlog.test.ts > loadAnnouncementBacklog > returns one row per org with flags derived from the subquery join
Error: Test timed out in 10000ms.

FAIL  tests/announcement/announcement-backlog.test.ts > loadAnnouncementBacklog > coerces non-boolean is_backfill to false (legacy rows)
TypeError: Cannot read properties of undefined (reading 'is_backfill')

These do not reproduce on isolated runs (tests/announcement/ alone passes 172/172 in ~7s; announcement-backlog.test.ts alone passes 8/8 in ~7s) — they only manifest in the full tests/ run under thread-pool contention.

Root cause (deeper than #3098 addressed)

#3098's vi.resetModules() is a workaround for an over-defensive mocking pattern, not a fix for the underlying cost. The pattern in all 7 announcement test files:

beforeEach(() => {
  vi.resetModules();
  vi.clearAllMocks();
  mockQuery.mockReset();
});

it('...', async () => {
  mockQuery.mockResolvedValueOnce({rows: [...]});
  const { loadAnnouncementBacklog } =
    await import('../../server/src/addie/jobs/announcement-handlers.js');
  ...
});

Each test calls await import() after vi.resetModules(), which re-resolves the entire transitive module tree for announcement-handlers.ts:

  • 8 first-order imports → including addie/mcp/admin-tools (17+ first-order imports of its own: OrganizationDatabase, SlackDatabase, WorkingGroupDatabase, BrandDatabase, MemberDatabase, brand-enrichment, prospect, …)
  • Transitively pulls in ~30 MCP files + 7 Slack files + DB clients + config modules + types

So every individual test re-evaluates tens to hundreds of TypeScript files. 7 announcement test files × ~8 tests/file = ~56 full-tree re-resolutions per precommit run. Under thread-pool contention, the re-resolution race window opens up enough that some module init path consumes the mockResolvedValueOnce queue before the test's actual loadAnnouncementBacklog() call reaches it — the test then sees mockQuery returning the safety-net default {rows: []} and crashes on rows[0].

vi.resetModules() was likely added by the original authors out of a belief that the vi.mock factory wouldn't apply to subsequently-imported modules without it. That belief is wrong: vi.mock is hoisted to the top of the file by vitest and applies to every subsequent import of the mocked path, including dynamic ones, regardless of the module registry's state.

Proposal — durable fix

Convert all 7 announcement test files from dynamic await import() inside tests to top-level static imports. Drop vi.resetModules() from beforeEach. The standard pattern is sufficient:

import { loadAnnouncementBacklog } from '../../server/src/addie/jobs/announcement-handlers.js';

const { mockQuery } = vi.hoisted(() => ({ mockQuery: vi.fn<any>() }));
vi.mock('../../server/src/db/client.js', () => ({
  query: (...args: unknown[]) => mockQuery(...args),
}));

beforeEach(() => {
  mockQuery.mockReset();
  mockQuery.mockResolvedValue({rows: []});  // safety net stays
});

it('...', async () => {
  mockQuery.mockResolvedValueOnce({rows: [...]});
  const rows = await loadAnnouncementBacklog();
  ...
});

Why this works: vi.mock is hoisted, so the static import at the top resolves through the mock factory. The mockQuery reference (via vi.hoisted) is stable across tests. mockReset() drains both the queue and call history per test. No transitive re-resolution per test → no race window → no flakes.

Effort estimate: ~30 lines per file × 7 files = ~200 LOC of mechanical changes. No production code changes.

Files to fix

  • tests/announcement/announcement-backlog.test.ts
  • tests/announcement/announcement-backlog-route.test.ts
  • tests/announcement/announcement-admin-route.test.ts
  • tests/announcement/announcement-handlers.test.ts
  • tests/announcement/announcement-backfill.test.ts
  • tests/announcement/announcement-channel-resolver.test.ts
  • tests/announcement/announcement-reminder.test.ts

Refs #3092, follow-up to #3098.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions