diff --git a/lib/quarantine.ts b/lib/quarantine.ts index 04ddb2dc3..2b4c619e9 100644 --- a/lib/quarantine.ts +++ b/lib/quarantine.ts @@ -146,10 +146,13 @@ export function entryProblems(id: string, e: QuarantineEntry, now: Date): string if (entered > now.getTime() + DAY) p.push('enteredAt is in the future'); if (!Array.isArray(e.renewals)) return [...p, 'renewals is not a list']; if (e.renewals.length > 1) p.push(`renewed ${e.renewals.length} times; one renewal is allowed, then fix the test or release it`); - const firstExpiry = e.renewals.length ? Date.parse(e.renewals[0]!.previousExpiresAt) : expires; + const firstExpiry = e.renewals.length ? Date.parse(e.renewals[0]?.previousExpiresAt) : expires; if (!(firstExpiry > entered)) p.push('expires before it entered'); if (firstExpiry - entered > MAX_DAYS * DAY) p.push(`expires more than ${MAX_DAYS} days after it entered`); for (const r of e.renewals) { + if (!r || typeof r !== 'object') { p.push('a renewal is not an object'); continue; } + const renewed = Date.parse(r.renewedAt); + if (!(renewed >= entered && renewed <= now.getTime() + DAY)) p.push('a renewal is dated before the entry or in the future'); if (!filled(r?.owner)) p.push('a renewal names no owner'); if (!filledList(r?.evidence) || r.evidence.every(x => filledList(e.evidence) && e.evidence.includes(x))) p.push('a renewal carries no new evidence'); const from = Math.max(Date.parse(r?.renewedAt), Date.parse(r?.previousExpiresAt)); @@ -281,9 +284,9 @@ export function checkRegistry(cwd: string, o: { base?: string; suiteTests?: numb const commit = resolveCommit(o.base, root); const mb = git(['merge-base', commit, 'HEAD'], root); if (!mb.ok) throw new QuarantineError('FLAKY_QUARANTINE_BASE', `no merge base between ${o.base} and HEAD (a shallow clone?)`); - const changed = git(['diff', '--name-only', mb.out.trim()], root); + const changed = git(['-c', 'core.quotePath=false', 'diff', '--name-only', '-z', mb.out.trim()], root); if (!changed.ok) throw new QuarantineError('FLAKY_QUARANTINE_BASE', `cannot diff against the merge base of ${o.base}`); - problems.push(...baseProblems(registryAt(mb.out.trim(), root, `the merge base of ${o.base}`), head, changed.out.split('\n').filter(Boolean))); + problems.push(...baseProblems(registryAt(mb.out.trim(), root, `the merge base of ${o.base}`), head, changed.out.split('\0').filter(Boolean))); } const live = liveEntries(head, now); const tests = o.suiteTests ?? smallestSuite(live); diff --git a/test/quarantine.test.ts b/test/quarantine.test.ts index 747afbd77..6d4b2ff20 100644 --- a/test/quarantine.test.ts +++ b/test/quarantine.test.ts @@ -67,6 +67,10 @@ describe('registry: add, renew, release', () => { expect(renewEntry(expired, id, { owner: 'sam', evidence: ['https://run/9'], days: 3 }, at(10)).expiresAt).toBe(at(13).toISOString()); const twice = { ...e, renewals: [...e.renewals, { ...e.renewals[0]!, evidence: ['https://run/3'] }] }; expect(entryProblems(id, twice, at(6)).join()).toContain('one renewal is allowed'); + const farFuture = { ...e, expiresAt: at(400).toISOString(), renewals: [{ ...e.renewals[0]!, renewedAt: at(393).toISOString(), expiresAt: at(400).toISOString() }] }; + expect(entryProblems(id, farFuture, at(6)).join()).toContain('a renewal is dated before the entry or in the future'); + expect(liveQuarantine({ schema: 1, entries: { [id]: farFuture }, released: [] }, at(6)).ids.size).toBe(0); + expect(entryProblems(id, { ...e, renewals: [null as never] }, at(6)).join()).toContain('a renewal is not an object'); }); test('release moves the entry to released with its reason and PR', () => { @@ -174,6 +178,17 @@ describe('check --base against a git fixture', () => { ]); }); + test('a non-ASCII test path is matched too (git would quote it by default)', () => { + const { dir, reg } = fixtureRepo(); + fs.writeFileSync(path.join(dir, 'test', 'über.test.ts'), '// ü\n'); + git(dir, 'add', '-A'); + git(dir, 'commit', '-qm', 'add über'); + const next = clone(reg); + add(next, 'test/über.test.ts::::t', { suiteIds: ['test/über.test.ts::::t', 'test/a.test.ts::::slow'] }); + writeRegistry(dir, next); + expect(checkRegistry(dir, { base: 'main' }, NOW).problems.join('\n')).toContain('test/über.test.ts::::t: added in the same change that edits test/über.test.ts'); + }); + test('the bin: check exits 1 with QUARANTINE_REFUSED lines, and an unknown base exits 3', () => { const { dir, reg } = fixtureRepo(new Date()); const deleted = clone(reg);