Skip to content

fix: update diff coverage logic #19

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
NiharDoshi99 merged 1 commit into from
Jun 23, 2026
Merged

fix: update diff coverage logic #19

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 42 additions & 21 deletions coverage-service/lib/src/prompts/test-generation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { inferJavaScriptTestFilePath, inferSourceImportPath, inferTestFilePath, sourceFileExtension } from '../test-paths';

Check failure on line 1 in coverage-service/lib/src/prompts/test-generation.ts

View workflow job for this annotation

GitHub Actions / lint-typecheck-test (22) 

'inferJavaScriptTestFilePath' is defined but never used. Allowed unused vars must match /^_/u

Check failure on line 1 in coverage-service/lib/src/prompts/test-generation.ts

View workflow job for this annotation

GitHub Actions / lint-typecheck-test (20) 

'inferJavaScriptTestFilePath' is defined but never used. Allowed unused vars must match /^_/u
import type { TestGenerationContext } from '../types';

function formatRepoPackages(packages: string[]): string {
Expand Down Expand Up @@ -168,20 +168,36 @@
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('<!DOCTYPE html><html><body></body></html>', { 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.

Expand Down Expand Up @@ -219,7 +235,7 @@
## 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.
Expand All @@ -243,13 +259,15 @@
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
Expand All @@ -259,11 +277,11 @@
## 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
Expand All @@ -272,10 +290,6 @@
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 '';

Expand All @@ -294,6 +308,12 @@
- 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.`;
}

Expand Down Expand Up @@ -324,7 +344,8 @@
'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'
);
}
Expand Down
147 changes: 106 additions & 41 deletions coverage-service/worker/src/lib/js-coverage.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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, `'"'"'`)}'`;
}
Expand Down Expand Up @@ -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<string, string>;
};
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<string>();
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 {
Expand All @@ -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(
Expand All @@ -95,29 +123,33 @@ 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 [
'npx c8',
'--reporter=cobertura',
'--reporter=text',
'--all',
include,
...includes,
"--exclude='tests/**'",
"--exclude='**/*.test.js'",
"--exclude='**/*.spec.js'",
Expand All @@ -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}`;
}

Expand Down Expand Up @@ -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[],
Expand All @@ -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);
}
}
}

Expand All @@ -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));
}
}
Expand All @@ -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);
}
}
Expand Down
Loading
Loading