diff --git a/src/api/rateLimit.test.ts b/src/api/rateLimit.test.ts index cce57b5..66f6823 100644 --- a/src/api/rateLimit.test.ts +++ b/src/api/rateLimit.test.ts @@ -107,18 +107,27 @@ describe('RateLimiter', () => { ).toEqual({ burst: 10, refillTokens: 5, refillIntervalMillis: 30000 }); }); - it.each(['0', '-1', 'abc', 'NaN'])('should fall back and complain about %s', value => { - const complaints: string[] = []; - const original = console.error; - console.error = (m: string) => complaints.push(m); - try { - expect(bucketFromEnvironment({ STEADYBIT_RATE_LIMIT_BURST: value }).burst).toEqual(defaultBucket.burst); - } finally { - console.error = original; - } - expect(complaints).toHaveLength(1); - expect(complaints[0]).toContain('STEADYBIT_RATE_LIMIT_BURST'); + it('should take a value with surrounding whitespace', () => { + expect(bucketFromEnvironment({ STEADYBIT_RATE_LIMIT_BURST: ' 10 ' }).burst).toEqual(10); }); + + // `Number` would have read these as 1000, 16 and 2.5. None is how a request count + // gets written on purpose, and accepting them changes the pacing silently. + it.each(['0', '-1', 'abc', 'NaN', '1e3', '0x10', '2.5', '10,5', '+5'])( + 'should fall back and complain about %s', + value => { + const complaints: string[] = []; + const original = console.error; + console.error = (m: string) => complaints.push(m); + try { + expect(bucketFromEnvironment({ STEADYBIT_RATE_LIMIT_BURST: value }).burst).toEqual(defaultBucket.burst); + } finally { + console.error = original; + } + expect(complaints).toHaveLength(1); + expect(complaints[0]).toContain('STEADYBIT_RATE_LIMIT_BURST'); + } + ); }); it('should default to the allowance the platform documents', () => { diff --git a/src/api/rateLimit.ts b/src/api/rateLimit.ts index 78144b6..d486e0c 100644 --- a/src/api/rateLimit.ts +++ b/src/api/rateLimit.ts @@ -37,14 +37,14 @@ const systemClock: Clock = { // 429 itself, by which point remaining is zero. export function bucketFromEnvironment(env: NodeJS.ProcessEnv = process.env): BucketOptions { return { - burst: positiveNumber(env.STEADYBIT_RATE_LIMIT_BURST, 'STEADYBIT_RATE_LIMIT_BURST', defaultBucket.burst), - refillTokens: positiveNumber( + burst: positiveInteger(env.STEADYBIT_RATE_LIMIT_BURST, 'STEADYBIT_RATE_LIMIT_BURST', defaultBucket.burst), + refillTokens: positiveInteger( env.STEADYBIT_RATE_LIMIT_REFILL, 'STEADYBIT_RATE_LIMIT_REFILL', defaultBucket.refillTokens ), refillIntervalMillis: - positiveNumber( + positiveInteger( env.STEADYBIT_RATE_LIMIT_INTERVAL, 'STEADYBIT_RATE_LIMIT_INTERVAL', defaultBucket.refillIntervalMillis / 1000 @@ -52,18 +52,23 @@ export function bucketFromEnvironment(env: NodeJS.ProcessEnv = process.env): Buc }; } -function positiveNumber(value: string | undefined, name: string, fallback: number): number { +// Plain decimal digits only. `Number` would also have taken '1e3', '0x10' and '2.5', +// which are not how anyone means to write a request count, and reading '0x10' as 16 +// would quietly change how hard the CLI hits the platform. +const POSITIVE_INTEGER = /^\d+$/; + +function positiveInteger(value: string | undefined, name: string, fallback: number): number { if (value === undefined || value.trim() === '') { return fallback; } - const parsed = Number(value); - if (!Number.isFinite(parsed) || parsed <= 0) { + const trimmed = value.trim(); + if (!POSITIVE_INTEGER.test(trimmed) || Number(trimmed) <= 0) { // Warned about rather than ignored: a typo here silently changes how hard the CLI // hits the platform, which is the last thing that should fail quietly. - console.error(`Ignoring ${name}: '${value}' is not a positive number. Using ${fallback}.`); + console.error(`Ignoring ${name}: '${value}' is not a positive whole number. Using ${fallback}.`); return fallback; } - return parsed; + return Number(trimmed); } export class RateLimiter { diff --git a/src/experiment/dump.test.ts b/src/experiment/dump.test.ts index 8cfd405..f9906b5 100644 --- a/src/experiment/dump.test.ts +++ b/src/experiment/dump.test.ts @@ -53,6 +53,24 @@ describe('experiment dump', () => { process.exitCode = undefined; }); + // The list-gathering phase is paced like everything else, so without this the CLI can + // sit silent for minutes before its first line of output. + it('should report progress while gathering the experiment lists', async () => { + const written: string[] = []; + const stdout = vi.spyOn(process.stdout, 'write').mockImplementation(chunk => { + written.push(String(chunk)); + return true; + }); + + await dump({ directory: freshDirectory() }); + + stdout.mockRestore(); + const output = written.join(''); + expect(output).toContain('Listing experiments for 1 team'); + // The dot lands before the per-team walk starts, not after it. + expect(output.indexOf('.\n')).toBeLessThan(output.indexOf('Fetching experiments for team')); + }); + it('should write the experiment and its executions', async () => { givenExecutions('TST-1', [1, 2, 3]); const directory = freshDirectory(); diff --git a/src/experiment/dump.ts b/src/experiment/dump.ts index 4375b4b..9ce49b2 100644 --- a/src/experiment/dump.ts +++ b/src/experiment/dump.ts @@ -36,11 +36,16 @@ export async function dump(options: Options) { // The experiment lists are fetched up front, which costs nothing extra because each // team needs one anyway, so that the size of the walk is known before it starts. + // These requests are paced like any other, so on a tenant with many teams, or a + // reduced allowance, they take long enough that silence here reads as a hang. const teams = selectTeams(await getAllTeams(false), options.team); + process.stdout.write(`Listing experiments for ${teams.length} ${teams.length === 1 ? 'team' : 'teams'}`); const listPerTeam = new Map(); for (const team of teams) { listPerTeam.set(team.key, await fetchExperiments(team.key)); + process.stdout.write('.'); } + process.stdout.write('\n'); warnAboutLargeDump([...listPerTeam.values()].reduce((total, list) => total + list.experiments.length, 0)); for (const team of teams) {