* fix(#1224): accept --pr 0 placeholder at changeset creation The required-field guard `!opts.pr` treated the integer 0 as falsy, rejecting the documented `pr: 0` two-push placeholder with a usage error (exit 2). Non-numeric `--pr abc` (NaN) was also silently accepted before (passes `!NaN === true`... actually `!NaN` is true, so NaN would trigger the guard already). The new explicit checks use `opts.pr === null` for missing flag and `Number.isNaN` for non-numeric input, accepting all finite integer values including 0. The merge-time safety net in parse.cjs (`pr <= 0` → INVALID_PR) is unchanged — a pr:0 fragment is still rejected at lint/render time. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * chore(#1224): backfill changeset PR number (#1231) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
@@ -6,15 +6,19 @@ const assert = require('node:assert/strict');
|
||||
const fs = require('node:fs');
|
||||
const os = require('node:os');
|
||||
const path = require('node:path');
|
||||
const { spawnSync } = require('node:child_process');
|
||||
|
||||
const ROOT = path.join(__dirname, '..');
|
||||
const { generateFragmentName, scaffoldFragment, parseFragment } = (() => {
|
||||
const { generateFragmentName, scaffoldFragment, parseFragment, parseArgs } = (() => {
|
||||
const newCs = require(path.join(ROOT, 'scripts', 'changeset', 'new.cjs'));
|
||||
const parse = require(path.join(ROOT, 'scripts', 'changeset', 'parse.cjs'));
|
||||
return { ...newCs, parseFragment: parse.parseFragment };
|
||||
})();
|
||||
const { FRAGMENT_ERROR } = require(path.join(ROOT, 'scripts', 'changeset', 'parse.cjs'));
|
||||
const { cleanup } = require('./helpers.cjs');
|
||||
|
||||
const NEW_CJS = path.join(ROOT, 'scripts', 'changeset', 'new.cjs');
|
||||
|
||||
let tmp;
|
||||
before(() => { tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-new-changeset-')); });
|
||||
after(() => { cleanup(tmp); });
|
||||
@@ -95,3 +99,85 @@ describe('changeset new: name generator + scaffold writer (#2975)', () => {
|
||||
assert.deepEqual(r.opts, { type: 'Fixed', pr: 42, body: 'a body', repo: '/tmp/x' });
|
||||
});
|
||||
});
|
||||
|
||||
describe('changeset new: --pr 0 placeholder acceptance (bug #1224)', () => {
|
||||
// (a) The headline bug: `main()` (not just parseArgs) must ACCEPT --pr 0.
|
||||
// We exercise the end-to-end CLI path via spawnSync so that main()'s guard
|
||||
// is on the critical path. Old code had `if (!opts.pr)` which treated 0 as
|
||||
// falsy and exited with code 2. Fixed code uses explicit `=== null` / isNaN
|
||||
// guards and must exit 0 and write a fragment file with pr: 0 in frontmatter.
|
||||
test('(a) CLI main() accepts --pr 0 (exit 0) and writes a pr:0 fragment rejected at lint time', () => {
|
||||
// Use an isolated temp dir per invocation to avoid .changeset/ pollution
|
||||
// from other tests sharing `tmp`.
|
||||
const isolatedDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-new-cs-a-'));
|
||||
try {
|
||||
const result = spawnSync(
|
||||
process.execPath,
|
||||
[NEW_CJS, '--type', 'Fixed', '--pr', '0', '--body', 'placeholder for pr-zero.', '--repo', isolatedDir],
|
||||
{ encoding: 'utf8', timeout: 10000 },
|
||||
);
|
||||
|
||||
// The bug: old main() did `if (!opts.pr)` → exit 2. Fixed: exit 0.
|
||||
assert.equal(result.status, 0,
|
||||
`CLI main() must exit 0 for --pr 0; got status=${result.status} stderr=${result.stderr}`);
|
||||
|
||||
// A fragment file must exist under .changeset/
|
||||
const csDir = path.join(isolatedDir, '.changeset');
|
||||
const files = fs.readdirSync(csDir).filter(f => f.endsWith('.md'));
|
||||
assert.equal(files.length, 1,
|
||||
`Expected exactly one fragment file in ${csDir}, got: ${JSON.stringify(files)}`);
|
||||
|
||||
// The written fragment must round-trip through parseFragment as INVALID_PR,
|
||||
// confirming pr:0 was embedded (not absent, NaN, or a positive integer).
|
||||
const src = fs.readFileSync(path.join(csDir, files[0]), 'utf8');
|
||||
const parsed = parseFragment(src);
|
||||
assert.equal(parsed.ok, false, 'parseFragment must reject a pr:0 fragment at lint time');
|
||||
assert.equal(parsed.reason, FRAGMENT_ERROR.INVALID_PR,
|
||||
`expected INVALID_PR, got: ${parsed.reason}`);
|
||||
} finally {
|
||||
cleanup(isolatedDir);
|
||||
}
|
||||
});
|
||||
|
||||
// (b) missing --pr: parseArgs leaves opts.pr as null, causing main() to reject.
|
||||
test('(b) missing --pr leaves opts.pr as null (rejected by main validation)', () => {
|
||||
const r = parseArgs(['--type', 'Fixed', '--body', 'body without pr flag.', '--repo', tmp]);
|
||||
assert.equal(r.ok, true, 'parseArgs itself succeeds; rejection is in main()');
|
||||
assert.equal(r.opts.pr, null, `opts.pr should be null when --pr is omitted, got: ${r.opts.pr}`);
|
||||
// prMissing = opts.pr === null → would trigger rejection in main()
|
||||
assert.equal(r.opts.pr === null, true, 'prMissing condition must be true');
|
||||
});
|
||||
|
||||
// (c) --pr abc: non-numeric input normalised to NaN by parseArgs.
|
||||
test('(c) --pr abc (non-numeric) produces NaN in opts.pr (rejected by main validation)', () => {
|
||||
const r = parseArgs(['--type', 'Fixed', '--pr', 'abc', '--body', 'body.', '--repo', tmp]);
|
||||
assert.equal(r.ok, true, 'parseArgs itself succeeds; rejection is in main()');
|
||||
assert.equal(Number.isNaN(r.opts.pr), true,
|
||||
`opts.pr should be NaN for non-numeric input, got: ${r.opts.pr}`);
|
||||
});
|
||||
|
||||
// (d1) --pr "" (empty string): trimmed to "", /^\d+$/ fails → NaN.
|
||||
test('(d1) --pr "" (empty string) produces NaN in opts.pr (rejected by main validation)', () => {
|
||||
const r = parseArgs(['--type', 'Fixed', '--pr', '', '--body', 'body.', '--repo', tmp]);
|
||||
assert.equal(r.ok, true, 'parseArgs itself succeeds; rejection is in main()');
|
||||
assert.equal(Number.isNaN(r.opts.pr), true,
|
||||
`opts.pr should be NaN for empty string, got: ${r.opts.pr}`);
|
||||
});
|
||||
|
||||
// (d2) --pr " " (whitespace-only): trimmed to "", /^\d+$/ fails → NaN.
|
||||
test('(d2) --pr " " (whitespace-only) produces NaN in opts.pr (rejected by main validation)', () => {
|
||||
const r = parseArgs(['--type', 'Fixed', '--pr', ' ', '--body', 'body.', '--repo', tmp]);
|
||||
assert.equal(r.ok, true, 'parseArgs itself succeeds; rejection is in main()');
|
||||
assert.equal(Number.isNaN(r.opts.pr), true,
|
||||
`opts.pr should be NaN for whitespace-only input, got: ${r.opts.pr}`);
|
||||
});
|
||||
|
||||
// (e) parse.cjs rejects a pr:0 fragment — the merge-time lint safety net is intact.
|
||||
test('(e) parseFragment rejects a pr:0 fragment with INVALID_PR (lint safety net intact)', () => {
|
||||
const fragmentContent = `---\ntype: Fixed\npr: 0\n---\nsome body text.\n`;
|
||||
const result = parseFragment(fragmentContent);
|
||||
assert.equal(result.ok, false, 'parseFragment must reject pr:0 at lint time');
|
||||
assert.equal(result.reason, FRAGMENT_ERROR.INVALID_PR,
|
||||
`expected INVALID_PR reason, got: ${result.reason}`);
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user