fix: make workspace test gate deterministic (#1175)

This commit is contained in:
Brad Groux 2026-08-23 10:51:29 -05:00 committed by GitHub
parent 1faff783ff
commit e24b75cf3b
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
18 changed files with 274 additions and 38 deletions

View file

@ -371,6 +371,7 @@ jobs:
pnpm --filter "$package_filter" exec vitest related \
--run \
--maxWorkers=4 \
--passWithNoTests \
"${extra_args[@]}" \
"${related_files[@]}"
@ -476,6 +477,8 @@ jobs:
echo "- Selection reason: $SELECTION_REASON"
echo "- Workflow checkout SHA: \`$GITHUB_SHA\`"
echo "- Current job status: \`$CURRENT_JOB_STATUS\`"
echo "- Unit-test workspaces: \`server, web, cli, mcp\`"
echo "- Workspace workers: \`4 maximum per Vitest project\`"
} >> "$GITHUB_STEP_SUMMARY"
- name: Record full-tier skip

View file

@ -58,9 +58,9 @@ pnpm build
pnpm dev
# Tests
pnpm test # Vitest across server, web, mcp, cli
pnpm test:unit # Per-workspace tests sequentially
pnpm test:e2e # Playwright end-to-end
pnpm test # Canonical sequential workspace unit gate
pnpm test:unit # Shared build, then server, web, CLI, and MCP
pnpm test:e2e # Playwright end-to-end, zero retries
# Type check (builds shared first)
pnpm typecheck

View file

