Skip to content

Commit 06431fd

Browse files
test: add temp directory helper guidance (openclaw#87298)
Summary: - Merged test: add temp directory helper guidance after ClawSweeper review. Automerge notes: - PR branch already contained follow-up commit before automerge: fix(scripts): honor temp report failure mode - PR branch already contained follow-up commit before automerge: fix(scripts): reduce temp report noise - PR branch already contained follow-up commit before automerge: fix(scripts): cover test support temp reports - PR branch already contained follow-up commit before automerge: fix(scripts): report temp use in test helpers - PR branch already contained follow-up commit before automerge: fix(scripts): broaden temp report test surface - PR branch already contained follow-up commit before automerge: fix(scripts): cover nested test temp reports Validation: - ClawSweeper review passed for head 132f14a. - Required merge gates passed before the squash merge. Prepared head SHA: 132f14a Review: openclaw#87298 (comment) Co-authored-by: masonxhuang <masonxhuang@tencent.com> Co-authored-by: Mason Huang <masonxhuang@tencent.com> Co-authored-by: clawsweeper <274271284+clawsweeper[bot]@users.noreply.github.com> Co-authored-by: clawsweeper[bot] <274271284+clawsweeper[bot]@users.noreply.github.com> Approved-by: hxy91819 Co-authored-by: hxy91819 <8814856+hxy91819@users.noreply.github.com>
1 parent 3d38c9a commit 06431fd

12 files changed

Lines changed: 672 additions & 9 deletions

.github/workflows/ci.yml

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1288,6 +1288,7 @@ jobs:
12881288
env:
12891289
OPENCLAW_LOCAL_CHECK: "0"
12901290
TASK: ${{ matrix.task }}
1291+
PR_BASE_SHA: ${{ github.event_name == 'pull_request' && github.event.pull_request.base.sha || '' }}
12911292
shell: bash
12921293
run: |
12931294
set -euo pipefail
@@ -1297,6 +1298,10 @@ jobs:
12971298
pnpm tool-display:check
12981299
pnpm check:host-env-policy:swift
12991300
pnpm dup:check:coverage
1301+
if [ -n "$PR_BASE_SHA" ]; then
1302+
git fetch --no-tags --depth=1 origin "+${PR_BASE_SHA}:refs/remotes/origin/pr-base"
1303+
node scripts/report-test-temp-creations.mjs --base refs/remotes/origin/pr-base --head HEAD --no-merge-base
1304+
fi
13001305
pnpm deps:patches:check
13011306
pnpm lint:webhook:no-low-level-body-read
13021307
pnpm lint:auth:no-pairing-store-group

docs/help/testing.md

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,45 @@ When you touch tests or want extra confidence:
4242
- Coverage gate: `pnpm test:coverage`
4343
- E2E suite: `pnpm test:e2e`
4444

45+
## Test Temp Directories
46+
47+
Prefer the shared helpers in `test/helpers/temp-dir.ts` for test-owned
48+
temporary directories. They make ownership explicit and keep cleanup in the same
49+
test lifecycle:
50+
51+
```ts
52+
import { afterEach } from "vitest";
53+
import { createTempDirTracker } from "../helpers/temp-dir.js";
54+
55+
const tempDirs = createTempDirTracker();
56+
57+
afterEach(tempDirs.cleanup);
58+
59+
it("uses a temp workspace", () => {
60+
const workspace = tempDirs.make("openclaw-example-");
61+
// use workspace
62+
});
63+
```
64+
65+
Use `makeTempDir(tempDirs, prefix)` and `cleanupTempDirs(tempDirs)` when a test
66+
already owns an array or set of paths. Avoid new bare `fs.mkdtemp*` calls in
67+
tests unless a case is explicitly verifying raw temp-dir behavior. Add an
68+
auditable allow comment with a concrete reason when a test intentionally needs a
69+
bare temp directory:
70+
71+
```ts
72+
// openclaw-temp-dir: allow verifies raw fs cleanup behavior
73+
const workspace = fs.mkdtempSync(prefix);
74+
```
75+
76+
For migration visibility, `node scripts/report-test-temp-creations.mjs` reports
77+
new bare temp-dir creation in added diff lines without blocking existing cleanup
78+
styles. Its file scope intentionally follows the same test-path classification
79+
used by `scripts/changed-lanes.mjs` instead of maintaining a separate test-helper
80+
filename heuristic, while skipping the shared helper implementation itself.
81+
`check:changed` runs this report for changed test paths as a warning-only CI
82+
signal; findings are GitHub warning annotations, not failures.
83+
4584
When debugging real providers/models (requires real creds):
4685

4786
- Live suite (models + gateway tool/image probes): `pnpm test:live`

scripts/changed-lanes.mjs

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,10 @@ export function createEmptyChangedLanes() {
8181
};
8282
}
8383

