mirror of
https://github.com/Ark0N/Codeman.git
synced 2026-09-30 12:39:42 +02:00
fix(build): merge-time fixes for #500
- pre-push hook: skip with a notice when npm is not on PATH (GUI git clients and IDEs often run hooks with a minimal PATH), instead of blocking every push on "npm: not found"; real-push test with a stripped PATH - test/git-hooks.test.ts: pin GIT_CONFIG_NOSYSTEM=1 and GIT_CONFIG_GLOBAL=/dev/null around the resolveGitHooksDir tests, so an exported global or a system core.hooksPath no longer fails them - watch tsconfig.json, .prettierignore and .editorconfig too: typecheck and format:check read them - check:browser-excludes: fail loudly when the vitest list output and the walked test/**/*.test.ts tree share no path (format drift would otherwise pass vacuously) - Reword the PRE_PUSH_MARKER comment: bumping its version would make every installed v1 hook read as foreign and never refresh again. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -63,17 +63,26 @@ function walk(dir) {
|
|||||||
}
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Every `*.test.ts` under `<root>/test` that imports a browser driver, as sorted
|
* Every `*.test.ts` under `<root>/test`, as sorted repo-relative POSIX paths (the form
|
||||||
* repo-relative POSIX paths (the form `vitest list` prints).
|
* `vitest list` prints).
|
||||||
|
*
|
||||||
|
* @param {string} root
|
||||||
|
* @returns {string[]}
|
||||||
|
*/
|
||||||
|
export function findTestFiles(root) {
|
||||||
|
return walk(join(root, 'test'))
|
||||||
|
.map((file) => relative(root, file).split(sep).join('/'))
|
||||||
|
.sort();
|
||||||
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* The subset of {@link findTestFiles} that imports a browser driver.
|
||||||
*
|
*
|
||||||
* @param {string} root
|
* @param {string} root
|
||||||
* @returns {string[]}
|
* @returns {string[]}
|
||||||
*/
|
*/
|
||||||
export function findBrowserTests(root) {
|
export function findBrowserTests(root) {
|
||||||
return walk(join(root, 'test'))
|
return findTestFiles(root).filter((file) => importsBrowserDriver(readFileSync(join(root, file), 'utf8')));
|
||||||
.filter((file) => importsBrowserDriver(readFileSync(file, 'utf8')))
|
|
||||||
.map((file) => relative(root, file).split(sep).join('/'))
|
|
||||||
.sort();
|
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
@@ -93,6 +102,19 @@ export function parseVitestFileList(output) {
|
|||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Whether the `vitest list` paths and the walked tree name at least one file in common.
|
||||||
|
* False means the two sides are not speaking the same path format (absolute paths, backslashes
|
||||||
|
* or a new prefix after a vitest upgrade), and then {@link findLeaks} would find nothing
|
||||||
|
* against a perfectly non-empty listing.
|
||||||
|
*
|
||||||
|
* @param {Set<string>} ciFiles
|
||||||
|
* @param {string[]} testFiles
|
||||||
|
*/
|
||||||
|
export function listingMatchesTree(ciFiles, testFiles) {
|
||||||
|
return testFiles.some((file) => ciFiles.has(file));
|
||||||
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* @param {string[]} browserTests
|
* @param {string[]} browserTests
|
||||||
* @param {Set<string>} ciFiles
|
* @param {Set<string>} ciFiles
|
||||||
@@ -103,6 +125,7 @@ export function findLeaks(browserTests, ciFiles) {
|
|||||||
}
|
}
|
||||||
|
|
||||||
function main() {
|
function main() {
|
||||||
|
const testFiles = findTestFiles(ROOT);
|
||||||
const browserTests = findBrowserTests(ROOT);
|
const browserTests = findBrowserTests(ROOT);
|
||||||
|
|
||||||
let collected;
|
let collected;
|
||||||
@@ -124,6 +147,16 @@ function main() {
|
|||||||
console.error('✗ `vitest list` reported no test files; refusing to pass on an empty CI set.');
|
console.error('✗ `vitest list` reported no test files; refusing to pass on an empty CI set.');
|
||||||
process.exit(1);
|
process.exit(1);
|
||||||
}
|
}
|
||||||
|
// Same vacuous pass, one step removed: a listing whose paths never match the tree. This guard,
|
||||||
|
// not `vitest list --json`, is the answer to format drift: the JSON form prints absolute paths
|
||||||
|
// that would need canonicalizing against ROOT (symlinked checkouts), and its shape can drift too.
|
||||||
|
if (!listingMatchesTree(ciFiles, testFiles)) {
|
||||||
|
const sample = [...ciFiles].slice(0, 3).join(', ');
|
||||||
|
console.error(
|
||||||
|
`✗ none of the ${ciFiles.size} paths \`vitest list\` reported (e.g. ${sample}) is one of the ${testFiles.length} test/**/*.test.ts files; its output format has probably changed.`
|
||||||
|
);
|
||||||
|
process.exit(1);
|
||||||
|
}
|
||||||
|
|
||||||
const leaked = findLeaks(browserTests, ciFiles);
|
const leaked = findLeaks(browserTests, ciFiles);
|
||||||
|
|
||||||
|
|||||||
+16
-3
@@ -28,7 +28,11 @@ import { execFileSync } from 'node:child_process';
|
|||||||
import { chmodSync, existsSync, mkdirSync, readFileSync, realpathSync, writeFileSync } from 'node:fs';
|
import { chmodSync, existsSync, mkdirSync, readFileSync, realpathSync, writeFileSync } from 'node:fs';
|
||||||
import { basename, dirname, join, resolve } from 'node:path';
|
import { basename, dirname, join, resolve } from 'node:path';
|
||||||
|
|
||||||
/** Ownership marker. Bump the version suffix when the body changes meaningfully. */
|
/**
|
||||||
|
* Ownership marker. ⚠️ Never bump the version suffix: ownership is matched on this exact
|
||||||
|
* string, so a `v2` would read every installed `v1` hook as foreign and never refresh it.
|
||||||
|
* A changed body still reaches installed hooks, because the refresh compares the whole file.
|
||||||
|
*/
|
||||||
export const PRE_PUSH_MARKER = '# codeman-managed-hook: pre-push v1';
|
export const PRE_PUSH_MARKER = '# codeman-managed-hook: pre-push v1';
|
||||||
|
|
||||||
/**
|
/**
|
||||||
@@ -52,8 +56,10 @@ export const PRE_PUSH_CHECKS = [
|
|||||||
* check:frontend-syntax), config/ (eslint + vitest configs, test-suites.ts, the CLI
|
* check:frontend-syntax), config/ (eslint + vitest configs, test-suites.ts, the CLI
|
||||||
* catalogue), scripts/ (every check is a script there, and typecheck's second pass compiles
|
* catalogue), scripts/ (every check is a script there, and typecheck's second pass compiles
|
||||||
* one), test/ (check:browser-excludes scans it and runs `vitest list` over it),
|
* one), test/ (check:browser-excludes scans it and runs `vitest list` over it),
|
||||||
* package.json + package-lock.json (check:lockfile) and install.sh (generate:cli-catalog
|
* package.json + package-lock.json (check:lockfile), install.sh (generate:cli-catalog
|
||||||
* --check diffs its generated block).
|
* --check diffs its generated block), tsconfig.json (typecheck, and
|
||||||
|
* config/tsconfig.scripts.json extends it) and .prettierignore + .editorconfig
|
||||||
|
* (format:check; the Prettier CLI honours .editorconfig by default).
|
||||||
*/
|
*/
|
||||||
export const PRE_PUSH_WATCHED_PATHS = [
|
export const PRE_PUSH_WATCHED_PATHS = [
|
||||||
'src',
|
'src',
|
||||||
@@ -63,6 +69,9 @@ export const PRE_PUSH_WATCHED_PATHS = [
|
|||||||
'package.json',
|
'package.json',
|
||||||
'package-lock.json',
|
'package-lock.json',
|
||||||
'install.sh',
|
'install.sh',
|
||||||
|
'tsconfig.json',
|
||||||
|
'.prettierignore',
|
||||||
|
'.editorconfig',
|
||||||
];
|
];
|
||||||
|
|
||||||
/**
|
/**
|
||||||
@@ -95,6 +104,10 @@ if [ ! -d node_modules ]; then
|
|||||||
exit 0
|
exit 0
|
||||||
fi
|
fi
|
||||||
|
|
||||||
|
# GUI git clients and IDEs often run hooks with a minimal PATH that lacks an nvm or
|
||||||
|
# Homebrew Node. Every check would then fail with "npm: not found", so skip instead.
|
||||||
|
command -v npm >/dev/null 2>&1 || { echo "pre-push: npm not on PATH, skipping checks."; exit 0; }
|
||||||
|
|
||||||
# git feeds us "<localref> <localsha> <remoteref> <remotesha>" per ref. A deletion has an
|
# git feeds us "<localref> <localsha> <remoteref> <remotesha>" per ref. A deletion has an
|
||||||
# all-zero local sha and no tree worth checking; if every ref is a deletion, skip.
|
# all-zero local sha and no tree worth checking; if every ref is a deletion, skip.
|
||||||
# The checks below read the working tree, so they only say something about a pushed commit
|
# The checks below read the working tree, so they only say something about a pushed commit
|
||||||
|
|||||||
@@ -11,7 +11,9 @@ import { join, resolve } from 'node:path';
|
|||||||
import {
|
import {
|
||||||
findBrowserTests,
|
findBrowserTests,
|
||||||
findLeaks,
|
findLeaks,
|
||||||
|
findTestFiles,
|
||||||
importsBrowserDriver,
|
importsBrowserDriver,
|
||||||
|
listingMatchesTree,
|
||||||
parseVitestFileList,
|
parseVitestFileList,
|
||||||
} from '../scripts/check-browser-test-excludes.mjs';
|
} from '../scripts/check-browser-test-excludes.mjs';
|
||||||
import { BROWSER_TEST_GLOBS } from '../config/test-suites';
|
import { BROWSER_TEST_GLOBS } from '../config/test-suites';
|
||||||
@@ -68,6 +70,15 @@ describe('findBrowserTests (fixture tree)', () => {
|
|||||||
'test/new.browser.test.ts',
|
'test/new.browser.test.ts',
|
||||||
]);
|
]);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it('lists every test file, browser-driven or not, in the same form', () => {
|
||||||
|
expect(findTestFiles(root)).toEqual([
|
||||||
|
'test/legacy-name.test.ts',
|
||||||
|
'test/nested/deep.test.ts',
|
||||||
|
'test/new.browser.test.ts',
|
||||||
|
'test/unit.test.ts',
|
||||||
|
]);
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
describe('parseVitestFileList + findLeaks', () => {
|
describe('parseVitestFileList + findLeaks', () => {
|
||||||
@@ -83,6 +94,19 @@ describe('parseVitestFileList + findLeaks', () => {
|
|||||||
]);
|
]);
|
||||||
expect(findLeaks(['test/new.browser.test.ts'], ci)).toEqual([]);
|
expect(findLeaks(['test/new.browser.test.ts'], ci)).toEqual([]);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it('flags a non-empty listing whose paths never match the tree instead of passing vacuously', () => {
|
||||||
|
const tree = ['test/legacy-name.test.ts', 'test/unit.test.ts'];
|
||||||
|
// e.g. a vitest upgrade that starts printing absolute paths: nothing leaks, but only
|
||||||
|
// because nothing matches, so the checker must refuse rather than report success.
|
||||||
|
const drifted = parseVitestFileList('/repo/test/legacy-name.test.ts\n/repo/test/unit.test.ts\n');
|
||||||
|
expect(drifted.size).toBe(2);
|
||||||
|
expect(findLeaks(['test/legacy-name.test.ts'], drifted)).toEqual([]);
|
||||||
|
expect(listingMatchesTree(drifted, tree)).toBe(false);
|
||||||
|
|
||||||
|
const healthy = parseVitestFileList('test/legacy-name.test.ts\ntest/unit.test.ts\n');
|
||||||
|
expect(listingMatchesTree(healthy, tree)).toBe(true);
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
describe('against this repository', () => {
|
describe('against this repository', () => {
|
||||||
|
|||||||
+51
-5
@@ -24,6 +24,7 @@ import {
|
|||||||
realpathSync,
|
realpathSync,
|
||||||
rmSync,
|
rmSync,
|
||||||
statSync,
|
statSync,
|
||||||
|
symlinkSync,
|
||||||
writeFileSync,
|
writeFileSync,
|
||||||
} from 'node:fs';
|
} from 'node:fs';
|
||||||
import { tmpdir } from 'node:os';
|
import { tmpdir } from 'node:os';
|
||||||
@@ -133,6 +134,24 @@ describe('planHookInstall', () => {
|
|||||||
});
|
});
|
||||||
|
|
||||||
describe('resolveGitHooksDir (temp repos)', () => {
|
describe('resolveGitHooksDir (temp repos)', () => {
|
||||||
|
// resolveGitHooksDir runs git with process.env, so an exported GIT_CONFIG_GLOBAL or a system
|
||||||
|
// gitconfig carrying core.hooksPath would otherwise redirect every expectation below.
|
||||||
|
// test/setup.ts swaps HOME, which only covers ~/.gitconfig.
|
||||||
|
const ambient = {
|
||||||
|
GIT_CONFIG_GLOBAL: process.env.GIT_CONFIG_GLOBAL,
|
||||||
|
GIT_CONFIG_NOSYSTEM: process.env.GIT_CONFIG_NOSYSTEM,
|
||||||
|
};
|
||||||
|
beforeAll(() => {
|
||||||
|
process.env.GIT_CONFIG_NOSYSTEM = '1';
|
||||||
|
process.env.GIT_CONFIG_GLOBAL = '/dev/null';
|
||||||
|
});
|
||||||
|
afterAll(() => {
|
||||||
|
for (const [k, v] of Object.entries(ambient)) {
|
||||||
|
if (v === undefined) delete process.env[k];
|
||||||
|
else process.env[k] = v;
|
||||||
|
}
|
||||||
|
});
|
||||||
|
|
||||||
it('resolves <root>/.git/hooks in a plain checkout', () => {
|
it('resolves <root>/.git/hooks in a plain checkout', () => {
|
||||||
const repo = newRepo();
|
const repo = newRepo();
|
||||||
expect(resolveGitHooksDir(repo)).toBe(join(repo, '.git', 'hooks'));
|
expect(resolveGitHooksDir(repo)).toBe(join(repo, '.git', 'hooks'));
|
||||||
@@ -352,9 +371,16 @@ describe('the installed hook on a real push (temp repos)', () => {
|
|||||||
expect(ran()).toEqual(expectedRuns);
|
expect(ran()).toEqual(expectedRuns);
|
||||||
});
|
});
|
||||||
|
|
||||||
it.each(['src/wip.ts', 'config/wip.json', 'scripts/wip.mjs', 'test/wip.test.ts', 'install.sh'])(
|
it.each([
|
||||||
'skips when %s is untracked (another session may own it)',
|
'src/wip.ts',
|
||||||
(rel) => {
|
'config/wip.json',
|
||||||
|
'scripts/wip.mjs',
|
||||||
|
'test/wip.test.ts',
|
||||||
|
'install.sh',
|
||||||
|
'tsconfig.json',
|
||||||
|
'.prettierignore',
|
||||||
|
'.editorconfig',
|
||||||
|
])('skips when %s is untracked (another session may own it)', (rel) => {
|
||||||
const { ran, push, repo } = setup({ failing: 'lint' });
|
const { ran, push, repo } = setup({ failing: 'lint' });
|
||||||
mkdirSync(join(repo, rel, '..'), { recursive: true });
|
mkdirSync(join(repo, rel, '..'), { recursive: true });
|
||||||
writeFileSync(join(repo, rel), 'wip\n');
|
writeFileSync(join(repo, rel), 'wip\n');
|
||||||
@@ -362,8 +388,7 @@ describe('the installed hook on a real push (temp repos)', () => {
|
|||||||
expect(r.status, r.stderr).toBe(0);
|
expect(r.status, r.stderr).toBe(0);
|
||||||
expect(r.stdout + r.stderr).toContain('pre-push: skipping static checks: uncommitted changes under');
|
expect(r.stdout + r.stderr).toContain('pre-push: skipping static checks: uncommitted changes under');
|
||||||
expect(ran()).toEqual([]);
|
expect(ran()).toEqual([]);
|
||||||
}
|
});
|
||||||
);
|
|
||||||
|
|
||||||
it('skips when a tracked package.json has an unstaged edit', () => {
|
it('skips when a tracked package.json has an unstaged edit', () => {
|
||||||
const { ran, push, repo } = setup({ failing: 'lint' });
|
const { ran, push, repo } = setup({ failing: 'lint' });
|
||||||
@@ -395,6 +420,9 @@ describe('the installed hook on a real push (temp repos)', () => {
|
|||||||
'package.json',
|
'package.json',
|
||||||
'package-lock.json',
|
'package-lock.json',
|
||||||
'install.sh',
|
'install.sh',
|
||||||
|
'tsconfig.json',
|
||||||
|
'.prettierignore',
|
||||||
|
'.editorconfig',
|
||||||
]);
|
]);
|
||||||
});
|
});
|
||||||
|
|
||||||
@@ -405,6 +433,24 @@ describe('the installed hook on a real push (temp repos)', () => {
|
|||||||
expect(r.stdout + r.stderr).toContain('node_modules missing');
|
expect(r.stdout + r.stderr).toContain('node_modules missing');
|
||||||
expect(ran()).toEqual([]);
|
expect(ran()).toEqual([]);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it('skips (never blocks) when npm is not on PATH, as under a GUI git client', () => {
|
||||||
|
const { ran, push } = setup({ failing: 'lint' });
|
||||||
|
// A PATH holding only what git and the hook need, and no npm/node. Symlinks rather than
|
||||||
|
// the real directories, since /usr/bin usually holds npm right next to git.
|
||||||
|
const bin = join(scratch, `bin-${counter}`);
|
||||||
|
mkdirSync(bin);
|
||||||
|
for (const tool of ['git', 'sh', 'mktemp', 'tail', 'rm', 'cat']) {
|
||||||
|
const found = spawnSync('sh', ['-c', `command -v ${tool}`], { encoding: 'utf8' }).stdout.trim();
|
||||||
|
expect(found, `${tool} not found on the test PATH`).toMatch(/^\//);
|
||||||
|
symlinkSync(found, join(bin, tool));
|
||||||
|
}
|
||||||
|
expect(spawnSync('sh', ['-c', 'command -v npm'], { env: { PATH: bin } }).status).not.toBe(0);
|
||||||
|
const r = push(['origin', 'main'], { PATH: bin });
|
||||||
|
expect(r.status, r.stderr + r.stdout).toBe(0);
|
||||||
|
expect(r.stdout + r.stderr).toContain('pre-push: npm not on PATH, skipping checks.');
|
||||||
|
expect(ran()).toEqual([]);
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
describe('postinstall wiring', () => {
|
describe('postinstall wiring', () => {
|
||||||
|
|||||||
Reference in New Issue
Block a user