diff --git a/src/commands/init.ts b/src/commands/init.ts index 87f02ceb0..8b34d94b8 100644 --- a/src/commands/init.ts +++ b/src/commands/init.ts @@ -12,7 +12,13 @@ import { useYesNoConfirm } from '../lib/hooks/user-confirmations/useYesNoConfirm import { createPrefilledInputFileFromInputSchema } from '../lib/input_schema.js'; import { error, info, success, warning } from '../lib/outputs.js'; import { wrapScrapyProject } from '../lib/projects/scrapy/wrapScrapyProject.js'; -import { sanitizeActorName, setLocalConfig, setLocalEnv, validateActorName } from '../lib/utils.js'; +import { + sanitizeActorName, + setLocalConfig, + setLocalEnv, + validateActorName, + validateActorNameForPrompt, +} from '../lib/utils.js'; export class InitCommand extends ApifyCommand { static override name = 'init' as const; @@ -66,6 +72,11 @@ export class InitCommand extends ApifyCommand { let { actorName } = this.args; const cwd = process.cwd(); + // Nothing downstream validates an explicit name, so an unusable one only surfaced on the next push. + if (actorName) { + validateActorName(actorName); + } + const projectResult = await useCwdProject(); // TODO: use direct .unwrap() once we migrate to yargs @@ -82,14 +93,15 @@ export class InitCommand extends ApifyCommand { } } - let defaultActorName = basename(cwd); + let actorNameSource = basename(cwd); if (project.type === ProjectLanguage.Python && project.entrypoint?.path) { const entryPath = project.entrypoint.path; // Extract the actual package name (last segment of dotted path) - const packageName = entryPath.includes('.') ? entryPath.split('.').pop()! : entryPath; - defaultActorName = sanitizeActorName(packageName); + actorNameSource = entryPath.includes('.') ? entryPath.split('.').pop()! : entryPath; } + const defaultActorName = sanitizeActorName(actorNameSource); + if (project.type === ProjectLanguage.Scrapy) { info({ message: 'The current directory looks like a Scrapy project. Using automatic project wrapping.' }); this.telemetryData.actorWrapper = 'scrapy'; @@ -102,7 +114,7 @@ export class InitCommand extends ApifyCommand { const confirmed = await useYesNoConfirm({ message: 'Do you want to continue?', - providedConfirmFromStdin: this.flags.yes, + errorMessageForStdin: 'Confirmation is required to continue. Pass --yes to skip the prompt.', }); if (!confirmed) { @@ -124,25 +136,33 @@ export class InitCommand extends ApifyCommand { } if (!actorName) { - let response = actorConfig.isOkAnd((cfg) => cfg.exists) ? (actorConfig.unwrap().config.name as string) : null; - - // TODO: see if we can use promptActorName instead - while (!response) { + const existingName = actorConfig.isOkAnd((cfg) => cfg.exists) + ? (actorConfig.unwrap().config.name as string) + : null; + + if (existingName) { + actorName = existingName; + } else if (this.flags.yes) { + // Sanitizing a name of only dashes or non-ASCII leaves nothing behind. try { - const answer = await useUserInput({ - message: 'Actor name:', - default: defaultActorName, - }); - - validateActorName(answer); - - response = answer; - } catch (err) { - error({ message: (err as Error).message }); + validateActorName(defaultActorName); + } catch { + throw new Error( + `Cannot derive an Actor name from '${actorNameSource}'. Run 'apify init ' to set it.`, + ); } - } - actorName = response; + actorName = defaultActorName; + } else { + // Validating inside the prompt lets inquirer re-ask on a typo. + actorName = await useUserInput({ + message: 'Actor name:', + default: defaultActorName, + validate: validateActorNameForPrompt, + errorMessageForStdin: + "Actor name is required. Run 'apify init ', or pass --yes to use the current directory name.", + }); + } } // Migrate apify.json to .actor/actor.json diff --git a/src/lib/create-utils.ts b/src/lib/create-utils.ts index 867ec8dc9..a5d463532 100644 --- a/src/lib/create-utils.ts +++ b/src/lib/create-utils.ts @@ -20,7 +20,7 @@ import { } from './templates/consts.js'; import { getTemplateRecommendation } from './templates/getTemplateRecommendation.js'; import { buildTemplateChoiceList, NON_EXACT_SEPARATOR_LABEL } from './templates/templateChoices.js'; -import { validateActorName } from './utils.js'; +import { validateActorName, validateActorNameForPrompt } from './utils.js'; /** Filters coming from the `--use-case` / `--language` flags. `undefined` means "not provided". */ export interface TemplateFilters { @@ -165,14 +165,7 @@ async function executePrompts(manifest: Manifest, filters: TemplateFilters) { async function promptActorName() { const answer = await useUserInput({ message: 'Name of your new Actor:', - validate: (promptText) => { - try { - validateActorName(promptText); - } catch (err) { - return (err as Error).message; - } - return true; - }, + validate: validateActorNameForPrompt, }); return answer; diff --git a/src/lib/utils.ts b/src/lib/utils.ts index 56524e708..06ec18183 100644 --- a/src/lib/utils.ts +++ b/src/lib/utils.ts @@ -680,6 +680,17 @@ export const validateActorName = (actorName: string) => { } }; +/** Adapts {@link validateActorName} to the `validate` contract of the inquirer prompts. */ +export const validateActorNameForPrompt = (actorName: string) => { + try { + validateActorName(actorName); + } catch (err) { + return (err as Error).message; + } + + return true; +}; + export const sanitizeActorName = (actorName: string) => { let sanitizedName = actorName.replaceAll(/[^a-zA-Z0-9-]/g, '-'); diff --git a/test/e2e/__helpers__/run-cli-bounded.ts b/test/e2e/__helpers__/run-cli-bounded.ts new file mode 100644 index 000000000..238f11254 --- /dev/null +++ b/test/e2e/__helpers__/run-cli-bounded.ts @@ -0,0 +1,96 @@ +import { spawn } from 'node:child_process'; +import { mkdir } from 'node:fs/promises'; +import process from 'node:process'; + +import { CliTestEnv, DistActor, DistApify } from './run-cli.js'; + +export interface RunCliBoundedOptions { + cwd: string; + /** Wall-clock budget. The child is killed once it elapses. */ + timeoutMs?: number; + /** Combined byte budget for stdout and stderr. The child is killed once it is passed. */ + outputCapBytes?: number; +} + +export interface RunCliBoundedResult { + stdout: string; + stderr: string; + exitCode: number | null; + /** The command was still running when `timeoutMs` elapsed. */ + timedOut: boolean; + /** The command wrote past `outputCapBytes`, which only a runaway loop does. */ + outputCapExceeded: boolean; + /** Bytes written across both streams, up to the point the child was killed. */ + bytesWritten: number; +} + +const DefaultTimeoutMs = 15_000; + +/** Two orders of magnitude above any command's real output, and a few milliseconds of a loop. */ +const DefaultOutputCapBytes = 256 * 1024; + +/** + * Run a built CLI command (requires `pnpm run build`) with stdin pointed at `/dev/null`. + * + * A deadline alone cannot tell a hang from a retry loop, and costs the full deadline either way. + * The output cap separates them in milliseconds: `outputCapExceeded` means the command retried + * something it can never succeed at. Use `runCli` for everything that does not prompt. + */ +export async function runCliBounded( + binary: 'apify' | 'actor', + args: string[], + options: RunCliBoundedOptions, +): Promise { + const { cwd, timeoutMs = DefaultTimeoutMs, outputCapBytes = DefaultOutputCapBytes } = options; + + await mkdir(cwd, { recursive: true }); + + const child = spawn(process.execPath, [binary === 'actor' ? DistActor : DistApify, ...args], { + cwd, + stdio: ['ignore', 'pipe', 'pipe'], + // `spawn` replaces the environment wholesale, unlike execa's `extendEnv`. + env: { + ...process.env, + ...CliTestEnv, + }, + }); + + const captured = { stdout: [] as Buffer[], stderr: [] as Buffer[] }; + let bytesWritten = 0; + let outputCapExceeded = false; + let timedOut = false; + + const capture = (stream: 'stdout' | 'stderr') => (chunk: Buffer) => { + bytesWritten += chunk.length; + + // Keep the head only: the tail after the cap is unbounded and carries nothing the assertions read. + if (!outputCapExceeded) captured[stream].push(chunk); + + if (bytesWritten > outputCapBytes && !outputCapExceeded) { + outputCapExceeded = true; + child.kill('SIGKILL'); + } + }; + + child.stdout.on('data', capture('stdout')); + child.stderr.on('data', capture('stderr')); + + const timer = setTimeout(() => { + timedOut = true; + child.kill('SIGKILL'); + }, timeoutMs); + + const exitCode = await new Promise((resolve, reject) => { + child.once('error', reject); + child.once('close', resolve); + }).finally(() => clearTimeout(timer)); + + return { + stdout: Buffer.concat(captured.stdout).toString('utf8'), + stderr: Buffer.concat(captured.stderr).toString('utf8'), + exitCode, + timedOut, + outputCapExceeded, + bytesWritten, + }; +} diff --git a/test/e2e/__helpers__/run-cli.ts b/test/e2e/__helpers__/run-cli.ts index 3915832f8..746bb9a25 100644 --- a/test/e2e/__helpers__/run-cli.ts +++ b/test/e2e/__helpers__/run-cli.ts @@ -4,8 +4,18 @@ import { fileURLToPath } from 'node:url'; import { execa } from 'execa'; const ProjectRoot = new URL('../../../', import.meta.url); -const DistApify = fileURLToPath(new URL('./dist/apify.js', ProjectRoot)); -const DistActor = fileURLToPath(new URL('./dist/actor.js', ProjectRoot)); +export const DistApify = fileURLToPath(new URL('./dist/apify.js', ProjectRoot)); +export const DistActor = fileURLToPath(new URL('./dist/actor.js', ProjectRoot)); + +export const CliTestEnv = { + APIFY_CLI_DISABLE_TELEMETRY: '1', + APIFY_CLI_SKIP_UPDATE_CHECK: '1', + APIFY_CLI_SKIP_RENTAL_SUNSET_NOTICE: '1', + // Pin the file backend so e2e subprocesses don't share the host's OS keyring across tests. + APIFY_DISABLE_KEYRING: '1', + // The resolver prefers APIFY_TOKEN, so a token on the host would pick the account. + APIFY_TOKEN: '', +}; export interface RunCliOptions { cwd?: string | URL; @@ -48,13 +58,7 @@ export async function runCli( stdin: options.stdin ? 'pipe' : 'ignore', input: options.stdin, env: { - APIFY_CLI_DISABLE_TELEMETRY: '1', - APIFY_CLI_SKIP_UPDATE_CHECK: '1', - APIFY_CLI_SKIP_RENTAL_SUNSET_NOTICE: '1', - // Pin the file backend so e2e subprocesses don't share the host's OS keyring across tests. - APIFY_DISABLE_KEYRING: '1', - // The resolver prefers APIFY_TOKEN, so a token on the host would pick the account. - APIFY_TOKEN: '', + ...CliTestEnv, ...options.env, }, }); diff --git a/test/e2e/commands/init-non-tty.test.ts b/test/e2e/commands/init-non-tty.test.ts new file mode 100644 index 000000000..8513182a8 --- /dev/null +++ b/test/e2e/commands/init-non-tty.test.ts @@ -0,0 +1,82 @@ +import { randomBytes } from 'node:crypto'; +import { access, mkdir, readFile, rm, writeFile } from 'node:fs/promises'; +import path from 'node:path'; + +import { runCliBounded } from '../__helpers__/run-cli-bounded.js'; +import { TestTmpRoot } from '../__helpers__/tmp.js'; + +const MissingNameError = 'Error: Actor name is required'; + +function countOccurrences(haystack: string, needle: string) { + return haystack.split(needle).length - 1; +} + +async function makeNodeProject(dir: string) { + await mkdir(dir, { recursive: true }); + await writeFile(path.join(dir, 'package.json'), JSON.stringify({ name: 'p', version: '1.0.0', main: 'index.js' })); + await writeFile(path.join(dir, 'index.js'), 'console.log(1);\n'); +} + +describe('[e2e] apify init with no usable stdin', () => { + let root: string; + + beforeAll(async () => { + root = path.join(TestTmpRoot, `e2e-init-non-tty-${randomBytes(6).toString('hex')}`); + await mkdir(root, { recursive: true }); + }); + + afterAll(async () => { + await rm(root, { recursive: true, force: true }); + }); + + it('--yes with no name initializes with the directory name', async () => { + const cwd = path.join(root, 'yes-default-name'); + await makeNodeProject(cwd); + + const result = await runCliBounded('apify', ['init', '--yes'], { cwd }); + + expect(result.outputCapExceeded, `the prompt retried instead of failing once (${result.bytesWritten} bytes)`).toBe( + false, + ); + expect(result.timedOut, `stderr: ${result.stderr}`).toBe(false); + expect(result.exitCode, `stderr: ${result.stderr}`).toBe(0); + expect(result.stderr).toContain('The Actor has been initialized in the current directory.'); + + const config = JSON.parse(await readFile(path.join(cwd, '.actor', 'actor.json'), 'utf8')); + expect(config.name).toBe('yes-default-name'); + }); + + it('without --yes fails once instead of retrying the name prompt', async () => { + const cwd = path.join(root, 'no-yes-missing-name'); + await makeNodeProject(cwd); + + const result = await runCliBounded('apify', ['init'], { cwd }); + + expect(result.outputCapExceeded, `the prompt retried instead of failing once (${result.bytesWritten} bytes)`).toBe( + false, + ); + expect(result.timedOut, `stderr: ${result.stderr}`).toBe(false); + expect(result.exitCode).not.toBe(0); + expect(countOccurrences(result.stderr, MissingNameError)).toBe(1); + expect(result.stderr).toContain('apify init '); + expect(result.stderr).toContain('--yes'); + + await expect(access(path.join(cwd, '.actor', 'actor.json'))).rejects.toThrow(); + }); + + it('without --yes fails in a directory that is not a Node.js or Python project', async () => { + const cwd = path.join(root, 'no-yes-unknown-project'); + await mkdir(cwd, { recursive: true }); + + const result = await runCliBounded('apify', ['init'], { cwd }); + + expect(result.outputCapExceeded, `the prompt retried instead of failing once (${result.bytesWritten} bytes)`).toBe( + false, + ); + expect(result.timedOut, `stderr: ${result.stderr}`).toBe(false); + expect(result.exitCode).not.toBe(0); + expect(result.stderr).toContain('Confirmation is required to continue'); + + await expect(access(path.join(cwd, '.actor', 'actor.json'))).rejects.toThrow(); + }); +}); diff --git a/test/local/commands/init-non-interactive.test.ts b/test/local/commands/init-non-interactive.test.ts new file mode 100644 index 000000000..0ff91e585 --- /dev/null +++ b/test/local/commands/init-non-interactive.test.ts @@ -0,0 +1,124 @@ +import { existsSync, readFileSync, writeFileSync } from 'node:fs'; +import { mkdir } from 'node:fs/promises'; +import { join } from 'node:path'; +import process from 'node:process'; + +// `isCI` short-circuits every prompt in `stdinCheckWrapper` into a throw, same as a non-TTY stdin. +vitest.mock('ci-info', async (importOriginal) => { + const original = await importOriginal(); + return { ...original, isCI: true }; +}); + +import { testRunCommand } from '../../../src/lib/command-framework/apify-command.js'; +import { LOCAL_CONFIG_PATH } from '../../../src/lib/consts.js'; +import { useTempPath } from '../../__setup__/hooks/useTempPath.js'; + +const { tmpPath, beforeAllCalls, afterAllCalls, forceNewCwd } = useTempPath('init-non-interactive', { + create: true, + remove: true, + cwd: true, + cwdParent: false, +}); + +const { InitCommand } = await import('../../../src/commands/init.js'); + +// Turns a retry loop into a failed assertion in milliseconds instead of a test timeout. +const MAX_ERROR_LINES = 20; + +function captureErrorsWithCap() { + const lines: string[] = []; + + vitest.spyOn(console, 'error').mockImplementation((...args: unknown[]) => { + lines.push(args.map(String).join(' ')); + + if (lines.length > MAX_ERROR_LINES) { + throw new Error(`Prompt retry loop: console.error was called more than ${MAX_ERROR_LINES} times.`); + } + }); + + vitest.spyOn(console, 'log').mockImplementation(() => {}); + + return lines; +} + +function errorLines(lines: string[]) { + return lines.filter((line) => line.includes('Error:')); +} + +// `resetCwdCaches` resolves the hooks outside the `node:process` mock graph, so it cannot clear the +// caches the command reads. A directory per test keeps the cache keys apart. +async function useFreshCwd(name: string) { + forceNewCwd(name); + await mkdir(join(tmpPath, name), { recursive: true }); + + return (...paths: string[]) => join(tmpPath, name, ...paths); +} + +describe('apify init without a usable stdin', () => { + beforeEach(async () => { + await beforeAllCalls(); + process.exitCode = undefined; + }); + + afterEach(async () => { + process.exitCode = undefined; + await afterAllCalls(); + }); + + it('--yes with no name uses the current directory name', async () => { + const joinCwd = await useFreshCwd('yes-default-name'); + writeFileSync(joinCwd('package.json'), JSON.stringify({ name: 'pkg', version: '1.0.0' })); + + const lines = captureErrorsWithCap(); + + await testRunCommand(InitCommand, { flags_yes: true }); + + expect(errorLines(lines)).toStrictEqual([]); + expect(process.exitCode ?? 0).toBe(0); + expect(JSON.parse(readFileSync(joinCwd(LOCAL_CONFIG_PATH), 'utf8')).name).toBe('yes-default-name'); + }); + + it('--yes sanitizes a directory name that is not a legal Actor name', async () => { + const joinCwd = await useFreshCwd('yes_sanitized.name'); + writeFileSync(joinCwd('package.json'), JSON.stringify({ name: 'pkg', version: '1.0.0' })); + + const lines = captureErrorsWithCap(); + + await testRunCommand(InitCommand, { flags_yes: true }); + + expect(errorLines(lines)).toStrictEqual([]); + expect(process.exitCode ?? 0).toBe(0); + expect(JSON.parse(readFileSync(joinCwd(LOCAL_CONFIG_PATH), 'utf8')).name).toBe('yes-sanitized-name'); + }); + + it('--yes fails when nothing is left of the directory name', async () => { + const joinCwd = await useFreshCwd('___'); + writeFileSync(joinCwd('package.json'), JSON.stringify({ name: 'pkg', version: '1.0.0' })); + + const lines = captureErrorsWithCap(); + + await testRunCommand(InitCommand, { flags_yes: true }); + + expect(errorLines(lines)).toHaveLength(1); + expect(errorLines(lines)[0]).toContain('apify init '); + expect(process.exitCode).toBeDefined(); + expect(process.exitCode).not.toBe(0); + expect(existsSync(joinCwd(LOCAL_CONFIG_PATH))).toBe(false); + }); + + it('without --yes reports the missing name once and exits non-zero', async () => { + const joinCwd = await useFreshCwd('no-yes-missing-name'); + writeFileSync(joinCwd('package.json'), JSON.stringify({ name: 'pkg', version: '1.0.0' })); + + const lines = captureErrorsWithCap(); + + await testRunCommand(InitCommand, {}); + + expect(errorLines(lines)).toHaveLength(1); + expect(errorLines(lines)[0]).toContain('apify init '); + expect(errorLines(lines)[0]).toContain('--yes'); + expect(process.exitCode).toBeDefined(); + expect(process.exitCode).not.toBe(0); + expect(existsSync(joinCwd(LOCAL_CONFIG_PATH))).toBe(false); + }); +}); diff --git a/test/local/commands/init.test.ts b/test/local/commands/init.test.ts index 782ef874e..40e1bf866 100644 --- a/test/local/commands/init.test.ts +++ b/test/local/commands/init.test.ts @@ -1,5 +1,6 @@ import { existsSync, readFileSync, writeFileSync } from 'node:fs'; import { mkdir } from 'node:fs/promises'; +import process from 'node:process'; import { KEY_VALUE_STORE_KEYS } from '@apify/consts'; @@ -24,9 +25,20 @@ describe('apify init', () => { }); afterEach(async () => { + process.exitCode = undefined; await afterAllCalls(); }); + it('rejects an Actor name that the platform cannot accept', async () => { + await testRunCommand(InitCommand, { + args_actorName: 'Bad Name!!', + flags_yes: true, + }); + + expect(process.exitCode).toBe(1); + expect(existsSync(joinPath(LOCAL_CONFIG_PATH))).toBeFalsy(); + }); + it('correctly creates basic structure with empty INPUT.json', async () => { await testRunCommand(InitCommand, { args_actorName: actName,