From 85c42aaadf44f44ad71ff5b120905e275cbd7c8b Mon Sep 17 00:00:00 2001 From: Johannes Edmeier Date: Wed, 29 Jul 2026 20:38:54 +0200 Subject: [PATCH 1/6] refactor: resolve the profile store path per call The config directory was computed from the home directory at module load, so nothing could point the CLI elsewhere once the module had been imported. The existing test worked around it by setting HOME and then importing the module through a top-level await; that dance is gone now, and with it the reason command-level tests could not exercise `config profile add` or `select`. The directory-creation memo is keyed by path rather than being a single flag, because the path now follows HOME: caching "already created" would otherwise leave a later directory unmade while every write into it failed. It still deduplicates concurrent callers and still does not cache failures, so the saving that motivated it is unchanged. Two error messages named a function instead of the path they were reporting, and the active-profile read named the profiles file rather than its own. --- src/config/profile/service.test.ts | 27 +++++++++++++--- src/config/profile/service.ts | 50 +++++++++++++++++++++--------- 2 files changed, 57 insertions(+), 20 deletions(-) diff --git a/src/config/profile/service.test.ts b/src/config/profile/service.test.ts index 52fff88..8494952 100644 --- a/src/config/profile/service.test.ts +++ b/src/config/profile/service.test.ts @@ -6,17 +6,17 @@ import os from 'node:os'; import path from 'node:path'; import { beforeAll, describe, expect, it, vi } from 'vitest'; -// service.ts resolves the config directory from the home directory at module load, so -// HOME is redirected to a scratch directory before importing it. The guard below refuses -// to run the suite if that ever stops working, since these tests write profile files. +import { addProfile, getActiveProfile, getProfiles, setActiveProfile } from './service.ts'; + +// These tests write profile files, so HOME is redirected to a scratch directory. The +// guard refuses to run the suite if that ever stops working. A plain import is enough +// because service.ts resolves the config directory per call rather than at module load. const fakeHome = await fs.mkdtemp(path.join(os.tmpdir(), 'steadybit-service-test-')); process.env.HOME = fakeHome; if (!os.homedir().startsWith(fakeHome)) { throw new Error(`refusing to run: home directory is ${os.homedir()}, not the scratch directory`); } -const { addProfile, getActiveProfile, getProfiles, setActiveProfile } = await import('./service.ts'); - const profilesFile = path.join(fakeHome, '.steadybit', 'profiles.json'); describe('profile service', () => { @@ -59,4 +59,21 @@ describe('profile service', () => { expect((await getProfiles()).map(p => p.name)).toContain('third'); }); + + // The config directory used to be computed at module load, which meant nothing could + // point the CLI at a different home once this module had been imported. + it('should follow a home directory that changes after import', async () => { + const otherHome = await fs.mkdtemp(path.join(os.tmpdir(), 'steadybit-other-home-')); + const previous = process.env.HOME; + process.env.HOME = otherHome; + + try { + await setActiveProfile('written-elsewhere'); + expect(await fs.readFile(path.join(otherHome, '.steadybit', 'activeProfile'), 'utf8')).toEqual( + 'written-elsewhere' + ); + } finally { + process.env.HOME = previous; + } + }); }); diff --git a/src/config/profile/service.ts b/src/config/profile/service.ts index b3e8bab..648fc83 100644 --- a/src/config/profile/service.ts +++ b/src/config/profile/service.ts @@ -8,9 +8,12 @@ import path from 'node:path'; import { abortExecution, errorMessage } from '../../errors.ts'; import type { Profile } from './types.ts'; -const configDir = path.join(homedir(), '.steadybit'); -const profilesFile = path.join(configDir, 'profiles.json'); -const activeProfileFile = path.join(configDir, 'activeProfile'); +// Resolved per call rather than at module load. HOME is what decides where the profile +// store lives, and a test — or anything else setting it after this module is imported — +// would otherwise be talking to the developer's real ~/.steadybit. +const configDir = () => path.join(homedir(), '.steadybit'); +const profilesFile = () => path.join(configDir(), 'profiles.json'); +const activeProfileFile = () => path.join(configDir(), 'activeProfile'); // The profile files are read on every API call, three times per call via // getConfiguration(). Reading them once per process turns thousands of redundant @@ -31,9 +34,26 @@ function readOnce(read: () => Promise): (() => Promise) & { forget: () return cachingRead; } -const ensureConfigDirectoryExists = readOnce(async () => { - await fs.mkdir(configDir, { recursive: true }); -}); +// Keyed by directory rather than memoised outright: the path follows HOME, and caching +// a single "already created" flag would leave a later directory unmade while the writes +// into it fail. Failures are not cached, so an unwritable home stays retryable. +const createdDirectories = new Map>(); + +function ensureConfigDirectoryExists(): Promise { + const directory = configDir(); + let created = createdDirectories.get(directory); + if (!created) { + created = fs.mkdir(directory, { recursive: true }).then( + () => undefined, + e => { + createdDirectories.delete(directory); + throw e; + } + ); + createdDirectories.set(directory, created); + } + return created; +} export async function addProfile(profile: Profile): Promise { const profiles = await getProfiles(); @@ -56,19 +76,19 @@ const readProfiles = readOnce(async (): Promise => { let fileContent: string; try { - fileContent = await fs.readFile(profilesFile, { encoding: 'utf8' }); + fileContent = await fs.readFile(profilesFile(), { encoding: 'utf8' }); } catch (e) { if ((e as any)?.code === 'ENOENT') { return []; } - throw abortExecution("Failed to read file '%s': %s", profilesFile, errorMessage(e)); + throw abortExecution("Failed to read file '%s': %s", profilesFile(), errorMessage(e)); } try { return JSON.parse(fileContent); } catch (e) { - throw abortExecution("Failed to parse file '%s' as JSON: %s", profilesFile, errorMessage(e)); + throw abortExecution("Failed to parse file '%s' as JSON: %s", profilesFile(), errorMessage(e)); } }); @@ -80,9 +100,9 @@ async function writeProfiles(profiles: Profile[]): Promise { await ensureConfigDirectoryExists(); try { - await fs.writeFile(profilesFile, JSON.stringify(profiles, undefined, 2)); + await fs.writeFile(profilesFile(), JSON.stringify(profiles, undefined, 2)); } catch (e) { - throw abortExecution("Failed to write to file '%s': %s", profilesFile, errorMessage(e)); + throw abortExecution("Failed to write to file '%s': %s", profilesFile(), errorMessage(e)); } readProfiles.forget(); } @@ -92,10 +112,10 @@ const readActiveProfileName = readOnce(async (): Promise => try { // Users opening and saving the file might end up adding a trailing new line character. - return (await fs.readFile(activeProfileFile, { encoding: 'utf8' })).trim(); + return (await fs.readFile(activeProfileFile(), { encoding: 'utf8' })).trim(); } catch (e) { if ((e as any)?.code !== 'ENOENT') { - throw abortExecution("Failed to read file '%s': %s", profilesFile, errorMessage(e)); + throw abortExecution("Failed to read file '%s': %s", activeProfileFile(), errorMessage(e)); } return undefined; } @@ -112,9 +132,9 @@ export async function setActiveProfile(profileName: string): Promise { await ensureConfigDirectoryExists(); try { - await fs.writeFile(activeProfileFile, profileName); + await fs.writeFile(activeProfileFile(), profileName); } catch (e) { - throw abortExecution("Failed to write to file '%s': %s", activeProfileFile, errorMessage(e)); + throw abortExecution("Failed to write to file '%s': %s", activeProfileFile(), errorMessage(e)); } readActiveProfileName.forget(); } From f1a836df1515c72473a7c7764de46a823c9cf816 Mon Sep 17 00:00:00 2001 From: Johannes Edmeier Date: Wed, 29 Jul 2026 20:46:03 +0200 Subject: [PATCH 2/6] test: cover the interactive flows with @inquirer/testing The prompts had no coverage at all. Under vitest process.stdout.isTTY is undefined, so confirm() returned its non-interactive default and the prompt implementation was never even imported; `config profile add` and `select` were not exercised in any form, and cancellation was checked only by throwing a hand-made ExitPromptError rather than by cancelling anything. @inquirer/testing drives the real prompts through a shared screen, so these tests go through the same code a user does: the question sequence, the offered default, the validators as they are actually wired, and the profile the flow writes. Ctrl+C now goes through a genuine prompt and produces a genuine ExitPromptError. The existing non-interactive tests are kept rather than converted. The three confirm() call sites pass three deliberately different values for defaultWhenNonInteractive, and those decide what happens in CI, which is where most runs of this CLI happen. Reaching a prompt requires faking isTTY, since confirm() checks it before deciding to prompt at all. Whether that check reads a real terminal correctly is not something a test can answer by disabling it, so it is left to the container tests. --- package-lock.json | 112 +++++++++++++++++++++++++++--- package.json | 1 + src/config/profile/add.test.ts | 105 ++++++++++++++++++++++++++++ src/config/profile/select.test.ts | 53 ++++++++++++++ src/mocks/prompts.ts | 34 +++++++++ src/prompt/cancellation.test.ts | 49 +++++++++++++ src/prompt/confirm.test.ts | 53 ++++++++++++++ 7 files changed, 398 insertions(+), 9 deletions(-) create mode 100644 src/config/profile/add.test.ts create mode 100644 src/config/profile/select.test.ts create mode 100644 src/mocks/prompts.ts create mode 100644 src/prompt/cancellation.test.ts create mode 100644 src/prompt/confirm.test.ts diff --git a/package-lock.json b/package-lock.json index e54f191..46bd80c 100644 --- a/package-lock.json +++ b/package-lock.json @@ -25,6 +25,7 @@ }, "devDependencies": { "@eslint/js": "^10.0.1", + "@inquirer/testing": "^3.3.9", "@types/node": "^22.19.4", "@types/semver": "^7.3.9", "eslint": "^10.8.0", @@ -319,15 +320,6 @@ "node": ">= 12" } }, - "node_modules/@inquirer/core/node_modules/mute-stream": { - "version": "3.0.0", - "resolved": "https://registry.npmjs.org/mute-stream/-/mute-stream-3.0.0.tgz", - "integrity": "sha512-dkEJPVvun4FryqBmZ5KhDo0K9iDXAwn08tMLDinNdRBNPcYEDiWYysLcc6k3mjTMlbP9KyylvRpd4wFtwrT9rw==", - "license": "ISC", - "engines": { - "node": "^20.17.0 || >=22.9.0" - } - }, "node_modules/@inquirer/core/node_modules/signal-exit": { "version": "4.1.0", "resolved": "https://registry.npmjs.org/signal-exit/-/signal-exit-4.1.0.tgz", @@ -415,6 +407,89 @@ } } }, + "node_modules/@inquirer/testing": { + "version": "3.3.9", + "resolved": "https://registry.npmjs.org/@inquirer/testing/-/testing-3.3.9.tgz", + "integrity": "sha512-083hjj4LYr0HrCGKgWsKaNCT7wFsLZVUN6oqvEXcmV0tXcQgnrQ8cqtTT18KCCSZyS0ANpQifrpbFpjrelaTmw==", + "dev": true, + "license": "MIT", + "dependencies": { + "@inquirer/type": "^4.0.7", + "@xterm/headless": "^6.0.0", + "mute-stream": "^3.0.0" + }, + "engines": { + "node": ">=23.5.0 || ^22.13.0 || ^20.17.0" + }, + "peerDependencies": { + "@inquirer/checkbox": ">=1.0.0", + "@inquirer/confirm": ">=1.0.0", + "@inquirer/editor": ">=1.0.0", + "@inquirer/expand": ">=1.0.0", + "@inquirer/external-editor": ">=1.0.0", + "@inquirer/input": ">=1.0.0", + "@inquirer/number": ">=1.0.0", + "@inquirer/password": ">=1.0.0", + "@inquirer/prompts": ">=1.0.0", + "@inquirer/rawlist": ">=1.0.0", + "@inquirer/search": ">=1.0.0", + "@inquirer/select": ">=1.0.0", + "@types/jest": ">=29.0.0", + "@types/node": ">=18", + "jest": ">=29.0.0", + "vitest": ">=1.0.0" + }, + "peerDependenciesMeta": { + "@inquirer/checkbox": { + "optional": true + }, + "@inquirer/confirm": { + "optional": true + }, + "@inquirer/editor": { + "optional": true + }, + "@inquirer/expand": { + "optional": true + }, + "@inquirer/external-editor": { + "optional": true + }, + "@inquirer/input": { + "optional": true + }, + "@inquirer/number": { + "optional": true + }, + "@inquirer/password": { + "optional": true + }, + "@inquirer/prompts": { + "optional": true + }, + "@inquirer/rawlist": { + "optional": true + }, + "@inquirer/search": { + "optional": true + }, + "@inquirer/select": { + "optional": true + }, + "@types/jest": { + "optional": true + }, + "@types/node": { + "optional": true + }, + "jest": { + "optional": true + }, + "vitest": { + "optional": true + } + } + }, "node_modules/@inquirer/type": { "version": "4.0.7", "resolved": "https://registry.npmjs.org/@inquirer/type/-/type-4.0.7.tgz", @@ -1067,6 +1142,16 @@ "url": "https://opencollective.com/vitest" } }, + "node_modules/@xterm/headless": { + "version": "6.0.0", + "resolved": "https://registry.npmjs.org/@xterm/headless/-/headless-6.0.0.tgz", + "integrity": "sha512-5Yj1QINYCyzrZtf8OFIHi47iQtI+0qYFPHmouEfG8dHNxbZ9Tb9YGSuLcsEwj9Z+OL75GJqPyJbyoFer80a2Hw==", + "dev": true, + "license": "MIT", + "workspaces": [ + "addons/*" + ] + }, "node_modules/acorn": { "version": "8.18.0", "resolved": "https://registry.npmjs.org/acorn/-/acorn-8.18.0.tgz", @@ -2263,6 +2348,15 @@ "url": "https://github.com/sponsors/sindresorhus" } }, + "node_modules/mute-stream": { + "version": "3.0.0", + "resolved": "https://registry.npmjs.org/mute-stream/-/mute-stream-3.0.0.tgz", + "integrity": "sha512-dkEJPVvun4FryqBmZ5KhDo0K9iDXAwn08tMLDinNdRBNPcYEDiWYysLcc6k3mjTMlbP9KyylvRpd4wFtwrT9rw==", + "license": "ISC", + "engines": { + "node": "^20.17.0 || >=22.9.0" + } + }, "node_modules/nanoid": { "version": "3.3.16", "resolved": "https://registry.npmjs.org/nanoid/-/nanoid-3.3.16.tgz", diff --git a/package.json b/package.json index 855a683..913ab34 100644 --- a/package.json +++ b/package.json @@ -54,6 +54,7 @@ }, "devDependencies": { "@eslint/js": "^10.0.1", + "@inquirer/testing": "^3.3.9", "@types/node": "^22.19.4", "@types/semver": "^7.3.9", "eslint": "^10.8.0", diff --git a/src/config/profile/add.test.ts b/src/config/profile/add.test.ts new file mode 100644 index 0000000..6cbec68 --- /dev/null +++ b/src/config/profile/add.test.ts @@ -0,0 +1,105 @@ +// SPDX-License-Identifier: MIT +// SPDX-FileCopyrightText: 2026 Steadybit GmbH + +import fs from 'node:fs/promises'; +import os from 'node:os'; +import path from 'node:path'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { wrapPrompt } from '@inquirer/testing/vitest'; +import { answerPrompt, waitForPrompt } from '../../mocks/prompts.ts'; +import { add } from './add.ts'; +import type { Profile } from './types.ts'; + +vi.mock('@inquirer/input', async importOriginal => { + const actual = await importOriginal(); + return { ...actual, default: wrapPrompt(actual.default) }; +}); +vi.mock('@inquirer/password', async importOriginal => { + const actual = await importOriginal(); + return { ...actual, default: wrapPrompt(actual.default) }; +}); + +// The flow writes a real profile store, so HOME points at a scratch directory. +const fakeHome = await fs.mkdtemp(path.join(os.tmpdir(), 'steadybit-add-test-')); +process.env.HOME = fakeHome; +if (!os.homedir().startsWith(fakeHome)) { + throw new Error(`refusing to run: home directory is ${os.homedir()}, not the scratch directory`); +} + +async function storedProfiles(): Promise { + return JSON.parse(await fs.readFile(path.join(fakeHome, '.steadybit', 'profiles.json'), 'utf8')); +} + +describe('config profile add', () => { + beforeEach(() => { + // Otherwise the flow wipes the test runner's output. + vi.spyOn(console, 'clear').mockImplementation(() => undefined); + vi.spyOn(console, 'log').mockImplementation(() => undefined); + }); + + it('should ask for a name, a base url and a token, and store them', async () => { + const done = add({} as never); + + await answerPrompt('Profile name:', 'from-the-prompt'); + await answerPrompt('Base URL of the Steadybit server:', 'https://platform.example.com'); + await answerPrompt('API access token:', 's3cr3t'); + await done; + + expect(await storedProfiles()).toContainEqual({ + name: 'from-the-prompt', + baseUrl: 'https://platform.example.com', + apiAccessToken: 's3cr3t', + }); + }); + + it('should fall back to the public platform when the base url is left empty', async () => { + const done = add({} as never); + + await answerPrompt('Profile name:', 'defaulted'); + await answerPrompt('Base URL of the Steadybit server:', ''); // accept the offered default + await answerPrompt('API access token:', 'tok'); + await done; + + expect((await storedProfiles()).find(p => p.name === 'defaulted')?.baseUrl).toEqual( + 'https://platform.steadybit.com' + ); + }); + + it('should refuse a blank name and ask again', async () => { + const done = add({} as never); + + await answerPrompt('Profile name:', ' '); + await waitForPrompt('You must provide a valid value'); + + await answerPrompt('Profile name:', 'eventually-valid', { replace: true }); + await answerPrompt('Base URL of the Steadybit server:', ''); + await answerPrompt('API access token:', 'tok'); + await done; + + expect((await storedProfiles()).map(p => p.name)).toContain('eventually-valid'); + }); + + it('should refuse a base url that is not http', async () => { + const done = add({} as never); + + await answerPrompt('Profile name:', 'bad-url'); + await answerPrompt('Base URL of the Steadybit server:', 'ftp://files.example.com'); + await waitForPrompt('Unsupported protocol ftp:'); + + await answerPrompt('Base URL of the Steadybit server:', 'https://platform.example.com', { replace: true }); + await answerPrompt('API access token:', 'tok'); + await done; + + expect((await storedProfiles()).map(p => p.name)).toContain('bad-url'); + }); + + it('should skip the questions entirely when name and token are given', async () => { + await add({ name: 'non-interactive', token: 'tok', baseUrl: 'https://given.example.com' }); + + expect(await storedProfiles()).toContainEqual({ + name: 'non-interactive', + baseUrl: 'https://given.example.com', + apiAccessToken: 'tok', + }); + }); +}); diff --git a/src/config/profile/select.test.ts b/src/config/profile/select.test.ts new file mode 100644 index 0000000..63e99a5 --- /dev/null +++ b/src/config/profile/select.test.ts @@ -0,0 +1,53 @@ +// SPDX-License-Identifier: MIT +// SPDX-FileCopyrightText: 2026 Steadybit GmbH + +import fs from 'node:fs/promises'; +import os from 'node:os'; +import path from 'node:path'; +import { beforeAll, describe, expect, it, vi } from 'vitest'; +import { wrapPrompt } from '@inquirer/testing/vitest'; +import { answerPrompt, waitForPrompt } from '../../mocks/prompts.ts'; +import { addProfile } from './service.ts'; +import { select } from './select.ts'; + +vi.mock('@inquirer/select', async importOriginal => { + const actual = await importOriginal(); + return { ...actual, default: wrapPrompt(actual.default) }; +}); + +const fakeHome = await fs.mkdtemp(path.join(os.tmpdir(), 'steadybit-select-test-')); +process.env.HOME = fakeHome; +if (!os.homedir().startsWith(fakeHome)) { + throw new Error(`refusing to run: home directory is ${os.homedir()}, not the scratch directory`); +} + +const activeProfileFile = path.join(fakeHome, '.steadybit', 'activeProfile'); + +describe('config profile select', () => { + beforeAll(async () => { + await addProfile({ name: 'alpha', apiAccessToken: 'a' }); + await addProfile({ name: 'beta', apiAccessToken: 'b' }); + }); + + it('should offer every configured profile', async () => { + const done = select(); + + await waitForPrompt('Choose the new active profile:'); + await waitForPrompt('alpha'); + await waitForPrompt('beta'); + + await answerPrompt('Choose the new active profile:', ''); + await done; + }); + + it('should make the chosen profile the active one', async () => { + const done = select(); + + await waitForPrompt('Choose the new active profile:'); + // Down to the second entry, then accept. + await answerPrompt('Choose the new active profile:', '\x1b[B'); + await done; + + expect(await fs.readFile(activeProfileFile, 'utf8')).toEqual('beta'); + }); +}); diff --git a/src/mocks/prompts.ts b/src/mocks/prompts.ts new file mode 100644 index 0000000..e43388e --- /dev/null +++ b/src/mocks/prompts.ts @@ -0,0 +1,34 @@ +// SPDX-License-Identifier: MIT +// SPDX-FileCopyrightText: 2026 Steadybit GmbH + +import { screen } from '@inquirer/testing/vitest'; + +// A prompt is not always on screen the moment the call under test starts: confirm() +// reaches its prompt through a dynamic import, and a rejected answer re-renders the +// same question. Polling for the text covers both without the caller having to know +// which case it is in, and reports the screen it did see when the wait runs out. +export async function waitForPrompt(text: string, timeoutMillis = 4000): Promise { + const deadline = Date.now() + timeoutMillis; + while (Date.now() < deadline) { + if (screen.getScreen().includes(text)) { + return; + } + await new Promise(resolve => setTimeout(resolve, 5)); + } + throw new Error(`Prompt "${text}" never appeared. Last screen was:\n${screen.getScreen()}`); +} + +// Enter is a carriage return; readline does not submit on a newline. `replace` clears +// what is already in the field first, which a prompt keeps after rejecting an answer — +// without it a second attempt is appended to the first rather than replacing it. +export async function answerPrompt(prompt: string, answer: string, { replace = false } = {}): Promise { + await waitForPrompt(prompt); + if (replace) { + screen.input.write('\x7f'.repeat(64)); // backspace past anything already typed + } + screen.input.write(`${answer}\r`); +} + +export function pressCtrlC(): void { + screen.input.write(''); +} diff --git a/src/prompt/cancellation.test.ts b/src/prompt/cancellation.test.ts new file mode 100644 index 0000000..51e4877 --- /dev/null +++ b/src/prompt/cancellation.test.ts @@ -0,0 +1,49 @@ +// SPDX-License-Identifier: MIT +// SPDX-FileCopyrightText: 2026 Steadybit GmbH + +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { wrapPrompt } from '@inquirer/testing/vitest'; +import { pressCtrlC, waitForPrompt } from '../mocks/prompts.ts'; +import { confirm } from './confirm.ts'; +import { cancelable } from './cancellation.ts'; + +vi.mock('@inquirer/confirm', async importOriginal => { + const actual = await importOriginal(); + return { ...actual, default: wrapPrompt(actual.default) }; +}); + +describe('cancelable', () => { + beforeEach(() => { + Object.defineProperty(process.stdout, 'isTTY', { value: true, configurable: true }); + }); + + afterEach(() => { + Object.defineProperty(process.stdout, 'isTTY', { value: undefined, configurable: true }); + vi.restoreAllMocks(); + }); + + // inquirer used to re-raise SIGINT; the @inquirer prompts reject instead, which would + // otherwise surface as an unhandled rejection with a stack trace. Previously this was + // only covered by throwing a hand-made ExitPromptError, never by a real cancellation. + it('should exit quietly with the conventional SIGINT status when the user cancels', async () => { + const exit = vi.spyOn(process, 'exit').mockImplementation((() => undefined) as never); + + // process.exit is stubbed, so cancelable() falls through to its rethrow instead of + // ending the process. In production it never gets that far. + void confirm('Run it?').catch(() => undefined); + await waitForPrompt('Run it?'); + pressCtrlC(); + + await vi.waitFor(() => expect(exit).toHaveBeenCalledWith(130)); + }); + + it('should let every other failure through untouched', async () => { + const boom = new Error('something else'); + + await expect(cancelable(Promise.reject(boom))).rejects.toBe(boom); + }); + + it('should pass a normal answer straight through', async () => { + await expect(cancelable(Promise.resolve('answered'))).resolves.toEqual('answered'); + }); +}); diff --git a/src/prompt/confirm.test.ts b/src/prompt/confirm.test.ts new file mode 100644 index 0000000..4395406 --- /dev/null +++ b/src/prompt/confirm.test.ts @@ -0,0 +1,53 @@ +// SPDX-License-Identifier: MIT +// SPDX-FileCopyrightText: 2026 Steadybit GmbH + +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { wrapPrompt } from '@inquirer/testing/vitest'; +import { answerPrompt, waitForPrompt } from '../mocks/prompts.ts'; +import { confirm } from './confirm.ts'; + +// confirm() reaches the prompt through a dynamic import, so the mock has to survive +// `await import(...)` rather than only a static one. +vi.mock('@inquirer/confirm', async importOriginal => { + const actual = await importOriginal(); + return { ...actual, default: wrapPrompt(actual.default) }; +}); + +describe('confirm', () => { + describe('without a terminal', () => { + it('should answer with the non-interactive default instead of prompting', async () => { + await expect(confirm('Run it?', { defaultWhenNonInteractive: false })).resolves.toBe(false); + await expect(confirm('Run it?', { defaultWhenNonInteractive: true })).resolves.toBe(true); + }); + }); + + describe('with a terminal', () => { + beforeEach(() => { + // The guard in confirm() decides whether to prompt at all. Faking it is the only + // way in-process; whether the guard reads the real terminal correctly is left to + // the container tests, which get a genuine tty from `docker run -t`. + Object.defineProperty(process.stdout, 'isTTY', { value: true, configurable: true }); + }); + + afterEach(() => { + Object.defineProperty(process.stdout, 'isTTY', { value: undefined, configurable: true }); + }); + + it('should take the answer the user gives', async () => { + const answer = confirm('Run it?', { defaultYes: false }); + + await answerPrompt('Run it?', 'y'); + + await expect(answer).resolves.toBe(true); + }); + + it('should offer the configured default', async () => { + const answer = confirm('Run it?', { defaultYes: false }); + + await waitForPrompt('(y/N)'); // the configured default is the one offered + await answerPrompt('Run it?', ''); + + await expect(answer).resolves.toBe(false); + }); + }); +}); From 07f4e18bcb8e6cdc41beaf09a11e58d91eaec26e Mon Sep 17 00:00:00 2001 From: Johannes Edmeier Date: Wed, 29 Jul 2026 20:50:50 +0200 Subject: [PATCH 3/6] test: add container smoke tests for what needs a real process MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four things no in-process test can reach: the exit status a pipeline reads, a real terminal, the spawn of a subcommand, and the artifact users actually install. The vitest suite has to fake the first two and cannot do the last two at all — abortExecution deliberately returns instead of exiting under NODE_ENV=test, which is what makes command-level tests possible in the first place and also why they can never check an exit code. The tests run against the shipped image, so the npm tarball, the bin entry, the shebang and the runtime package.json lookup are exercised on the way. A separate Dockerfile adds expect to drive the prompts through a pty; nothing in it reaches the image users install. They stay at the level of "did it exit correctly" on purpose. Assertions about content belong at the command level, where a failure points at a line rather than a terminal transcript. CONTRIBUTING now describes the three levels and which one new tests belong in. --- .github/workflows/verify.yml | 15 +++++++ CONTRIBUTING.md | 35 ++++++++++++++++ Dockerfile.e2e | 12 ++++++ e2e/add-profile.exp | 10 +++++ e2e/cancel-profile.exp | 9 ++++ e2e/colour-on-tty.exp | 13 ++++++ e2e/run.sh | 80 ++++++++++++++++++++++++++++++++++++ 7 files changed, 174 insertions(+) create mode 100644 Dockerfile.e2e create mode 100644 e2e/add-profile.exp create mode 100644 e2e/cancel-profile.exp create mode 100644 e2e/colour-on-tty.exp create mode 100755 e2e/run.sh diff --git a/.github/workflows/verify.yml b/.github/workflows/verify.yml index 51fb4aa..5b83c47 100644 --- a/.github/workflows/verify.yml +++ b/.github/workflows/verify.yml @@ -41,3 +41,18 @@ jobs: tags: steadybit/cli:latest - name: Test container run: docker run --rm steadybit/cli:latest -V + # Everything these cover needs a real process: an exit status, a terminal, or the + # spawn of a subcommand. They run against the image users install, built from the + # npm tarball, so the packaging is exercised too. + - name: Build smoke test container + uses: docker/build-push-action@v7 + env: + DOCKER_BUILD_RECORD_UPLOAD: 'false' + with: + context: ./ + file: ./Dockerfile.e2e + build-args: IMAGE=steadybit/cli:latest + load: true + tags: steadybit/cli:smoke-test + - name: Run container smoke tests + run: docker run --rm steadybit/cli:smoke-test diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index b4e72d9..6fb4b84 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -13,6 +13,41 @@ npm ci Run `npm run ci` before pushing. It type-checks, tests, lints and builds, and is the same script CI runs. +## Tests + +Tests sit at three levels. Put a test at the lowest one that can hold it — the +levels get slower and harder to debug as you go down this list. + +| Level | Tool | Covers | +| --------- | ---------------------------------- | ------------------------------------------------------ | +| Unit | vitest | A single function or class, no I/O | +| Command | vitest + msw + `@inquirer/testing` | A command end to end in process, including its prompts | +| Container | `Dockerfile.e2e` + expect | Only what needs a real process | + +Prompts are driven through `@inquirer/testing`. Mock the prompt package with +`wrapPrompt` so the application's own call is intercepted, and use the helpers in +`src/mocks/prompts.ts` rather than writing to the screen directly: + +```ts +vi.mock('@inquirer/input', async importOriginal => { + const actual = await importOriginal(); + return { ...actual, default: wrapPrompt(actual.default) }; +}); + +await answerPrompt('Profile name:', 'my-profile'); +``` + +The container tests are deliberately thin. They exist for the four things no in-process +test can reach — real exit codes, a real terminal, the spawn of a subcommand, and +the packaged artifact — and they assert exit status and a line of output, never +content. Anything checking structure belongs at the command level. + +```sh +docker build -t steadybit/cli:latest . +docker build --build-arg IMAGE=steadybit/cli:latest -f Dockerfile.e2e -t steadybit/cli:smoke-test . +docker run --rm steadybit/cli:smoke-test +``` + ### Local CLI Execution ```sh diff --git a/Dockerfile.e2e b/Dockerfile.e2e new file mode 100644 index 0000000..9a4903c --- /dev/null +++ b/Dockerfile.e2e @@ -0,0 +1,12 @@ +# SPDX-License-Identifier: MIT +# SPDX-FileCopyrightText: 2026 Steadybit GmbH + +# Adds a pty allocator to the shipped image so the prompts can be driven. Kept apart +# from Dockerfile so that nothing here reaches the image users install. +ARG IMAGE=steadybit/cli:latest +FROM ${IMAGE} + +RUN apk add --no-cache expect + +COPY e2e /e2e +ENTRYPOINT ["sh", "/e2e/run.sh"] diff --git a/e2e/add-profile.exp b/e2e/add-profile.exp new file mode 100644 index 0000000..9557565 --- /dev/null +++ b/e2e/add-profile.exp @@ -0,0 +1,10 @@ +# Drives `config profile add` through a real pty. +log_user 0 +set timeout 20 +spawn steadybit config profile add +expect "Profile name:" { send "e2e\r" } +expect "Base URL" { send "https://platform.example.com\r" } +expect "API access token:" { send "s3cr3t\r" } +expect eof +catch wait result +exit [lindex $result 3] diff --git a/e2e/cancel-profile.exp b/e2e/cancel-profile.exp new file mode 100644 index 0000000..b2ec2dc --- /dev/null +++ b/e2e/cancel-profile.exp @@ -0,0 +1,9 @@ +# Ctrl+C at the first question must exit quietly with the SIGINT status. +log_user 0 +set timeout 20 +spawn steadybit config profile add +expect "Profile name:" +send "\003" +expect eof +catch wait result +exit [lindex $result 3] diff --git a/e2e/colour-on-tty.exp b/e2e/colour-on-tty.exp new file mode 100644 index 0000000..690d032 --- /dev/null +++ b/e2e/colour-on-tty.exp @@ -0,0 +1,13 @@ +# Colour is gated on stdout being a terminal, so the CLI has to be given one. Piping +# its output into a matcher would remove the very thing under test. +# +# The pattern is the escape character alone: expect matches globs by default, in which +# a "[" opens a character class rather than matching itself. +log_user 0 +set timeout 20 +spawn env STEADYBIT_TOKEN= steadybit experiment get -k ADM-1 +expect { + "\033" { catch { exp_close }; exit 0 } + eof { exit 1 } + timeout { exit 1 } +} diff --git a/e2e/run.sh b/e2e/run.sh new file mode 100755 index 0000000..f24d6e4 --- /dev/null +++ b/e2e/run.sh @@ -0,0 +1,80 @@ +#!/bin/sh +# SPDX-License-Identifier: MIT +# SPDX-FileCopyrightText: 2026 Steadybit GmbH + +# Smoke tests for the packaged CLI. Everything here needs a real process: an exit +# status, a terminal, or the spawn of a subcommand. Anything that can be asserted +# in-process belongs in the vitest suite instead, so these stay at the level of +# "did it exit correctly" rather than checking output in detail. + +set -u +failures=0 + +check() { + description=$1 + shift + if "$@"; then + echo " ok $description" + else + echo " FAIL $description" + failures=$((failures + 1)) + fi +} + +exits_with() { + expected=$1 + shift + "$@" >/dev/null 2>&1 + actual=$? + [ "$actual" -eq "$expected" ] || { + echo " expected exit $expected, got $actual" + return 1 + } +} + +echo "steadybit CLI container smoke tests" + +check "--version succeeds" exits_with 0 steadybit --version +check "--help succeeds" exits_with 0 steadybit --help +check "a subcommand is spawned and runs" exits_with 0 steadybit experiment --help +check "an unknown command fails" exits_with 1 steadybit definitely-not-a-command + +# The access token is resolved before anything else, so this is the guard on every +# platform-touching command. +check "a missing access token fails" exits_with 1 env STEADYBIT_TOKEN= steadybit experiment get -k ADM-1 +check "an unreachable platform fails" exits_with 1 \ + env STEADYBIT_TOKEN=t STEADYBIT_URL=http://127.0.0.1:1 steadybit experiment get -k ADM-1 + +# Colour is gated on stdout being a terminal. Both halves are checked here rather than +# depending on how the container was started: the pipe below is genuinely not a +# terminal, and expect genuinely provides one. +check "output is clean when piped" sh -c \ + '! env STEADYBIT_TOKEN= steadybit experiment get -k ADM-1 2>&1 | grep -q "$(printf "\033")"' +check "output is coloured on a terminal" expect /e2e/colour-on-tty.exp + +# The interactive flow, driven through a pty. Writes into the container's own home. +check "profile add stores what was typed" sh -c ' + expect /e2e/add-profile.exp >/dev/null 2>&1 || exit 1 + grep -q "\"name\": \"e2e\"" "$HOME/.steadybit/profiles.json" || exit 1 + steadybit config profile list | grep -q "e2e" +' + +check "ctrl-c during a prompt exits 130" sh -c ' + expect /e2e/cancel-profile.exp >/dev/null 2>&1 + [ $? -eq 130 ] +' + +check "ctrl-c leaves no stack trace and no profile" sh -c ' + rm -rf "$HOME/.steadybit" + out=$(expect /e2e/cancel-profile.exp 2>&1) + echo "$out" | grep -q "ExitPromptError" && exit 1 + [ ! -f "$HOME/.steadybit/profiles.json" ] +' + +echo +if [ "$failures" -eq 0 ]; then + echo "all container smoke tests passed" +else + echo "$failures container smoke test(s) failed" +fi +exit "$failures" From 898842883e1f13241e99a57900a914df58bb013a Mon Sep 17 00:00:00 2001 From: Johannes Edmeier Date: Wed, 29 Jul 2026 21:00:08 +0200 Subject: [PATCH 4/6] fix: run the container smoke tests against the build under test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI built the smoke-test image with `FROM steadybit/cli:latest`, and the buildx container driver cannot read the local image store, so that tag resolved from Docker Hub instead of from the image built moments earlier in the same job. The suite was exercising the last release. It reported two failures, both real for 4.3.6 — the colour gating and the SIGINT exit status are new here — and after the next release it would have gone green again while still testing a stale artifact, which is worse than failing. There is no derived image now. The scripts are mounted into the image that was built, and the pty allocator they need is installed at run time, so nothing resolves a tag on the way. CI additionally tags its build by commit rather than `latest`: a tag that cannot exist in a registry fails loudly instead of quietly finding something else. --- .github/workflows/verify.yml | 21 ++++++++------------- CONTRIBUTING.md | 13 +++++++++---- Dockerfile.e2e | 12 ------------ e2e/run.sh | 11 +++++++++++ 4 files changed, 28 insertions(+), 29 deletions(-) delete mode 100644 Dockerfile.e2e diff --git a/.github/workflows/verify.yml b/.github/workflows/verify.yml index 5b83c47..5089640 100644 --- a/.github/workflows/verify.yml +++ b/.github/workflows/verify.yml @@ -38,21 +38,16 @@ jobs: with: context: ./ load: true - tags: steadybit/cli:latest + # Tagged by commit, not `latest`. A tag that also exists on Docker Hub can be + # resolved from there instead of from this build without anything failing, so + # the smoke tests would quietly exercise the last release. + tags: steadybit/cli:ci-${{ github.sha }} - name: Test container - run: docker run --rm steadybit/cli:latest -V + run: docker run --rm steadybit/cli:ci-${{ github.sha }} -V # Everything these cover needs a real process: an exit status, a terminal, or the # spawn of a subcommand. They run against the image users install, built from the # npm tarball, so the packaging is exercised too. - - name: Build smoke test container - uses: docker/build-push-action@v7 - env: - DOCKER_BUILD_RECORD_UPLOAD: 'false' - with: - context: ./ - file: ./Dockerfile.e2e - build-args: IMAGE=steadybit/cli:latest - load: true - tags: steadybit/cli:smoke-test - name: Run container smoke tests - run: docker run --rm steadybit/cli:smoke-test + run: | + docker run --rm -v "$PWD/e2e:/e2e" --entrypoint sh \ + steadybit/cli:ci-${{ github.sha }} /e2e/run.sh diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 6fb4b84..ddf1195 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -22,7 +22,7 @@ levels get slower and harder to debug as you go down this list. | --------- | ---------------------------------- | ------------------------------------------------------ | | Unit | vitest | A single function or class, no I/O | | Command | vitest + msw + `@inquirer/testing` | A command end to end in process, including its prompts | -| Container | `Dockerfile.e2e` + expect | Only what needs a real process | +| Container | `e2e/run.sh` + expect | Only what needs a real process | Prompts are driven through `@inquirer/testing`. Mock the prompt package with `wrapPrompt` so the application's own call is intercepted, and use the helpers in @@ -43,11 +43,16 @@ the packaged artifact — and they assert exit status and a line of output, content. Anything checking structure belongs at the command level. ```sh -docker build -t steadybit/cli:latest . -docker build --build-arg IMAGE=steadybit/cli:latest -f Dockerfile.e2e -t steadybit/cli:smoke-test . -docker run --rm steadybit/cli:smoke-test +docker build -t steadybit/cli:under-test . +docker run --rm -v "$PWD/e2e:/e2e" --entrypoint sh steadybit/cli:under-test /e2e/run.sh ``` +The scripts are mounted into the image rather than baked into a derived one, and CI tags +the image it builds by commit rather than `latest`. Both guard the same mistake: a +`FROM steadybit/cli:latest` is resolved by tag, and under the buildx container driver +that tag resolves from Docker Hub rather than from the build under test — so the suite +passes while exercising the previous release. + ### Local CLI Execution ```sh diff --git a/Dockerfile.e2e b/Dockerfile.e2e deleted file mode 100644 index 9a4903c..0000000 --- a/Dockerfile.e2e +++ /dev/null @@ -1,12 +0,0 @@ -# SPDX-License-Identifier: MIT -# SPDX-FileCopyrightText: 2026 Steadybit GmbH - -# Adds a pty allocator to the shipped image so the prompts can be driven. Kept apart -# from Dockerfile so that nothing here reaches the image users install. -ARG IMAGE=steadybit/cli:latest -FROM ${IMAGE} - -RUN apk add --no-cache expect - -COPY e2e /e2e -ENTRYPOINT ["sh", "/e2e/run.sh"] diff --git a/e2e/run.sh b/e2e/run.sh index f24d6e4..a9fe7fd 100755 --- a/e2e/run.sh +++ b/e2e/run.sh @@ -10,6 +10,17 @@ set -u failures=0 +# Driving the prompts needs a pty allocator, which the shipped image has no reason to +# carry. Installed here rather than baked into a second image: a derived image has to +# name its base by tag, and a tag can silently resolve to something from a registry +# instead of the build under test. +if ! command -v expect >/dev/null 2>&1; then + apk add --no-cache expect >/dev/null 2>&1 || { + echo "cannot install expect, which the interactive checks need" + exit 1 + } +fi + check() { description=$1 shift From 96ab1cc1cbe46decf358218781f3551eb291903f Mon Sep 17 00:00:00 2001 From: Johannes Edmeier Date: Thu, 30 Jul 2026 07:41:01 +0200 Subject: [PATCH 5/6] test: close the gaps found reviewing the test strategy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four things, all found by reviewing what had just been written rather than by anything failing. The Ctrl+C helper held a literal 0x03 byte in its string. Editors and diffs render that as an empty string, so the next person to tidy it would have removed the only thing the test sends. It is an escape now, and nothing else in src or e2e carries an unprintable character. The memoisation followed the home directory for the directory it creates but not for the files it reads, so a changed home produced correct directories holding the previous home's contents. Reads, writes and the directory itself now go through one helper keyed the same way, which is also less code than the two mechanisms it replaces. Two container checks asserted only absences — no escape codes, no stack trace, no profile — and all of those hold when the CLI is missing entirely. Removing the binary used to leave them green; now every one of the eleven fails. The first attempt at a fix was still too weak, since `2>&1` catches the shell's own "not found" and made the output look present. The "an experiment is already running, run it in parallel?" recovery had no test at all. It was dead code until the response body it inspects stopped being consumed before it got there, and it has been reachable but unexercised since. It is now driven through the prompt, in both directions. --- e2e/cancel-profile.exp | 2 +- e2e/run.sh | 14 ++++- src/config/profile/service.test.ts | 22 ++++++++ src/config/profile/service.ts | 66 +++++++++++------------ src/experiment/exec.interactive.test.ts | 72 +++++++++++++++++++++++++ src/mocks/handlers.ts | 20 +++++++ src/mocks/prompts.ts | 4 +- 7 files changed, 160 insertions(+), 40 deletions(-) create mode 100644 src/experiment/exec.interactive.test.ts diff --git a/e2e/cancel-profile.exp b/e2e/cancel-profile.exp index b2ec2dc..2f5b2e3 100644 --- a/e2e/cancel-profile.exp +++ b/e2e/cancel-profile.exp @@ -2,7 +2,7 @@ log_user 0 set timeout 20 spawn steadybit config profile add -expect "Profile name:" +expect "Profile name:" { puts "REACHED_PROMPT" } send "\003" expect eof catch wait result diff --git a/e2e/run.sh b/e2e/run.sh index a9fe7fd..ce9647a 100755 --- a/e2e/run.sh +++ b/e2e/run.sh @@ -59,8 +59,15 @@ check "an unreachable platform fails" exits_with 1 \ # Colour is gated on stdout being a terminal. Both halves are checked here rather than # depending on how the container was started: the pipe below is genuinely not a # terminal, and expect genuinely provides one. -check "output is clean when piped" sh -c \ - '! env STEADYBIT_TOKEN= steadybit experiment get -k ADM-1 2>&1 | grep -q "$(printf "\033")"' +# Asserting only the absence of escapes would also hold if nothing were printed at all, +# so the output has to be there first. +check "output is clean when piped" sh -c ' + out=$(env STEADYBIT_TOKEN= steadybit experiment get -k ADM-1 2>&1) + # Something the CLI itself prints. Merely having output would also be satisfied by the + # shell reporting that there is no such command. + echo "$out" | grep -q "No API access token" || exit 1 + ! printf "%s" "$out" | grep -q "$(printf "\033")" +' check "output is coloured on a terminal" expect /e2e/colour-on-tty.exp # The interactive flow, driven through a pty. Writes into the container's own home. @@ -75,9 +82,12 @@ check "ctrl-c during a prompt exits 130" sh -c ' [ $? -eq 130 ] ' +# Same trap: no stack trace and no profile are both true of a CLI that never ran, so +# the prompt has to be shown to have been reached. check "ctrl-c leaves no stack trace and no profile" sh -c ' rm -rf "$HOME/.steadybit" out=$(expect /e2e/cancel-profile.exp 2>&1) + echo "$out" | grep -q "REACHED_PROMPT" || exit 1 echo "$out" | grep -q "ExitPromptError" && exit 1 [ ! -f "$HOME/.steadybit/profiles.json" ] ' diff --git a/src/config/profile/service.test.ts b/src/config/profile/service.test.ts index 8494952..a671458 100644 --- a/src/config/profile/service.test.ts +++ b/src/config/profile/service.test.ts @@ -76,4 +76,26 @@ describe('profile service', () => { process.env.HOME = previous; } }); + + // The directory memo followed HOME while the read memo did not, so a changed home + // produced correct directories with the previous home's contents in them. + it('should read from a home directory that changes after import', async () => { + const otherHome = await fs.mkdtemp(path.join(os.tmpdir(), 'steadybit-other-home-')); + await fs.mkdir(path.join(otherHome, '.steadybit'), { recursive: true }); + await fs.writeFile( + path.join(otherHome, '.steadybit', 'profiles.json'), + JSON.stringify([{ name: 'only-over-here', apiAccessToken: 'z' }]) + ); + + const previous = process.env.HOME; + process.env.HOME = otherHome; + try { + expect((await getProfiles()).map(p => p.name)).toEqual(['only-over-here']); + } finally { + process.env.HOME = previous; + } + + // ...and the original home is still answered correctly afterwards. + expect((await getProfiles()).map(p => p.name)).toContain('first'); + }); }); diff --git a/src/config/profile/service.ts b/src/config/profile/service.ts index 648fc83..346c578 100644 --- a/src/config/profile/service.ts +++ b/src/config/profile/service.ts @@ -16,44 +16,38 @@ const profilesFile = () => path.join(configDir(), 'profiles.json'); const activeProfileFile = () => path.join(configDir(), 'activeProfile'); // The profile files are read on every API call, three times per call via -// getConfiguration(). Reading them once per process turns thousands of redundant -// syscalls into a handful during commands like `experiment dump`. Failures are not -// cached, so they stay retryable, and the writers below drop the cache. -function readOnce(read: () => Promise): (() => Promise) & { forget: () => void } { - let cached: Promise | undefined; - const cachingRead = () => { - cached ??= read().catch(e => { - cached = undefined; - throw e; - }); - return cached; +// getConfiguration(). Doing the work once turns thousands of redundant syscalls into a +// handful during commands like `experiment dump`. +// +// Remembered against the config directory rather than outright, because that directory +// follows HOME: a single cached value would answer for whichever home happened to be +// current first, and for the directory-creation entry that meant leaving a later +// directory unmade while every write into it failed. Failures are not remembered, so an +// unreadable or unwritable home stays retryable, and the writers below forget the entry +// for the home they wrote to. +function oncePerConfigDirectory(work: () => Promise): (() => Promise) & { forget: () => void } { + const done = new Map>(); + const runOnce = () => { + const directory = configDir(); + let result = done.get(directory); + if (!result) { + result = work().catch(e => { + done.delete(directory); + throw e; + }); + done.set(directory, result); + } + return result; }; - cachingRead.forget = () => { - cached = undefined; + runOnce.forget = () => { + done.delete(configDir()); }; - return cachingRead; + return runOnce; } -// Keyed by directory rather than memoised outright: the path follows HOME, and caching -// a single "already created" flag would leave a later directory unmade while the writes -// into it fail. Failures are not cached, so an unwritable home stays retryable. -const createdDirectories = new Map>(); - -function ensureConfigDirectoryExists(): Promise { - const directory = configDir(); - let created = createdDirectories.get(directory); - if (!created) { - created = fs.mkdir(directory, { recursive: true }).then( - () => undefined, - e => { - createdDirectories.delete(directory); - throw e; - } - ); - createdDirectories.set(directory, created); - } - return created; -} +const ensureConfigDirectoryExists = oncePerConfigDirectory(async () => { + await fs.mkdir(configDir(), { recursive: true }); +}); export async function addProfile(profile: Profile): Promise { const profiles = await getProfiles(); @@ -71,7 +65,7 @@ export async function removeProfile(profileName: string): Promise { await writeProfiles(updatedProfiles); } -const readProfiles = readOnce(async (): Promise => { +const readProfiles = oncePerConfigDirectory(async (): Promise => { await ensureConfigDirectoryExists(); let fileContent: string; @@ -107,7 +101,7 @@ async function writeProfiles(profiles: Profile[]): Promise { readProfiles.forget(); } -const readActiveProfileName = readOnce(async (): Promise => { +const readActiveProfileName = oncePerConfigDirectory(async (): Promise => { await ensureConfigDirectoryExists(); try { diff --git a/src/experiment/exec.interactive.test.ts b/src/experiment/exec.interactive.test.ts new file mode 100644 index 0000000..1e37ade --- /dev/null +++ b/src/experiment/exec.interactive.test.ts @@ -0,0 +1,72 @@ +// SPDX-License-Identifier: MIT +// SPDX-FileCopyrightText: 2026 Steadybit GmbH + +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { wrapPrompt } from '@inquirer/testing/vitest'; +import { answerPrompt } from '../mocks/prompts.ts'; +import { givenAnotherExperimentIsRunning } from '../mocks/handlers.ts'; +import { executeExperiments } from './exec.ts'; + +vi.mock('@inquirer/confirm', async importOriginal => { + const actual = await importOriginal(); + return { ...actual, default: wrapPrompt(actual.default) }; +}); + +// These go through the prompts rather than past them. exec.test.ts covers the same +// commands with no terminal attached, which is what CI does and what the +// defaultWhenNonInteractive values are about; both branches matter. +describe('experiment run, with a terminal', () => { + let logged: string[]; + + beforeEach(() => { + Object.defineProperty(process.stdout, 'isTTY', { value: true, configurable: true }); + logged = []; + vi.spyOn(console, 'log').mockImplementation((...args) => { + logged.push(args.join(' ')); + }); + }); + + afterEach(() => { + Object.defineProperty(process.stdout, 'isTTY', { value: undefined, configurable: true }); + vi.restoreAllMocks(); + }); + + it('should not run anything when the confirmation is declined', async () => { + // Throwing rather than returning, so that the flow stops here the way a real exit + // would. A no-op stub lets execution carry on and run the experiment anyway. + const exit = vi.spyOn(process, 'exit').mockImplementation(code => { + throw new Error(`exited with ${code}`); + }); + + const done = executeExperiments({ key: 'TST-1', recursive: false }); + await answerPrompt('Are you sure you want to run the experiment?', 'n'); + + await expect(done).rejects.toThrow('exited with 0'); + expect(exit).toHaveBeenCalledWith(0); + expect(logged.join('\n')).not.toContain('Executing experiment'); + }); + + // This recovery was unreachable until the response body it inspects stopped being + // consumed before it got there, so it has never been exercised end to end. + it('should offer to run in parallel when another experiment is already running', async () => { + givenAnotherExperimentIsRunning(); + + const done = executeExperiments({ key: 'TST-1', recursive: false }); + await answerPrompt('Are you sure you want to run the experiment?', 'y'); + await answerPrompt('There is already an experiment running', 'y'); + await done; + + expect(logged.join('\n')).toContain('Executing experiment: TST-1'); + }); + + it('should give up when running in parallel is declined', async () => { + givenAnotherExperimentIsRunning(); + + const done = executeExperiments({ key: 'TST-1', recursive: false }); + await answerPrompt('Are you sure you want to run the experiment?', 'y'); + await answerPrompt('There is already an experiment running', 'n'); + + await expect(done).rejects.toThrow('Failed to run experiment (TST-1)'); + expect(logged.join('\n')).not.toContain('Executing experiment'); + }); +}); diff --git a/src/mocks/handlers.ts b/src/mocks/handlers.ts index fd2be22..61dc33b 100644 --- a/src/mocks/handlers.ts +++ b/src/mocks/handlers.ts @@ -12,6 +12,13 @@ let experimentStore: Record = {}; let validationFailuresRemaining = 0; let executionsPerExperiment: Record = {}; let unfetchableExecutions = new Set(); +let anotherExperimentRunning = false; + +// Lets a test reach the "an experiment is already running, run it in parallel?" recovery, +// which the platform only offers when the caller has not already asked for parallel. +export const givenAnotherExperimentIsRunning = () => { + anotherExperimentRunning = true; +}; function executionsFor(key: string): { id: number }[] { return (executionsPerExperiment[key] ?? []).map(id => ({ id })); @@ -31,6 +38,7 @@ export const resetExperiments = () => { validationFailuresRemaining = 0; executionsPerExperiment = {}; unfetchableExecutions = new Set(); + anotherExperimentRunning = false; }; export const setValidationFailures = (count: number) => { @@ -186,6 +194,18 @@ const executeExperimentHandler = http.post('http://example.com/api/experiments/: ); } + if (anotherExperimentRunning && requestUrl.searchParams.get('allowParallel') !== 'true') { + return HttpResponse.json( + { + type: 'https://steadybit.com/problems/another-experiment-running-exception', + title: 'Another experiment is currently running.', + status: 409, + instance: `/api/experiments/${params.key}/execute`, + }, + { status: 409 } + ); + } + const run = runSequence++; if (experiment) { return HttpResponse.json( diff --git a/src/mocks/prompts.ts b/src/mocks/prompts.ts index e43388e..529cb74 100644 --- a/src/mocks/prompts.ts +++ b/src/mocks/prompts.ts @@ -30,5 +30,7 @@ export async function answerPrompt(prompt: string, answer: string, { replace = f } export function pressCtrlC(): void { - screen.input.write(''); + // Written as an escape rather than the literal byte, which is invisible in an editor + // and reads as an empty string. + screen.input.write('\x03'); } From e079116bd7603f8276c3075acd7b1df2c2fac02a Mon Sep 17 00:00:00 2001 From: Johannes Edmeier Date: Thu, 30 Jul 2026 08:29:00 +0200 Subject: [PATCH 6/6] ci: run the container by image id rather than by name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `docker run` pulls a name it cannot find locally, so steadybit/cli:latest would quietly fall back to the published release if this build ever stopped being loaded — someone switching to push, adding platforms, which disables load, or reordering the steps. That is the same wrong-artifact failure the smoke tests already hit once, arriving through a different door. The build action reports the id of what it just built, and an id is never looked up in a registry: a missing one is "No such image" rather than a pull. That removes the failure rather than making it loud, and the tag it replaces was only ever there to be run. --- .github/workflows/verify.yml | 12 ++++++------ CONTRIBUTING.md | 9 ++++----- 2 files changed, 10 insertions(+), 11 deletions(-) diff --git a/.github/workflows/verify.yml b/.github/workflows/verify.yml index 5089640..4a33084 100644 --- a/.github/workflows/verify.yml +++ b/.github/workflows/verify.yml @@ -32,22 +32,22 @@ jobs: - name: Set up Docker Buildx uses: docker/setup-buildx-action@v4 - name: Build container + id: build uses: docker/build-push-action@v7 env: DOCKER_BUILD_RECORD_UPLOAD: 'false' with: context: ./ load: true - # Tagged by commit, not `latest`. A tag that also exists on Docker Hub can be - # resolved from there instead of from this build without anything failing, so - # the smoke tests would quietly exercise the last release. - tags: steadybit/cli:ci-${{ github.sha }} + # By image id rather than a tag. `docker run` pulls a tag it cannot find locally, + # so a name like steadybit/cli:latest would quietly fall back to the last release + # if this build ever stopped being loaded. An id is never looked up in a registry. - name: Test container - run: docker run --rm steadybit/cli:ci-${{ github.sha }} -V + run: docker run --rm ${{ steps.build.outputs.imageid }} -V # Everything these cover needs a real process: an exit status, a terminal, or the # spawn of a subcommand. They run against the image users install, built from the # npm tarball, so the packaging is exercised too. - name: Run container smoke tests run: | docker run --rm -v "$PWD/e2e:/e2e" --entrypoint sh \ - steadybit/cli:ci-${{ github.sha }} /e2e/run.sh + ${{ steps.build.outputs.imageid }} /e2e/run.sh diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index ddf1195..edeadcd 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -47,11 +47,10 @@ docker build -t steadybit/cli:under-test . docker run --rm -v "$PWD/e2e:/e2e" --entrypoint sh steadybit/cli:under-test /e2e/run.sh ``` -The scripts are mounted into the image rather than baked into a derived one, and CI tags -the image it builds by commit rather than `latest`. Both guard the same mistake: a -`FROM steadybit/cli:latest` is resolved by tag, and under the buildx container driver -that tag resolves from Docker Hub rather than from the build under test — so the suite -passes while exercising the previous release. +The scripts are mounted into the image rather than baked into a derived one, and CI runs +the image by id rather than by name. Both avoid the same mistake: a name is resolved +against a registry when it cannot be found locally, so the suite can end up exercising +the last release while reporting success. ### Local CLI Execution