diff --git a/coverage-service/lib/src/prompts/test-generation.ts b/coverage-service/lib/src/prompts/test-generation.ts index 62897ab..acf1b3e 100644 --- a/coverage-service/lib/src/prompts/test-generation.ts +++ b/coverage-service/lib/src/prompts/test-generation.ts @@ -168,20 +168,36 @@ function buildJavaScriptPrompt(ctx: TestGenerationContext, symbols: string): str const testFile = outputTestPath(ctx); const importPath = inferSourceImportPath(testFile, ctx.file); const importExt = sourceFileExtension(ctx.file); - const repoHasJest = ctx.repoPackages.some((p) => normalizePackageName(p) === 'jest'); - const repoHasVitest = ctx.repoPackages.some((p) => normalizePackageName(p) === 'vitest'); - const useNodeTest = !repoHasJest && !repoHasVitest; - - const frameworkSection = useNodeTest - ? `Use Node.js built-in test runner ONLY: + const frameworkSection = `Use Node.js built-in test runner ONLY: import test from 'node:test'; import assert from 'node:assert/strict'; -Do NOT use @jest/globals, jest, vitest, or mocha unless they appear in Repository Packages.` - : ctx.framework.includes('vitest') - ? 'Use Vitest APIs matching existing tests in the repo.' - : 'Use Jest APIs matching existing tests in the repo.'; +Do NOT use @jest/globals, jest, vitest, mocha, describe, it, expect, or jest.fn. The validation command is node --test.`; const hasExports = (ctx.exportedSymbols?.length ?? 0) > 0; + const isReactSource = /\.(jsx|tsx)$/i.test(ctx.file); + const reactSection = + isReactSource + ? `## React / JSX (mandatory for ${ctx.file}) +- Prefer simple tests that do NOT render the component. First test exported helpers, constants, style objects, prop-building functions, module exports, or callback behavior that can be called directly. +- For complex Next/MUI/browser components, a smoke test that imports the module and asserts exported symbols are functions/objects is better than a brittle render test. +- Avoid @testing-library/react and jsdom unless the target behavior cannot be tested through pure/static exports. +- If you truly must render, node:test has no JSDOM — set up jsdom before any import that uses the DOM: + import { JSDOM } from 'jsdom'; + const { window } = new JSDOM('', { url: 'http://localhost' }); + globalThis.window = window; + globalThis.document = window.document; + Object.defineProperty(globalThis, 'navigator', { value: window.navigator, configurable: true }); + globalThis.HTMLElement = window.HTMLElement; + globalThis.Node = window.Node; +- Place jsdom setup immediately after the test-deps line and before all other imports. +- Use @testing-library/react only if it appears in Repository Packages and the component has simple dependencies; otherwise use pure/static assertions. +- Mock firebase, routers, fetch, and service modules — never call real backends. +- Avoid module mocking when possible. Do not import modules that require Next runtime, auth providers, real stores, CSS-heavy widgets, or localStorage unless the test sets up those globals. +- For rendered text, use flexible matchers (getByText(/Team:\\s*-/) or regex) — JSX whitespace varies; never assert exact strings with ambiguous spacing. +- Wrap state updates in act() from 'react' when needed. + +` + : ''; return `You are writing JavaScript unit tests for ONE production file. Tests must pass on first run with no manual fixes. @@ -219,7 +235,7 @@ ${symbols || '(none detected)'} ## Test runner ${frameworkSection} -## Imports (critical) +${reactSection}## Imports (critical) ${hasExports ? `- Import ONLY from exported symbols listed above via: '${importPath}' - Always include the ${importExt} extension in relative ESM imports. @@ -243,13 +259,15 @@ Example layout (exported module): import { loadConfig } from '../../src/config/loadConfig.js'; ## What to test -1. Pure functions: validators, branching, error cases, defaults. -2. Exported classes/methods with stubbed dependencies. -3. CLI/entry-point files (no exports): subprocess tests with mocked env and stubbed child processes. +1. Pure/static logic first: validators, formatters, constants, style object keys/values, enum/type runtime exports, barrel re-exports. +2. Exported classes/methods with plain object fakes. +3. React/Next files: prefer import/export smoke tests and direct helper tests over rendering. +4. CLI/entry-point files (no exports): subprocess tests with mocked env and stubbed child processes. ## Mocking (mandatory for I/O) - Never call real network, filesystem, databases, or external services. - Stub fetch, fs, and SDK clients with plain objects or node:test mock.method when available. +- Do NOT use mock.module or test.mock.module; this Node runtime does not support it. Prefer testing exported pure helpers, passing props/callback fakes, or mocking globals like fetch/localStorage directly. - For MCP/HTTP clients, mock at the boundary the production code uses. ## ESM @@ -259,11 +277,11 @@ Example layout (exported module): ## Test deps header (only if needed) If tests need npm packages NOT in Repository Packages, first line: // test-deps: package-name -Examples: sinon, nock. Do NOT list jest if using node:test. +Examples: sinon, nock. Avoid new packages. Do NOT list jest, vitest, @testing-library/react, or jsdom unless absolutely required and absent from Repository Packages. Never list Node built-ins (node:test, node:assert, node:fs, fs, path, etc.) — they are not npm packages. ## Self-check before returning -- [ ] 2–4 tests using ${useNodeTest ? 'node:test + node:assert' : ctx.framework} +- [ ] 2–4 tests using node:test + node:assert - [ ] Import path is '${importPath}' (or equivalent correct relative path) - [ ] Every imported symbol exists in source exports - [ ] All external I/O mocked @@ -272,10 +290,6 @@ Never list Node built-ins (node:test, node:assert, node:fs, fs, path, etc.) — Return only the complete test file.`; } -function normalizePackageName(name: string): string { - return name.trim().toLowerCase().replace(/\[.*\]/, '').split(/[<>=!;]/)[0].trim(); -} - function buildRepairSection(ctx: TestGenerationContext): string { if (!ctx.failureLogs) return ''; @@ -294,6 +308,12 @@ 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. +- Prefer simplifying the test over adding mocks: replace brittle render tests with pure/static assertions, exported helper tests, or import/export smoke tests. +- If failure is "document is not defined", add jsdom setup before all other imports (see React / JSX section). +- If failure mentions localStorage or "opaque origins", create JSDOM with { url: 'http://localhost' } and stub globalThis.localStorage, or avoid rendering/importing the browser-dependent path. +- If failure mentions ".css" unknown extension, avoid importing the CSS-heavy component path unless necessary; prefer testing a pure export or style/helper module. +- If failure says "mock.module is not a function" or "test.mock.module is not a function", remove module mocking and test via exported helpers, props, callback fakes, or globals instead. +- If TestingLibraryElementError mentions whitespace/normalization, use regex matchers (e.g. getByText(/Team:\\s*-/)) instead of exact strings. - Return the complete corrected test file only.`; } @@ -324,7 +344,8 @@ export function buildTestGenerationSystemPrompt(language: string): string { 'Use correct relative ESM import paths with the source file extension (.js, .jsx, .ts, .tsx). ' + 'Only import symbols listed as exported; never import private functions. ' + 'For CLI files with no exports, test via child_process.spawnSync instead of importing. ' + - 'Mock all external I/O. Return only runnable test file code. ' + + 'Mock all external I/O. Prefer pure/static assertions and import/export smoke tests over rendering React components. Return only runnable test file code. ' + + 'For React/JSX files with node:test, avoid jsdom/rendering unless needed; if rendering, set up jsdom before DOM imports and use flexible text matchers. ' + 'Declare extra npm packages on the first line as // test-deps: pkg1, pkg2' ); } diff --git a/coverage-service/worker/src/lib/js-coverage.ts b/coverage-service/worker/src/lib/js-coverage.ts index 898572c..8ddf692 100644 --- a/coverage-service/worker/src/lib/js-coverage.ts +++ b/coverage-service/worker/src/lib/js-coverage.ts @@ -3,6 +3,25 @@ import { join, relative } from 'path'; import { sourceFileExtension, type ChangedFile, type RepositoryProvider } from '@openreview/coverage-lib'; +import { needsJsTranspileLoader } from './repo-packages.js'; + +const TEST_REGISTER_PATH = '.openreview-test-register.cjs'; +const TEST_LOADER_PATH = '.openreview-test-loader.mjs'; +const TEST_REGISTER_CONTENT = `const noop = (module) => { module.exports = {}; }; +for (const ext of ['.css', '.scss', '.sass', '.less', '.svg', '.png', '.jpg', '.jpeg', '.gif', '.webp']) { + require.extensions[ext] = noop; +} +`; +const TEST_LOADER_CONTENT = `const ASSET_EXTENSIONS = /\\.(css|scss|sass|less|svg|png|jpg|jpeg|gif|webp)(\\?.*)?$/; + +export async function load(url, context, nextLoad) { + if (ASSET_EXTENSIONS.test(url)) { + return { format: 'module', source: 'export default {};', shortCircuit: true }; + } + return nextLoad(url, context); +} +`; + function shellQuote(value: string): string { return `'${value.replace(/'/g, `'"'"'`)}'`; } @@ -33,36 +52,44 @@ function isEsmRepo(repoDir: string): boolean { } } -function hasNpmTestScript(repoDir: string): boolean { - const pkgPath = join(repoDir, 'package.json'); - if (!existsSync(pkgPath)) return false; - try { - const pkg = JSON.parse(readFileSync(pkgPath, 'utf-8')) as { - scripts?: Record; - }; - return Boolean(pkg.scripts?.test?.trim()); - } catch { - return false; - } +function isCliEntryPoint(sourcePath: string): boolean { + const base = sourcePath.split('/').pop() ?? sourcePath; + return base === 'cli.js' || base === 'main.js' || base === 'index.js'; } -function deriveCoverageInclude(sourcePaths: string[]): string { - const includes = new Set(); - for (const p of sourcePaths) { - const normalized = normalizePath(p); - const parts = normalized.split('/'); - if (parts.length > 1) { - includes.add(`${parts[0]}/**`); - } else { - includes.add(normalized); - } - } - return [...includes].join(','); +function needsRuntimeRegister(paths: string[]): boolean { + return needsJsTranspileLoader(paths); } -function isCliEntryPoint(sourcePath: string): boolean { - const base = sourcePath.split('/').pop() ?? sourcePath; - return base === 'cli.js' || base === 'main.js' || base === 'index.js'; +function runtimeRegisterSetupCommand(paths: string[]): string | null { + if (!needsRuntimeRegister(paths)) return null; + const body = [ + `require('fs').writeFileSync(${JSON.stringify(TEST_REGISTER_PATH)}, ${JSON.stringify(TEST_REGISTER_CONTENT)})`, + `require('fs').writeFileSync(${JSON.stringify(TEST_LOADER_PATH)}, ${JSON.stringify(TEST_LOADER_CONTENT)})`, + ].join(';'); + return `node -e ${shellQuote(body)}`; +} + +function nodeLoaderPrefix(paths: string[]): string { + if (!needsJsTranspileLoader(paths)) return ''; + const loaderRegister = + `data:text/javascript,import { register } from "node:module"; ` + + `import { pathToFileURL } from "node:url"; ` + + `register("./${TEST_LOADER_PATH}", pathToFileURL("./"));`; + return [ + '--require', + shellQuote(`./${TEST_REGISTER_PATH}`), + '--import', + shellQuote(loaderRegister), + '--import', + 'tsx', + '', + ].join(' '); +} + +function withRuntimeRegister(paths: string[], command: string): string { + const setup = runtimeRegisterSetupCommand(paths); + return setup ? `${setup} && ${command}` : command; } function buildSourceImportCommand(repoDir: string, sourcePaths: string[]): string { @@ -77,16 +104,17 @@ function buildSourceImportCommand(repoDir: string, sourcePaths: string[]): strin } const paths = targets.map((p) => (p.startsWith('./') ? p : `./${p}`)); + const loader = nodeLoaderPrefix(targets); if (isEsmRepo(repoDir)) { const body = paths .map((p) => `try{await import(${shellQuote(p)})}catch(e){}`) .join(';'); - return `node --input-type=module -e ${shellQuote(body)}`; + return withRuntimeRegister(targets, `node ${loader}--input-type=module -e ${shellQuote(body)}`); } const body = paths.map((p) => `try{require(${shellQuote(p)})}catch(e){}`).join(';'); - return `node -e ${shellQuote(body)}`; + return withRuntimeRegister(targets, `node ${loader}-e ${shellQuote(body)}`); } function buildRunTarget( @@ -95,21 +123,25 @@ function buildRunTarget( testPaths: string[], ): string { if (testPaths.length > 0) { - return `node --test ${testPaths.map(shellQuote).join(' ')}`; - } - - if (hasNpmTestScript(repoDir)) { - return 'npm test -- --passWithNoTests'; + const runtimePaths = [...testPaths, ...sourcePaths]; + const loader = nodeLoaderPrefix(runtimePaths); + return withRuntimeRegister( + runtimePaths, + `node ${loader}--test ${testPaths.map(shellQuote).join(' ')}`, + ); } + // No PR-specific tests were found. Avoid running the repository-wide test + // suite here; import only changed source files so diff coverage stays scoped + // to the PR files under analysis. return buildSourceImportCommand(repoDir, sourcePaths); } function coverageToolPrefix(sourcePaths: string[]): string { - const include = + const includes = sourcePaths.length > 0 - ? `--include='${deriveCoverageInclude(sourcePaths)}'` - : "--include='**/*'"; + ? sourcePaths.map((p) => `--include=${shellQuote(normalizePath(p))}`) + : ["--include='**/*'"]; // c8 uses V8 native coverage and works with ESM; nyc/istanbul often reports 0% for "type":"module" repos. return [ @@ -117,7 +149,7 @@ function coverageToolPrefix(sourcePaths: string[]): string { '--reporter=cobertura', '--reporter=text', '--all', - include, + ...includes, "--exclude='tests/**'", "--exclude='**/*.test.js'", "--exclude='**/*.spec.js'", @@ -131,13 +163,21 @@ function coverageToolPrefix(sourcePaths: string[]): string { ].join(' '); } +/** c8 only collects V8 coverage from its direct child process. */ +function wrapForCoverageInstrumentedChild(command: string): string { + if (!command.includes(' && ')) return command; + return `sh -c ${shellQuote(command)}`; +} + export function buildJsCoverageCommand( sourcePaths: string[], testPaths: string[], repoDir?: string, ): string { const dir = repoDir ?? '.'; - const runTarget = buildRunTarget(dir, sourcePaths, testPaths); + const runTarget = wrapForCoverageInstrumentedChild( + buildRunTarget(dir, sourcePaths, testPaths), + ); return `${coverageToolPrefix(sourcePaths)} ${runTarget}`; } @@ -224,6 +264,25 @@ function testFileMatchesSource(testPath: string, sourcePath: string): boolean { ); } +function isNodeTestCompatible(repoDir: string, testPath: string): boolean { + let content: string; + try { + content = readFileSync(join(repoDir, testPath), 'utf-8'); + } catch { + return false; + } + + if (/\bfrom\s+['"](?:node:)?test['"]/.test(content) || /require\(\s*['"](?:node:)?test['"]\s*\)/.test(content)) { + return true; + } + + if (/\bfrom\s+['"](?:vitest|@jest\/globals)['"]/.test(content)) { + return false; + } + + return !/\b(?:describe|it|expect|beforeEach|afterEach)\s*\(/.test(content); +} + export async function collectJsTestPaths( repoDir: string, changedFiles: ChangedFile[], @@ -235,7 +294,10 @@ export async function collectJsTestPaths( for (const file of changedFiles) { if (!/\.(js|jsx|ts|tsx)$/.test(file.path) || file.status === 'deleted') continue; if (isTestFile(file.path)) { - testPaths.add(normalizePath(file.path)); + const normalized = normalizePath(file.path); + if (isNodeTestCompatible(repoDir, normalized)) { + testPaths.add(normalized); + } } } @@ -255,7 +317,7 @@ export async function collectJsTestPaths( } for (const candidate of guessTestFileNames(sourcePath)) { - if (existsSync(join(repoDir, candidate))) { + if (existsSync(join(repoDir, candidate)) && isNodeTestCompatible(repoDir, candidate)) { testPaths.add(normalizePath(candidate)); } } @@ -267,7 +329,10 @@ export async function collectJsTestPaths( walkJsTestFiles(join(repoDir, testsDir), repoDir, allTests); } for (const testPath of allTests) { - if (sourcePaths.some((s) => testFileMatchesSource(testPath, s))) { + if ( + isNodeTestCompatible(repoDir, testPath) && + sourcePaths.some((s) => testFileMatchesSource(testPath, s)) + ) { testPaths.add(testPath); } } diff --git a/coverage-service/worker/src/lib/repo-packages.ts b/coverage-service/worker/src/lib/repo-packages.ts index a5a6266..e1224ff 100644 --- a/coverage-service/worker/src/lib/repo-packages.ts +++ b/coverage-service/worker/src/lib/repo-packages.ts @@ -204,10 +204,54 @@ export interface ParsedGeneratedTest { } /** Strip optional LLM-declared test deps and return cleaned test content. */ +const JSDOM_SETUP = `import { JSDOM } from 'jsdom'; + +const { window } = new JSDOM('', { + url: 'http://localhost', +}); +globalThis.window = window; +globalThis.document = window.document; +Object.defineProperty(globalThis, 'navigator', { + value: window.navigator, + configurable: true, +}); +globalThis.HTMLElement = window.HTMLElement; +globalThis.Node = window.Node; +`; + +export function needsJsdomEnvironment(content: string): boolean { + return ( + /@testing-library\/react/.test(content) || + /\bfrom\s+['"]react-dom\/test-utils['"]/.test(content) || + (/\brender\s*\(/.test(content) && /\bfrom\s+['"]react['"]/.test(content)) + ); +} + +function hasJsdomSetup(content: string): boolean { + return ( + /\bfrom\s+['"]jsdom['"]/.test(content) || + /globalThis\.document\s*=/.test(content) || + /global\.document\s*=/.test(content) + ); +} + +/** Inject jsdom globals before imports when React DOM tests omit setup. */ +export function prepareGeneratedJsTestContent(content: string): string { + if (!needsJsdomEnvironment(content) || hasJsdomSetup(content)) { + return content; + } + + const depsMatch = content.match(/^\s*\/\/\s*test-deps:\s*.+\s*\n/); + const depsHeader = depsMatch?.[0] ?? ''; + const rest = depsMatch ? content.slice(depsMatch[0].length) : content; + + return `${depsHeader}${JSDOM_SETUP}\n${rest}`; +} + export function parseGeneratedTestContent(content: string): ParsedGeneratedTest { const match = content.match(/^\s*(?:\/\/|#)\s*test-deps:\s*(.+)\s*$/m); if (!match) { - return { content: content.trim(), declaredDeps: [] }; + return { content: prepareGeneratedJsTestContent(content.trim()), declaredDeps: [] }; } const declaredDeps = match[1] @@ -218,7 +262,10 @@ export function parseGeneratedTestContent(content: string): ParsedGeneratedTest const cleaned = content .replace(/^\s*(?:\/\/|#)\s*test-deps:\s*.+\s*\n?/m, '') .trim(); - return { content: cleaned, declaredDeps }; + return { + content: prepareGeneratedJsTestContent(cleaned), + declaredDeps, + }; } function extractPythonImports(content: string): string[] { @@ -245,6 +292,7 @@ function extractJsImportSpecs(content: string): string[] { function importSpecToNpmPackage(spec: string): string | null { if (spec.startsWith('.') || spec.startsWith('/')) return null; + if (spec.startsWith('@/') || spec.startsWith('#') || spec.startsWith('~')) return null; if (isNonNpmPackage(spec)) return null; const mapped = JS_IMPORT_TO_PACKAGE[spec]; @@ -303,6 +351,10 @@ function inferJsTestPackages(testContents: string[]): string[] { packages.add('sinon'); } + if (needsJsdomEnvironment(combined)) { + packages.add('jsdom'); + } + for (const content of testContents) { for (const spec of extractJsImportSpecs(content)) { const pkg = importSpecToNpmPackage(spec); @@ -319,18 +371,36 @@ function inferAutoTestPackages(testContents: string[], ecosystem: 'python' | 'ja : inferPythonTestPackages(testContents); } +/** tsx transpiles .jsx/.tsx/.ts for node --test and dynamic imports. */ +export function needsJsTranspileLoader(paths: string[]): boolean { + return paths.some((p) => /\.(jsx|tsx|ts)$/i.test(p.trim())); +} + +function inferJsTranspilePackages(filePaths: string[]): string[] { + return needsJsTranspileLoader(filePaths) ? ['tsx'] : []; +} + export function collectTestRunDependencies( testContents: string[], declaredDeps: string[], repoPackages: string[], ecosystem: 'python' | 'javascript' = 'python', + filePaths: string[] = [], ): string[] { const repoNormalized = new Set( repoPackages.map((pkg) => normalizePackageName(pkg)), ); const deps = new Set(); - for (const dep of [...declaredDeps, ...inferAutoTestPackages(testContents, ecosystem)]) { + const autoDeps = + ecosystem === 'javascript' + ? [ + ...inferAutoTestPackages(testContents, ecosystem), + ...inferJsTranspilePackages(filePaths), + ] + : inferAutoTestPackages(testContents, ecosystem); + + for (const dep of [...declaredDeps, ...autoDeps]) { if (isNonNpmPackage(dep)) continue; const normalized = normalizePackageName(dep); if (!normalized || repoNormalized.has(normalized)) continue; @@ -349,5 +419,5 @@ export function buildPipInstallCommand(packages: string[]): string | null { export function buildNpmInstallCommand(packages: string[]): string | null { if (packages.length === 0) return null; const quoted = packages.map((pkg) => `'${pkg.replace(/'/g, `'"'"'`)}'`); - return `npm install -D ${quoted.join(' ')}`; + return `npm install --no-save --legacy-peer-deps ${quoted.join(' ')}`; } diff --git a/coverage-service/worker/src/lib/repo-setup.ts b/coverage-service/worker/src/lib/repo-setup.ts index ca3f490..f8dad57 100644 --- a/coverage-service/worker/src/lib/repo-setup.ts +++ b/coverage-service/worker/src/lib/repo-setup.ts @@ -44,7 +44,7 @@ export function detectRepoSetup(repoDir: string): RepoSetup { const hasPackageJson = existsSync(join(repoDir, 'package.json')); const coverageDeps = 'coverage pytest pytest-asyncio'; - const jsCoverageDeps = 'c8 check-code-coverage'; + const jsCoverageDeps = 'c8 check-code-coverage tsx'; const noVenv: RepoSetup = { isPython: false, isJavaScript: false, @@ -98,7 +98,7 @@ export function detectRepoSetup(repoDir: string): RepoSetup { } const jsInstallBase = hasPackageLock ? 'npm ci' : 'npm install'; - const jsInstallCommand = `${jsInstallBase} && npm install -D ${jsCoverageDeps}`; + const jsInstallCommand = `${jsInstallBase} && npm install --no-save --legacy-peer-deps ${jsCoverageDeps}`; if (hasPnpmLock) { return { diff --git a/coverage-service/worker/src/processors/pr-analysis.processor.ts b/coverage-service/worker/src/processors/pr-analysis.processor.ts index 3bf70f8..2eb06ff 100644 --- a/coverage-service/worker/src/processors/pr-analysis.processor.ts +++ b/coverage-service/worker/src/processors/pr-analysis.processor.ts @@ -866,6 +866,7 @@ export class PrAnalysisProcessor { params.declaredTestDeps, params.repoPackages, ecosystem, + params.generatedTests.flatMap((t) => [t.filePath, t.targetFile]), ); const extraInstall = params.repoSetup.isJavaScript ? buildNpmInstallCommand(testRunDeps)