84+
export function isChangedLaneTestPath(changedPath) {
85+
return TEST_PATH_RE.test(normalizeChangedPath(changedPath));
86+
}
87+
8488
/**
8589
* @param {string[]} changedPaths
8690
* @param {{ packageJsonChangeKind?: "liveDockerTooling" | "tooling" | null }} [options]
@@ -165,7 +169,7 @@ export function detectChangedLanes(changedPaths, options = {}) {
165169
}
166170

167171
if (EXTENSION_PATH_RE.test(changedPath)) {
168-
if (TEST_PATH_RE.test(changedPath)) {
172+
if (isChangedLaneTestPath(changedPath)) {
169173
lanes.extensionTests = true;
170174
reasons.push(`${changedPath}: extension test`);
171175
} else {
@@ -177,7 +181,7 @@ export function detectChangedLanes(changedPaths, options = {}) {
177181
}
178182

179183
if (CORE_PATH_RE.test(changedPath)) {
180-
if (TEST_PATH_RE.test(changedPath)) {
184+
if (isChangedLaneTestPath(changedPath)) {
181185
lanes.coreTests = true;
182186
reasons.push(`${changedPath}: core test`);
183187
} else {

scripts/check-changed.mjs

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import path from "node:path";
1313
import { performance } from "node:perf_hooks";
1414
import {
1515
detectChangedLanesForPaths,
16+
isChangedLaneTestPath,
1617
listChangedPathsFromGit,
1718
listStagedChangedPaths,
1819
normalizeChangedPath,
@@ -160,6 +161,10 @@ export function shouldRunShrinkwrapGuard(paths) {
160161
return paths.some((changedPath) => SHRINKWRAP_POLICY_PATH_RE.test(changedPath));
161162
}
162163

164+
export function shouldRunTestTempCreationReport(paths) {
165+
return paths.some((changedPath) => isChangedLaneTestPath(changedPath));
166+
}
167+
163168
export function createShrinkwrapGuardCommand(paths) {
164169
if (!shouldRunShrinkwrapGuard(paths)) {
165170
return null;
@@ -210,6 +215,22 @@ export function createChangedCheckPlan(result, options = {}) {
210215
};
211216
const addTypecheck = (name, args) => add(name, args, createSparseTsgoSkipEnv(baseEnv));
212217
const addLint = (name, args) => add(name, args, baseEnv);
218+
const addTestTempCreationReport = () => {
219+
if (!shouldRunTestTempCreationReport(result.paths)) {
220+
return;
221+
}
222+
addCommand(
223+
"test temp creation report (warning-only)",
224+
"node",
225+
[
226+
"scripts/report-test-temp-creations.mjs",
227+
...(options.staged
228+
? ["--staged"]
229+
: ["--base", options.base ?? "origin/main", "--head", options.head ?? "HEAD"]),
230+
],
231+
baseEnv,
232+
);
233+
};
213234

214235
add("conflict markers", ["check:no-conflict-markers"]);
215236
add("changelog attributions", ["check:changelog-attributions"]);
@@ -235,6 +256,8 @@ export function createChangedCheckPlan(result, options = {}) {
235256
};
236257
}
237258

259+
addTestTempCreationReport();
260+
238261
const lanes = result.lanes;
239262
const runAll = lanes.all;
240263

Lines changed: 234 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,234 @@
1+
#!/usr/bin/env node
2+
3+
import { execFileSync } from "node:child_process";
4+
import { isChangedLaneTestPath } from "./changed-lanes.mjs";
5+
import { booleanFlag, parseFlagArgs, stringFlag } from "./lib/arg-utils.mjs";
6+
import { runAsScript } from "./lib/ts-guard-utils.mjs";
7+
8+
const DEFAULT_BASE_REF = "origin/main";
9+
const DEFAULT_HEAD_REF = "HEAD";
10+
const TEMP_DIR_HELPER_PATH = "test/helpers/temp-dir.ts";
11+
const FINDING_PATTERNS = [
12+
{
13+
pattern: /\bmkdtemp(?:Sync)?\s*\(/u,
14+
reason: "new mkdtemp temp directory creation",
15+
},
16+
{
17+
pattern: /\btmp\s*\.\s*dir(?:Sync)?\s*\(/u,
18+
reason: "new tmp.dir temp directory creation",
19+
},
20+
];
21+
const TEMP_DIR_ALLOW_COMMENT_RE =
22+
/(?:^|\s)(?:\/\/|\/\*|\*|#)\s*openclaw-temp-dir:\s*allow\s+(.+)$/u;
23+
24+
function usage() {
25+
return `Usage: node scripts/report-test-temp-creations.mjs [options]
26+
27+
Description:
28+
Reports new bare test temp-directory creation patterns in added diff lines.
29+
This is a low-noise migration aid, not a cleanup data-flow checker. It does
30+
not scan existing lines and does not decide whether cleanup is sufficient.
31+
Add "openclaw-temp-dir: allow <reason>" in a same-line or immediately
32+
preceding added comment when a test intentionally needs bare temp creation.
33+
File scope intentionally reuses scripts/changed-lanes.mjs test-path
34+
classification instead of maintaining a separate test-helper heuristic.
35+
36+
Options:
37+
--base <ref> Base ref for branch diffs. Default: ${DEFAULT_BASE_REF}
38+
--head <ref> Head ref for branch diffs. Default: ${DEFAULT_HEAD_REF}
39+
--no-merge-base Use a two-dot base..head diff for shallow CI checkouts.
40+
--staged Inspect staged changes instead of a branch diff.
41+
--json Print JSON findings to stdout.
42+
--fail-on-findings Exit 1 when findings are present. Default is report-only.
43+
-h, --help Show this help.
44+
45+
Outputs:
46+
Human mode prints findings to stderr and exits 0 unless --fail-on-findings is set.
47+
GitHub Actions mode prints warning annotations and exits 0 unless --fail-on-findings is set.
48+
JSON mode prints an array of { file, line, reason, source } to stdout.
49+
50+
Examples:
51+
node scripts/report-test-temp-creations.mjs --base origin/main --head HEAD
52+
node scripts/report-test-temp-creations.mjs --staged --json
53+
`;
54+
}
55+
56+
function normalizePath(filePath) {
57+
return String(filePath ?? "")
58+
.replaceAll("\\", "/")
59+
.replace(/^\.\/+/u, "");
60+
}
61+
62+
function shouldInspectFile(filePath) {
63+
const normalizedPath = normalizePath(filePath);
64+
return normalizedPath !== TEMP_DIR_HELPER_PATH && isChangedLaneTestPath(normalizedPath);
65+
}
66+
67+
function isTruthyEnvFlag(value) {
68+
const normalized = String(value ?? "")
69+
.trim()
70+
.toLowerCase();
71+
return normalized !== "" && normalized !== "0" && normalized !== "false" && normalized !== "no";
72+
}
73+
74+
function escapeGithubCommandValue(value) {
75+
return String(value).replaceAll("%", "%25").replaceAll("\r", "%0D").replaceAll("\n", "%0A");
76+
}
77+
78+
function escapeGithubCommandProperty(value) {
79+
return escapeGithubCommandValue(value).replaceAll(":", "%3A").replaceAll(",", "%2C");
80+
}
81+
82+
function hasTempDirAllowMarker(source) {
83+
const reason = source.match(TEMP_DIR_ALLOW_COMMENT_RE)?.[1]?.trim() ?? "";
84+
return reason.length > 0;
85+
}
86+
87+
function isTempDirAllowComment(source) {
88+
const trimmed = source.trim();
89+
return /^(?:\/\/|\/\*|\*|#)/u.test(trimmed) && hasTempDirAllowMarker(trimmed);
90+
}
91+
92+
export function formatGithubWarning(finding) {
93+
const file = escapeGithubCommandProperty(finding.file);
94+
const line = escapeGithubCommandProperty(finding.line);
95+
const message = escapeGithubCommandValue(
96+
`${finding.reason}: prefer test/helpers/temp-dir.ts for new test-owned temp directories.`,
97+
);
98+
return `::warning file=${file},line=${line}::${message}`;
99+
}
100+
101+
function parseArgs(argv) {
102+
const args = {
103+
base: DEFAULT_BASE_REF,
104+
failOnFindings: false,
105+
head: DEFAULT_HEAD_REF,
106+
help: false,
107+
json: false,
108+
noMergeBase: false,
109+
staged: false,
110+
};
111+
return parseFlagArgs(argv, args, [
112+
stringFlag("--base", "base"),
113+
booleanFlag("--fail-on-findings", "failOnFindings"),
114+
stringFlag("--head", "head"),
115+
booleanFlag("-h", "help"),
116+
booleanFlag("--help", "help"),
117+
booleanFlag("--json", "json"),
118+
booleanFlag("--no-merge-base", "noMergeBase"),
119+
booleanFlag("--staged", "staged"),
120+
]);
121+
}
122+
123+
function readDiff(args, cwd = process.cwd()) {
124+
const range = args.noMergeBase ? `${args.base}..${args.head}` : `${args.base}...${args.head}`;
125+
const diffArgs = args.staged
126+
? ["diff", "--cached", "--unified=0", "--diff-filter=ACMR", "--"]
127+
: ["diff", "--unified=0", "--diff-filter=ACMR", range, "--"];
128+
return execFileSync("git", diffArgs, {
129+
cwd,
130+
encoding: "utf8",
131+
maxBuffer: 64 * 1024 * 1024,
132+
stdio: ["ignore", "pipe", "pipe"],
133+
});
134+
}
135+
136+
export function collectTempCreationFindingsFromDiff(diffText) {
137+
const findings = [];
138+
let currentFile = null;
139+
let currentLine = 0;
140+
let allowNextLine = null;
141+
142+
for (const line of diffText.split(/\r?\n/u)) {
143+
const fileMatch = line.match(/^\+\+\+ b\/(.+)$/u);
144+
if (fileMatch) {
145+
currentFile = normalizePath(fileMatch[1]);
146+
allowNextLine = null;
147+
continue;
148+
}
149+
if (line === "+++ /dev/null") {
150+
currentFile = null;
151+
allowNextLine = null;
152+
continue;
153+
}
154+
155+
const hunkMatch = line.match(/^@@ -\d+(?:,\d+)? \+(\d+)(?:,\d+)? @@/u);
156+
if (hunkMatch) {
157+
currentLine = Number.parseInt(hunkMatch[1], 10);
158+
allowNextLine = null;
159+
continue;
160+
}
161+
162+
if (line.startsWith("+") && !line.startsWith("+++")) {
163+
if (currentFile && shouldInspectFile(currentFile)) {
164+
const source = line.slice(1);
165+
const allowed =
166+
hasTempDirAllowMarker(source) ||
167+
(allowNextLine?.file === currentFile && allowNextLine.line === currentLine);
168+
for (const { pattern, reason } of FINDING_PATTERNS) {
169+
if (pattern.test(source)) {
170+
if (!allowed) {
171+
findings.push({
172+
file: currentFile,
173+
line: currentLine,
174+
reason,
175+
source: source.trim(),
176+
});
177+
}
178+
break;
179+
}
180+
}
181+
allowNextLine = isTempDirAllowComment(source)
182+
? { file: currentFile, line: currentLine + 1 }
183+
: null;
184+
}
185+
currentLine += 1;
186+
continue;
187+
}
188+
189+
if (line.startsWith(" ") || line === "") {
190+
allowNextLine = null;
191+
currentLine += 1;
192+
}
193+
}
194+
195+
return findings;
196+
}
197+
198+
export async function main(argv, io) {
199+
const args = parseArgs(argv ?? process.argv.slice(2));
200+
const stdout = io?.stdout ?? process.stdout;
201+
const stderr = io?.stderr ?? process.stderr;
202+
const env = io?.env ?? process.env;
203+
if (args.help) {
204+
stdout.write(usage());
205+
return 0;
206+
}
207+
208+
const findings = collectTempCreationFindingsFromDiff(readDiff(args));
209+
if (args.json) {
210+
stdout.write(`${JSON.stringify(findings, null, 2)}\n`);
211+
} else if (findings.length === 0) {
212+
stderr.write("No new bare test temp-directory creation patterns found.\n");
213+
} else if (isTruthyEnvFlag(env.GITHUB_ACTIONS)) {
214+
for (const finding of findings) {
215+
stderr.write(`${formatGithubWarning(finding)}\n`);
216+
}
217+
} else {
218+
stderr.write("New bare test temp-directory creation patterns:\n");
219+
for (const finding of findings) {
220+
stderr.write(`- ${finding.file}:${finding.line} ${finding.reason}: ${finding.source}\n`);
221+
}
222+
stderr.write("Prefer test/helpers/temp-dir.ts for new test-owned temp directories.\n");
223+
}
224+
225+
return args.failOnFindings && findings.length > 0 ? 1 : 0;
226+
}
227+
228+
runAsScript(import.meta.url, async (argv, io) => {
229+
const exitCode = await main(argv, io);
230+
if (!io) {
231+
process.exitCode = exitCode;
232+
}
233+
return exitCode;
234+
});

0 commit comments

Comments
 (0)