From c813948a33054810ae4d64558d360be0f32c6e66 Mon Sep 17 00:00:00 2001 From: Kenil Shah Date: Mon, 22 Jun 2026 20:34:39 +0530 Subject: [PATCH] Fix unit tests generation --- .../lib/src/prompts/test-generation.ts | 16 ++++++- .../src/processors/pr-analysis.processor.ts | 18 +++++-- .../jobs/processors/coverage-analysis.test.ts | 47 +++++++++++++++++++ .../src/jobs/processors/coverage-analysis.ts | 45 ++++++++++++------ 4 files changed, 106 insertions(+), 20 deletions(-) diff --git a/coverage-service/lib/src/prompts/test-generation.ts b/coverage-service/lib/src/prompts/test-generation.ts index 62897ab..cdd8ac3 100644 --- a/coverage-service/lib/src/prompts/test-generation.ts +++ b/coverage-service/lib/src/prompts/test-generation.ts @@ -182,6 +182,18 @@ Do NOT use @jest/globals, jest, vitest, or mocha unless they appear in Repositor : 'Use Jest APIs matching existing tests in the repo.'; const hasExports = (ctx.exportedSymbols?.length ?? 0) > 0; + const isTypeScript = ctx.language === 'typescript'; + + const typeScriptSection = isTypeScript + ? ` +## TypeScript (critical) +- NEVER read \`.ts\` source with fs.readFileSync and eval/new Function — TypeScript syntax fails at runtime. +- Use the repo test runner from Repository Packages (vitest, jest, etc.) with normal imports of the module under test. +- Mock all external I/O: database (Sequelize/db), Redis, HTTP, filesystem, dynamic import() side effects. +- Prefer testing pure exported functions with vi.mock/jest.mock for dependencies — match patterns in Existing Tests. +- Do not import paths that only exist after compilation unless the repo already uses tsx/ts-jest that way. +` + : ''; return `You are writing JavaScript unit tests for ONE production file. Tests must pass on first run with no manual fixes. @@ -215,6 +227,7 @@ ${ctx.uncoveredLines} All Symbols in File: ${symbols || '(none detected)'} +${typeScriptSection} ## Test runner ${frameworkSection} @@ -282,7 +295,7 @@ function buildRepairSection(ctx: TestGenerationContext): string { return ` ## Repair (attempt ${ctx.attemptNumber ?? 2}) -The previous generated test failed when executed. Fix the test file so it compiles, runs, and passes. +The previous generated test failed when executed. **Fix the test file so it compiles, runs, and passes** — failed tests are never included in the PR. Failure output: ${ctx.failureLogs} @@ -294,6 +307,7 @@ Requirements: - Fix syntax, import paths, mocks, and assertions only — do not change production code. - Reuse existing fixtures/mocks from Existing Tests when possible. - Do not add new npm/pip dependencies unless declared via test-deps header. +- For TypeScript: never use fs.readFileSync + new Function on .ts source; use proper imports and mocks. - Return the complete corrected test file only.`; } diff --git a/coverage-service/worker/src/processors/pr-analysis.processor.ts b/coverage-service/worker/src/processors/pr-analysis.processor.ts index 3bf70f8..c184ef3 100644 --- a/coverage-service/worker/src/processors/pr-analysis.processor.ts +++ b/coverage-service/worker/src/processors/pr-analysis.processor.ts @@ -429,13 +429,23 @@ export class PrAnalysisProcessor { totalGenerationAttempts += outcome.attempts; - if (outcome.test) { + if (outcome.test && outcome.passed) { generatedTests.push(outcome.test); generatedTestResults.push({ filePath: outcome.test.filePath, - passed: outcome.passed, + passed: true, }); declaredTestDeps.push(...outcome.declaredDeps); + } else if (outcome.test && !outcome.passed) { + await this.log( + data.prRunId, + 'warn', + `Excluding ${outcome.test.filePath} from PR — test did not pass after ${outcome.attempts} repair attempt(s)`, + ); + generatedTestResults.push({ + filePath: outcome.test.filePath, + passed: false, + }); } } catch (err) { const msg = `Test generation failed for ${file.path}: ${(err as Error).message}`; @@ -923,7 +933,6 @@ export class PrAnalysisProcessor { }> { let lastFailureLogs = ''; let previousContent = ''; - let lastTest: GeneratedTest | null = null; const declaredDeps: string[] = []; for (let attempt = 1; attempt <= this.maxGenerationAttempts; attempt++) { @@ -952,7 +961,6 @@ export class PrAnalysisProcessor { generated.content = parsed.content; declaredDeps.push(...parsed.declaredDeps); previousContent = generated.content; - lastTest = generated; const testDir = join( params.runDir, @@ -1068,7 +1076,7 @@ export class PrAnalysisProcessor { } return { - test: lastTest, + test: null, passed: false, attempts: this.maxGenerationAttempts, declaredDeps, diff --git a/service/src/jobs/processors/coverage-analysis.test.ts b/service/src/jobs/processors/coverage-analysis.test.ts index 656c894..71c3518 100644 --- a/service/src/jobs/processors/coverage-analysis.test.ts +++ b/service/src/jobs/processors/coverage-analysis.test.ts @@ -307,6 +307,53 @@ describe('processCoverageAnalysis', () => { expect(prAuthor.openOrUpdatePR).not.toHaveBeenCalled(); }); + it('does not open a stacked PR when generated tests failed validation', async () => { + makeRuntime(); + + const coverageClient = { + findOrCreateRepository: vi.fn(async () => baseRepo), + triggerAnalysis: vi.fn(async () => ({ prRunId: 'run-1', status: 'enqueued' })), + waitForPrRun: vi.fn( + async (): Promise => ({ + ...baseRun, + generatedTestFiles: [ + { + id: 't1', + filePath: 'tests/match.service.test.ts', + targetFile: 'src/match.service.ts', + passed: false, + fileContent: 'broken test content', + }, + ], + }), + ), + }; + const prAuthor = { + branchExists: vi.fn(), + commitFiles: vi.fn(), + openOrUpdatePR: vi.fn(), + }; + + await processCoverageAnalysis(job, { + auth, + logger: makeLogger(), + cfg, + coverageClientFactory: () => + coverageClient as unknown as Pick< + CoverageServiceClient, + 'findOrCreateRepository' | 'triggerAnalysis' | 'waitForPrRun' + >, + prAuthorFactory: () => + prAuthor as unknown as Pick< + PRAuthor, + 'branchExists' | 'commitFiles' | 'openOrUpdatePR' + >, + }); + + expect(prAuthor.commitFiles).not.toHaveBeenCalled(); + expect(prAuthor.openOrUpdatePR).not.toHaveBeenCalled(); + }); + it('throws when the coverage run terminates with FAILED', async () => { makeRuntime(); const coverageClient = { diff --git a/service/src/jobs/processors/coverage-analysis.ts b/service/src/jobs/processors/coverage-analysis.ts index 833fe23..d59fa19 100644 --- a/service/src/jobs/processors/coverage-analysis.ts +++ b/service/src/jobs/processors/coverage-analysis.ts @@ -150,17 +150,32 @@ export async function processCoverageAnalysis( let testPrUrl: string | null = null; let testPrFileCount = 0; - if (run.status === 'COMPLETED' && run.generatedTestFiles.length > 0) { - const { url, count } = await openStackedTestPR({ - job, - run, - client, - cfg: deps.cfg, - log, - prAuthorFactory: deps.prAuthorFactory, - }); - testPrUrl = url; - testPrFileCount = count; + if (run.status === 'COMPLETED') { + const passingFiles = run.generatedTestFiles.filter( + (t) => t.passed === true && t.fileContent.length > 0, + ); + + if (passingFiles.length > 0) { + const { url, count } = await openStackedTestPR({ + job, + run, + files: passingFiles, + client, + cfg: deps.cfg, + log, + prAuthorFactory: deps.prAuthorFactory, + }); + testPrUrl = url; + testPrFileCount = count; + } else if (run.generatedTestFiles.length > 0) { + log.warn( + { + generated: run.generatedTestFiles.length, + passed: 0, + }, + 'generated tests did not pass validation; skipping stacked PR', + ); + } } // ------------------------------------------------------------------ // @@ -187,6 +202,7 @@ export async function processCoverageAnalysis( async function openStackedTestPR(args: { job: CoverageAnalysisJob; run: PrRun; + files: PrRun['generatedTestFiles']; client: GitHubClient; cfg: ServiceConfig; log: Logger; @@ -194,9 +210,10 @@ async function openStackedTestPR(args: { }): Promise<{ url: string | null; count: number }> { const { job, run, client, cfg, log } = args; - const files = run.generatedTestFiles - .filter((t) => t.fileContent.length > 0) - .map((t) => ({ path: t.filePath, content: t.fileContent })); + const files = args.files.map((t) => ({ + path: t.filePath, + content: t.fileContent, + })); if (files.length === 0) { log.warn('coverage service reported tests with empty content; skipping PR');