@ -305,12 +305,19 @@ Follow the existing conventions in `.eslintrc.*`, `.prettierrc`, and `tsconfig.j
pnpm test
```
This is the canonical unit gate. It builds the shared package, then runs the
server, web, CLI, and MCP suites sequentially with at most four Vitest workers
per project. The final line reports PASS, FAIL, or NOT RUN for every workspace.
- **End-to-end tests** use [Playwright](https://playwright.dev/):
```bash
pnpm test:e2e
```
Playwright does not retry failures. Screenshots and traces from the first
failure are retained in `test-results/` and uploaded by Scheduled QA.
- **Load smoke tests** use [k6](https://k6.io/):
```bash
@ -329,16 +336,16 @@ Follow the existing conventions in `.eslintrc.*`, `.prettierrc`, and `tsconfig.j
### CI tiers
| Trigger | Stable checks | Scope |
| ----------------------------------------------------------------------------------------------------------- | --------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------- |
| Documentation-only pull request or merge | Static gates; unit-test jobs record skip decisions | No workspace unit suite |
| Ordinary code pull request | `Lint & Type Check`, `Changed Tests`, `Build`, `Security Audit` | Vitest `related` coverage for affected server, web, CLI, or MCP workspaces |
| Ordinary code merge to `main` | Default static gates plus `Changed Tests` | Related coverage limited to affected workspaces |
| Pull request with `ci:full`, or a CI selector/workflow control change | Default checks plus `Workspace Unit Tests` | Complete workspace, desktop readiness regressions, and exact dual-storage parity |
| Merge whose reviewed head already passed `Workspace Unit Tests` | Static gates; both unit-test tiers record skip decisions | Reuses exact successful head evidence when that head is an ancestor of the merge commit |
| Nightly 08:00 UTC or manual `CI` dispatch with `test_scope=full` | Static gates, `Workspace Unit Tests`, `Build`, `Security Audit` | Complete authoritative workspace suite |
| Manual `CI` dispatch with `test_scope=focused` and optional `base_sha` | Static gates plus the selected unit-test tier | Classifies `base_sha...HEAD` (or `HEAD^...HEAD`) and stays focused unless CI controls changed |
| Desktop/package/release-workflow pull request, relevant `main` push, or manual `Desktop Artifacts` dispatch | Unsigned macOS, Linux, and Windows artifact jobs | Cross-platform packaging |
| Trigger | Stable checks | Scope |
| ----------------------------------------------------------------------------------------------------------- | --------------------------------------------------------------- | --------------------------------------------------------------------------------------------- |
| Documentation-only pull request or merge | Static gates; unit-test jobs record skip decisions | No workspace unit suite |
| Ordinary code pull request | `Lint & Type Check`, `Changed Tests`, `Build`, `Security Audit` | Vitest `related` coverage for affected server, web, CLI, or MCP workspaces |
| Ordinary code merge to `main` | Default static gates plus `Changed Tests` | Related coverage limited to affected workspaces |
| Pull request with `ci:full`, or a CI selector/workflow control change | Default checks plus `Workspace Unit Tests` | Complete workspace, desktop readiness regressions, and exact dual-storage parity |
| Merge whose reviewed head already passed `Workspace Unit Tests` | Static gates; both unit-test tiers record skip decisions | Reuses exact successful head evidence when that head is an ancestor of the merge commit |
| Nightly 08:00 UTC or manual `CI` dispatch with `test_scope=full` | Static gates, `Workspace Unit Tests`, `Build`, `Security Audit` | Complete authoritative workspace suite |
| Manual `CI` dispatch with `test_scope=focused` and optional `base_sha` | Static gates plus the selected unit-test tier | Classifies `base_sha...HEAD` (or `HEAD^...HEAD`) and stays focused unless CI controls changed |
| Desktop/package/release-workflow pull request, relevant `main` push, or manual `Desktop Artifacts` dispatch | Unsigned macOS, Linux, and Windows artifact jobs | Cross-platform packaging |
`Select Test Scope` is the decision record for each run. Its summary names the
event, exact base/head range, changed-path count, selected tier, affected

View file

@ -837,7 +837,7 @@ pnpm build # Production build
pnpm typecheck # TypeScript strict check
pnpm lint # ESLint
pnpm lint:budget # ESLint with current warning budget
pnpm test # Unit tests (Vitest)
pnpm test # Canonical unit gate (server, web, CLI, MCP)
pnpm test:e2e # E2E tests (Playwright)
pnpm test:load:smoke # k6 API smoke test
pnpm validate:release # Release readiness checks

View file

@ -9,6 +9,7 @@
"scripts": {
"build": "tsc",
"dev": "tsx src/index.ts",
"test": "vitest run --maxWorkers=4",
"typecheck": "tsc --noEmit"
},
"dependencies": {

View file

@ -10,6 +10,7 @@
"scripts": {
"build": "tsc",
"dev": "tsx src/index.ts",
"test": "vitest run --maxWorkers=4",
"start": "node dist/index.js"
},
"dependencies": {

View file

@ -38,8 +38,8 @@
"check:pnpm-settings": "node scripts/check-pnpm-settings.mjs",
"check:security-artifacts": "node scripts/check-security-artifacts.mjs",
"typecheck": "pnpm --filter @veritas-kanban/shared build && pnpm -r typecheck",
"test": "vitest run",
"test:unit": "pnpm -r --workspace-concurrency=1 test",
"test": "pnpm test:unit",
"test:unit": "node --test scripts/run-workspace-unit-tests.test.mjs && node scripts/run-workspace-unit-tests.mjs",
"test:ci-scope": "node --test scripts/select-ci-test-scope.test.mjs",
"test:e2e": "playwright test",
"test:e2e:headed": "playwright test --headed",

View file

@ -33,7 +33,7 @@ export default defineConfig({
testDir: './e2e',
fullyParallel: false, // Run sequentially — tests may share board state
forbidOnly: !!process.env.CI,
retries: process.env.CI ? 2 : 0,
retries: 0,
workers: 1,
reporter:
process.env.CI && process.env.PLAYWRIGHT_HTML_REPORT === '1'
@ -45,7 +45,7 @@ export default defineConfig({
use: {
baseURL: 'http://127.0.0.1:3000',
trace: 'on-first-retry',
trace: 'retain-on-failure',
screenshot: 'only-on-failure',
// Auth — read admin key from server/.env (loaded via dotenv above)
extraHTTPHeaders: {

View file

@ -0,0 +1,121 @@
#!/usr/bin/env node
import { spawnSync } from 'node:child_process';
import { readFileSync } from 'node:fs';
import { fileURLToPath } from 'node:url';
import path from 'node:path';
const repositoryRoot = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '..');
const pnpmExecutable = process.platform === 'win32' ? 'pnpm.cmd' : 'pnpm';
export const UNIT_TEST_STAGES = Object.freeze([
{
id: 'shared-build',
label: 'shared prerequisite',
directory: 'shared',
packageName: '@veritas-kanban/shared',
requiredScript: 'build',
},
{
id: 'server',
label: 'server unit tests',
directory: 'server',
packageName: '@veritas-kanban/server',
requiredScript: 'test',
},
{
id: 'web',
label: 'web unit tests',
directory: 'web',
packageName: '@veritas-kanban/web',
requiredScript: 'test',
},
{
id: 'cli',
label: 'CLI unit tests',
directory: 'cli',
packageName: '@veritas-kanban/cli',
requiredScript: 'test',
},
{
id: 'mcp',
label: 'MCP unit tests',
directory: 'mcp',
packageName: '@veritas-kanban/mcp',
requiredScript: 'test',
},
]);
function readWorkspacePackage(stage) {
const packagePath = path.join(repositoryRoot, stage.directory, 'package.json');
return JSON.parse(readFileSync(packagePath, 'utf8'));
}
export function validateStage(stage, { readPackageJson = readWorkspacePackage } = {}) {
let packageJson;
try {
packageJson = readPackageJson(stage);
} catch (error) {
return `Cannot read ${stage.directory}/package.json: ${error.message}`;
}
if (packageJson.name !== stage.packageName) {
return `${stage.directory}/package.json must declare name ${stage.packageName}`;
}
if (
typeof packageJson.scripts?.[stage.requiredScript] !== 'string' ||
packageJson.scripts[stage.requiredScript].trim() === ''
) {
return `${stage.packageName} must define a non-empty ${stage.requiredScript} script`;
}
return undefined;
}
function executeStage(stage) {
const result = spawnSync(pnpmExecutable, ['--filter', stage.packageName, stage.requiredScript], {
cwd: repositoryRoot,
env: process.env,
stdio: 'inherit',
});
if (result.error) {
console.error(`Could not start ${stage.label}: ${result.error.message}`);
return 1;
}
return result.status ?? 1;
}
export function runWorkspaceUnitTests({
run = executeStage,
validate = validateStage,
log = console.log,
} = {}) {
const results = new Map(UNIT_TEST_STAGES.map((stage) => [stage.id, 'NOT RUN']));
let exitCode = 0;
for (const stage of UNIT_TEST_STAGES) {
log(`\n==> ${stage.label}`);
const validationError = validate(stage);
if (validationError) {
log(`Configuration error: ${validationError}`);
results.set(stage.id, 'FAIL');
exitCode = 1;
break;
}
const status = run(stage);
results.set(stage.id, status === 0 ? 'PASS' : 'FAIL');
if (status !== 0) {
exitCode = status > 0 ? status : 1;
break;
}
}
const workspaceSummary = UNIT_TEST_STAGES.filter((stage) => stage.id !== 'shared-build')
.map((stage) => `${stage.id}: ${results.get(stage.id)}`)
.join(' | ');
log(`\nWorkspace unit-test summary: ${workspaceSummary}`);
return exitCode;
}
if (process.argv[1] && path.resolve(process.argv[1]) === fileURLToPath(import.meta.url)) {
process.exitCode = runWorkspaceUnitTests();
}

View file

@ -0,0 +1,70 @@
import assert from 'node:assert/strict';
import test from 'node:test';
import {
UNIT_TEST_STAGES,
runWorkspaceUnitTests,
validateStage,
} from './run-workspace-unit-tests.mjs';
test('runs the shared prerequisite and every unit-test workspace in order', () => {
const calls = [];
const lines = [];
const status = runWorkspaceUnitTests({
run: (stage) => {
calls.push(stage.id);
return 0;
},
log: (line) => lines.push(line),
});
assert.equal(status, 0);
assert.deepEqual(
calls,
UNIT_TEST_STAGES.map((stage) => stage.id)
);
assert.match(lines.at(-1), /server: PASS.*web: PASS.*cli: PASS.*mcp: PASS/);
});
test('stops after the first failure and reports unexecuted workspaces', () => {
const calls = [];
const lines = [];
const status = runWorkspaceUnitTests({
run: (stage) => {
calls.push(stage.id);
return stage.id === 'web' ? 7 : 0;
},
log: (line) => lines.push(line),
});
assert.equal(status, 7);
assert.deepEqual(calls, ['shared-build', 'server', 'web']);
assert.match(lines.at(-1), /server: PASS.*web: FAIL.*cli: NOT RUN.*mcp: NOT RUN/);
});
test('fails before execution when a workspace test script is missing', () => {
const calls = [];
const lines = [];
const status = runWorkspaceUnitTests({
validate: (stage) =>
validateStage(stage, {
readPackageJson: () => ({
name: stage.packageName,
scripts: { [stage.requiredScript]: stage.id === 'cli' ? '' : 'test-command' },
}),
}),
run: (stage) => {
calls.push(stage.id);
return 0;
},
log: (line) => lines.push(line),
});
assert.equal(status, 1);
assert.deepEqual(calls, ['shared-build', 'server', 'web']);
assert.match(lines.at(-2), /must define a non-empty test script/);
assert.match(lines.at(-1), /server: PASS.*web: PASS.*cli: FAIL.*mcp: NOT RUN/);
});

View file

@ -10,7 +10,7 @@
"start": "node dist/index.js",
"typecheck": "tsc --noEmit",
"lint": "eslint src --ext .ts",
"test": "VERITAS_DISABLE_WATCHERS=1 vitest run",
"test": "VERITAS_DISABLE_WATCHERS=1 vitest run --maxWorkers=4",
"test:watch": "vitest",
"clean": "rm -rf dist",
"reset-password": "tsx src/scripts/reset-password.ts"

View file

@ -95,14 +95,12 @@ describe('StatusHistoryService', () => {
});
it('should calculate duration from previous entry', async () => {
let nowMs = Date.parse('2026-08-23T15:00:00.000Z');
service = new StatusHistoryService({ historyFile, now: () => new Date(nowMs) });
await service.logStatusChange('idle', 'working');
// Wait a small amount for timestamp difference
await new Promise((r) => setTimeout(r, 50));
nowMs += 50;
const second = await service.logStatusChange('working', 'idle');
expect(second.durationMs).toBeDefined();
expect(second.durationMs!).toBeGreaterThan(0);
expect(second.durationMs).toBe(50);
});
it('should include subAgentCount when provided', async () => {

View file

@ -80,10 +80,12 @@ describe('SQLite workflow run execution', () => {
let fixture: TestSqliteDatabase;
let testRoot: string;
let workflowService: WorkflowService;
let runServices: Array<{ dispose(): void }>;
beforeEach(async () => {
fixture = createTestSqliteDatabase();
fixture.database.open();
runServices = [];
testRoot = await fs.mkdtemp(path.join(os.tmpdir(), 'veritas-sqlite-workflow-execution-'));
workflowService = new WorkflowService({
workflowsDir: path.join(testRoot, 'storage', 'workflows'),
@ -95,6 +97,7 @@ describe('SQLite workflow run execution', () => {
afterEach(async () => {
vi.clearAllMocks();
for (const runService of runServices) runService.dispose();
fixture.cleanup();
await fs.rm(testRoot, { recursive: true, force: true });
});
@ -109,6 +112,7 @@ describe('SQLite workflow run execution', () => {
sqliteDatabase: fixture.database,
workflowService,
});
runServices.push(runService);
const counts: Record<string, number> = {};
await workflowService.saveWorkflow(definition);
@ -183,6 +187,7 @@ describe('SQLite workflow run execution', () => {
workflowService,
admission,
});
runServices.push(first, second);
const recovery: RunRecoveryRecord = {
schemaVersion: 'run-recovery/v1',
rootRunId: 'run_root',

View file

@ -96,6 +96,7 @@ describe('WorkflowRunService', () => {
});
afterEach(async () => {
service.dispose();
vi.clearAllMocks();
await fs.rm(tmpDir, { recursive: true, force: true });
});
@ -425,6 +426,17 @@ describe('WorkflowRunService', () => {
expect(mockExecuteStep).toHaveBeenCalledTimes(1);
});
it('clears owned recovery timers before test storage is removed', () => {
service.scheduleWorkflowRecovery('run_timer_cleanup', 'step-1', {
state: 'scheduled',
notBefore: '2999-01-01T00:00:00.000Z',
});
expect(service.scheduledWorkflowRecoveryTimers.size).toBe(1);
service.dispose();
expect(service.scheduledWorkflowRecoveryTimers.size).toBe(0);
});
it('blocks a restarted workflow when a launched recovery cannot be proven terminal', async () => {
mockLoadWorkflow.mockResolvedValue(
makeWorkflow({

View file

@ -151,13 +151,10 @@ describe('WorktreeService transactional lifecycle', () => {
it('waits for a timed-out Git child to close before rejecting the operation', async () => {
const runner = new DefaultWorktreeGitRunner(100, process.execPath, 50);
const startedAt = Date.now();
await expect(
runner.run(root, ['-e', "process.on('SIGTERM', () => {}); setInterval(() => {}, 1000);"])
).rejects.toThrow(/timed out/i);
expect(Date.now() - startedAt).toBeGreaterThanOrEqual(140);
});
it('resolves and persists the exact remote base commit before creating a unique worktree', async () => {

View file

@ -44,6 +44,7 @@ export interface StatusHistoryServiceOptions {
storageType?: 'file' | 'sqlite';
sqliteDatabase?: SqliteDatabase;
sqliteConnectionOptions?: SqliteConnectionOptions;
now?: () => Date;
}
export class StatusHistoryService {
@ -54,8 +55,10 @@ export class StatusHistoryService {
private repository: StatusHistoryRepository | null = null;
private sqliteDatabase: SqliteDatabase | null = null;
private ownsSqliteDatabase = false;
private readonly now: () => Date;
constructor(options: StatusHistoryServiceOptions = {}) {
this.now = options.now ?? (() => new Date());
this.historyFile =
options.historyFile || join(process.cwd(), '.veritas-kanban', 'status-history.json');
const storageType =
@ -139,7 +142,7 @@ export class StatusHistoryService {
await this.initPromise;
const now = new Date();
const now = this.now();
const timestamp = now.toISOString();
// Calculate duration of previous status
@ -151,7 +154,7 @@ export class StatusHistoryService {
}
const entry: StatusHistoryEntry = {
id: `status_${Date.now()}_${Math.random().toString(36).substr(2, 9)}`,
id: `status_${now.getTime()}_${Math.random().toString(36).substr(2, 9)}`,
timestamp,
previousStatus,
newStatus,
@ -209,7 +212,7 @@ export class StatusHistoryService {
return this.repository.getDailySummary(date);
}
const targetDate = date || new Date().toISOString().split('T')[0];
const targetDate = date || this.now().toISOString().split('T')[0];
const startOfDay = new Date(`${targetDate}T00:00:00.000Z`);
const endOfDay = new Date(`${targetDate}T23:59:59.999Z`);
@ -237,7 +240,7 @@ export class StatusHistoryService {
endTime = new Date(nextEntry.timestamp);
} else {
// Last entry - use current time or end of day if analyzing past dates
const now = new Date();
const now = this.now();
endTime = now < endOfDay ? now : endOfDay;
}
@ -277,7 +280,7 @@ export class StatusHistoryService {
if (beforeDay.length > 0) {
// The most recent entry before this day determines the starting status
const lastBeforeDay = beforeDay[0];
const now = new Date();
const now = this.now();
const effectiveEnd = now < endOfDay ? now : endOfDay;
const durationMs = effectiveEnd.getTime() - startOfDay.getTime();
@ -318,7 +321,7 @@ export class StatusHistoryService {
}
const summaries: DailySummary[] = [];
const today = new Date();
const today = this.now();
for (let i = 0; i < 7; i++) {
const date = new Date(today);

View file

@ -64,9 +64,10 @@ const log = createLogger('workflow-run');
/** Default maximum cross-step reroutes per run before exhaustion policy fires (#780) */
const MAX_REROUTES_DEFAULT = 10;
type WorkflowRecoveryTimer = ReturnType<typeof setTimeout>;
const scheduledWorkflowRecoveries = new Map<
string,
{ stepId: string; timer: ReturnType<typeof setTimeout> }
{ stepId: string; timer: WorkflowRecoveryTimer; ownerTimers: Set<WorkflowRecoveryTimer> }
>();
const RUN_ID_PATTERN = /^run_\d{10,}_[a-zA-Z0-9_-]{6,}$/;
const WORKFLOW_ADMISSION_ID_PREFIX = 'workflow';
@ -113,6 +114,7 @@ class WorkflowRunChangedError extends ConflictError {
}
export class WorkflowRunService {
private readonly scheduledWorkflowRecoveryTimers = new Set<WorkflowRecoveryTimer>();
private runsDir: string;
private workflowService: ReturnType<typeof getWorkflowService>;
private stepExecutor: WorkflowStepExecutor;
@ -2220,20 +2222,27 @@ export class WorkflowRunService {
const delay = Math.max(0, Math.min(2_147_483_647, notBefore - Date.now()));
const timer = setTimeout(() => {
const scheduled = scheduledWorkflowRecoveries.get(runId);
if (!scheduled || scheduled.stepId !== stepId) return;
if (!scheduled || scheduled.stepId !== stepId || scheduled.timer !== timer) return;
scheduledWorkflowRecoveries.delete(runId);
scheduled.ownerTimers.delete(scheduled.timer);
void this.resumeScheduledWorkflowRecovery(runId, stepId).catch((error) => {
log.error({ err: error, runId, stepId }, 'Scheduled workflow recovery failed');
});
}, delay);
timer.unref?.();
scheduledWorkflowRecoveries.set(runId, { stepId, timer });
this.scheduledWorkflowRecoveryTimers.add(timer);
scheduledWorkflowRecoveries.set(runId, {
stepId,
timer,
ownerTimers: this.scheduledWorkflowRecoveryTimers,
});
}
private clearScheduledWorkflowRecovery(runId: string, expectedStepId?: string): void {
const scheduled = scheduledWorkflowRecoveries.get(runId);
if (!scheduled || (expectedStepId && scheduled.stepId !== expectedStepId)) return;
clearTimeout(scheduled.timer);
scheduled.ownerTimers.delete(scheduled.timer);
scheduledWorkflowRecoveries.delete(runId);
}
@ -2845,6 +2854,15 @@ export class WorkflowRunService {
}
dispose(): void {
for (const timer of this.scheduledWorkflowRecoveryTimers) {
clearTimeout(timer);
}
for (const [runId, scheduled] of scheduledWorkflowRecoveries) {
if (scheduled.ownerTimers === this.scheduledWorkflowRecoveryTimers) {
scheduledWorkflowRecoveries.delete(runId);
}
}
this.scheduledWorkflowRecoveryTimers.clear();
if (this.ownsSqliteDatabase) {
this.sqliteDatabase?.close();
}

View file

@ -9,7 +9,7 @@
"preview": "vite preview",
"typecheck": "tsc --noEmit",
"lint": "eslint src --ext .ts,.tsx",
"test": "vitest run --testTimeout 15000",
"test": "vitest run --testTimeout 15000 --maxWorkers=4",
"test:watch": "vitest --testTimeout 15000",
"clean": "rm -rf dist"
},