From 3f998b8f1af8a70b7f1f41a7d2c413318ae7f83a Mon Sep 17 00:00:00 2001 From: JuanGalilea Date: Mon, 21 Sep 2026 14:10:52 +0200 Subject: [PATCH 1/4] feat(config/modes): lay groundwork for multiple configuration structures --- bin/utils/config/load-config.ts | 148 ++------------- bin/utils/config/parser.ts | 89 +++++++++ bin/utils/config/structures/base.ts | 50 +++++ bin/utils/config/structures/legacy.ts | 20 ++ .../load-config.test.ts} | 177 +++--------------- test/unit/bin/utils/config/parser.test.ts | 138 ++++++++++++++ .../utils/config/structures/legacy.test.ts | 78 ++++++++ 7 files changed, 420 insertions(+), 280 deletions(-) create mode 100644 bin/utils/config/parser.ts create mode 100644 bin/utils/config/structures/base.ts create mode 100644 bin/utils/config/structures/legacy.ts rename test/unit/bin/utils/{actor-config.test.ts => config/load-config.test.ts} (69%) create mode 100644 test/unit/bin/utils/config/parser.test.ts create mode 100644 test/unit/bin/utils/config/structures/legacy.test.ts diff --git a/bin/utils/config/load-config.ts b/bin/utils/config/load-config.ts index 903895f..70d6a8d 100644 --- a/bin/utils/config/load-config.ts +++ b/bin/utils/config/load-config.ts @@ -1,61 +1,27 @@ import path from 'node:path'; -import { z } from 'zod'; - import { selectActors } from '../../actor-filtering.js'; import { isPathWithinScope } from '../../path-utils.js'; import type { ActorConfig } from '../../types.js'; import { safeReadJsonObjectFile } from '../json-file.js'; +import { parseConfigFile } from './parser.js'; +import type { ResolvedActorConfig } from './structures/base.js'; export const CONFIG_FILE_NAME = 'apify-test-tools.config.json'; -// Reading the config happens in four stages, each one swappable on its own: +// Reading the config happens in three stages, each one swappable on its own: // // 1. file -> plain object (safeReadJsonObjectFile) -// 2. plain object -> ActorConfigFile (parseConfigFile — the schema, swap point for new flavours) -// 3. ActorConfigFile -> ResolvedActorConfig[] (resolveRawConfig — one normalized shape for everyone) -// 4. ResolvedActorConfig -> ActorConfig (loadActorConfig — merges in .actor/actor.json) +// 2. plain object -> ResolvedActorConfig[] (parseConfigFile — picks a strategy, validates +// and normalizes; see ./parser.ts) +// 3. ResolvedActorConfig -> ActorConfig (loadActorConfig — merges in .actor/actor.json) // -// Stages 1-3 are the "what did the user write" half and stage 4 the "what does the repo look like" -// half. A differently shaped config file only has to reach stage 3's output to work with the rest -// of the tool. - -// #region schema (soon to be moved) - -const ACTOR_ENTRY_SCHEMA = z.object({ - folder: z.string(), - // "owner/name", with neither half empty — deliberately permissive about what characters those - // halves may contain, the platform is the authority on that. - actorFullName: z.string().regex(/^[^/]+\/[^/]+$/), - tokenEnvVar: z.string(), - overrideActorContext: z.array(z.string()).optional(), -}); - -const CONFIG_FILE_SCHEMA = z.object({ - actors: z.array(ACTOR_ENTRY_SCHEMA), -}); - -export type ActorConfigFile = z.infer; - -/** - * The single shape stage 4 understands: an actor entry whose paths have been normalized, with no - * trace left of which config flavour produced it. Whatever replaces or extends - * {@link parseConfigFile} and {@link resolveRawConfig} only has to produce this. - */ -export interface ResolvedActorConfig { - actorFullName: string; - folder: string; - tokenEnvVar: string; - overrideActorContext?: string[]; -} - -// #endregion +// Stages 1-2 are the "what did the user write" half and stage 3 the "what does the repo look like" +// half. A differently shaped config file only has to reach stage 2's output — a new strategy under +// ./structures/ — to work with the rest of the tool. // #region utils -// Strips a trailing slash so config-declared paths ("actors/shopify/" vs "actors/shopify") compare equal. -const stripTrailingSlash = (pathValue: string): string => pathValue.replace(/\/+$/, ''); - const findOverlappingContextPaths = (contextPaths: string[]): [string, string] | undefined => { for (let i = 0; i < contextPaths.length; i++) { for (let j = i + 1; j < contextPaths.length; j++) { @@ -70,41 +36,9 @@ const findOverlappingContextPaths = (contextPaths: string[]): [string, string] | return undefined; }; -type ConfigParsingIssue = z.ZodError['issues'][number]; - -// Restates a schema violation in the wording this config file has always used, so error output -// stays recognizable now that the hand-rolled checks are gone. -const describeIssue = (issue: ConfigParsingIssue, rawActors: unknown[]): string => { - const [, index, field] = issue.path; - - if (typeof index !== 'number') { - return `Config file "${CONFIG_FILE_NAME}" must have an "actors" array at the top level.`; - } - - const folder = (rawActors[index] as { folder?: unknown } | undefined)?.folder; - - switch (field) { - case 'folder': - return ( - `Invalid "folder" for actor entry at index ${index} in "${CONFIG_FILE_NAME}". ` + - `Must be a string (use "." for a single-actor repo).` - ); - case 'actorFullName': - return ( - `Invalid "actorFullName" for folder "${folder}" in "${CONFIG_FILE_NAME}". ` + - `Must be in "owner/name" format (e.g. "apify/web-scraper").` - ); - case 'tokenEnvVar': - return `Invalid "tokenEnvVar" for folder "${folder}" in "${CONFIG_FILE_NAME}". Must be a string.`; - case 'overrideActorContext': - return ( - `Invalid "overrideActorContext" for folder "${folder}" in "${CONFIG_FILE_NAME}". ` + - `Must be an array of strings.` - ); - default: - return `Invalid actor entry at index ${index} in "${CONFIG_FILE_NAME}": ${issue.message}`; - } -}; +// The repo root is spelled "" internally but "." in a config file, so report it the way a reader +// would have written it. +const displayFolder = (folder: string): string => folder || '.'; // #endregion @@ -132,53 +66,10 @@ const readConfigFileContents = async (): Promise> => { } }; -/** - * Stage 2 — validates the file's shape. Every problem is reported at once rather than only the - * first one to fail. - */ -export const parseConfigFile = (contents: Record): ActorConfigFile => { - const parsed = CONFIG_FILE_SCHEMA.safeParse(contents); - if (parsed.success) { - return parsed.data; - } - - const rawActors = Array.isArray(contents.actors) ? contents.actors : []; - const messages = new Set(parsed.error.issues.map((issue) => describeIssue(issue, rawActors))); - throw new Error([...messages].join('\n')); -}; - -/** - * Stage 3 — normalizes the paths the user wrote and rejects entries that collide once normalized. - * The repo root is "" from here on, however it was spelled in the file. - */ -export const resolveRawConfig = (config: ActorConfigFile): ResolvedActorConfig[] => { - const seenFolders = new Set(); - - return config.actors.map((entry) => { - const folder = entry.folder === '.' ? '' : stripTrailingSlash(entry.folder); - - if (seenFolders.has(folder)) { - throw new Error( - `Duplicate folder "${entry.folder}" in "${CONFIG_FILE_NAME}". Each actor must have a unique folder.`, - ); - } - seenFolders.add(folder); - - return { - actorFullName: entry.actorFullName, - folder, - tokenEnvVar: entry.tokenEnvVar, - overrideActorContext: entry.overrideActorContext?.map(stripTrailingSlash), - }; - }); -}; - -// The repo root is spelled "" internally but "." in a config file, so report it the way a reader -// would have written it. -const displayFolder = (folder: string): string => folder || '.'; +// Stage 2 lives in ./parser.ts. /** - * Stage 4 — resolves one normalized entry against the repo, reading the actor's `.actor/actor.json` + * Stage 3 — resolves one normalized entry against the repo, reading the actor's `.actor/actor.json` * for its `dockerContextDir` and deciding which paths count as the actor's context. */ export const loadActorConfig = (entry: ResolvedActorConfig): ActorConfig => { @@ -235,16 +126,13 @@ export const loadActorConfig = (entry: ResolvedActorConfig): ActorConfig => { // #endregion export const readConfigFile = async (selection: { actors: string[]; ignore: string[] }): Promise => { - const rawConfig = await readConfigFileContents(); - - // replace this when enabling different file structures - // some kind of strategy pattern seems appropriate here - const resolvedConfigs = resolveRawConfig(parseConfigFile(rawConfig)); + const rawFileContents = await readConfigFileContents(); + const parsedConfigs = parseConfigFile(rawFileContents); const actorConfigs: ActorConfig[] = []; - for (const resolved of resolvedConfigs) { + for (const parsed of parsedConfigs) { // Sequential on purpose: the first actor with a problem should be the one reported. - actorConfigs.push(loadActorConfig(resolved)); + actorConfigs.push(loadActorConfig(parsed)); } return selectActors(selection, actorConfigs); diff --git a/bin/utils/config/parser.ts b/bin/utils/config/parser.ts new file mode 100644 index 0000000..7d3fc80 --- /dev/null +++ b/bin/utils/config/parser.ts @@ -0,0 +1,89 @@ +import z from 'zod'; + +import { CONFIG_FILE_STRATEGY, type ResolvedActorConfig, type StrategyParser } from './structures/base.js'; +import { LEGACY_PARSER } from './structures/legacy.js'; + +const CONFIG_FILE_STRATEGIES: { + // instead of Record, we do this to ensure that the modes match + [key in CONFIG_FILE_STRATEGY]: StrategyParser & { mode: key }; +} = { + [CONFIG_FILE_STRATEGY.LEGACY]: LEGACY_PARSER, +} as const; + +const ModeSelectionSchema = z.object({ + mode: z.enum(CONFIG_FILE_STRATEGY).default(CONFIG_FILE_STRATEGY.LEGACY), +}); + +function selectStrategy(body: Record): StrategyParser { + const parsed = ModeSelectionSchema.safeParse(body); + if (!parsed.success) { + throw new Error(z.prettifyError(parsed.error)); + } + + const { mode } = parsed.data; + return CONFIG_FILE_STRATEGIES[mode]; +} + +// Strips a trailing slash so config-declared paths ("actors/shopify/" vs "actors/shopify") compare equal. +const stripTrailingSlash = (pathValue: string): string => pathValue.replace(/\/+$/, ''); + +// The repo root may be written as ".", "./", "/" or ""; everything downstream spells it "". +const normalizeFolder = (folder: string): string => { + const stripped = stripTrailingSlash(folder); + return stripped === '.' ? '' : stripped; +}; + +/** + * Normalizes the paths the user wrote and checks the cross-entry invariants a per-entry schema + * cannot: that no two actors claim the same folder once normalized, nor the same actor on the + * platform. Collects every violation before throwing, so one run reports all of them rather than + * only the first. + * + * @returns the actors it vouched for, with their paths normalized. + * @throws Error listing every problem found. + */ +function verifyConfiguration(body: ResolvedActorConfig[]): ResolvedActorConfig[] { + const seenFolders = new Set(); + const seenActorFullNames = new Set(); + const errors: string[] = []; + + const normalized = body.map((entry) => ({ + ...entry, + folder: normalizeFolder(entry.folder), + overrideActorContext: entry.overrideActorContext?.map(stripTrailingSlash), + })); + + for (const [index, entry] of normalized.entries()) { + // TODO: drop folder uniqueness (this requires several changes on other places) + if (seenFolders.has(entry.folder)) { + errors.push(`Duplicate folder "${body[index].folder}". Each actor must have a unique folder.`); + } else { + seenFolders.add(entry.folder); + } + + if (seenActorFullNames.has(entry.actorFullName)) { + errors.push(`Duplicate actor "${entry.actorFullName}". Each entry must point at a different actor.`); + } else { + seenActorFullNames.add(entry.actorFullName); + } + } + + if (errors.length > 0) { + throw new Error(errors.join('\n')); + } + + return normalized; +} + +// eslint-disable-next-line no-underscore-dangle +export const _privates = { + selectStrategy, + verifyConfiguration, +}; + +export function parseConfigFile(body: Record): ResolvedActorConfig[] { + const strategy = selectStrategy(body); + const resolved = strategy.parse(body); + const validated = verifyConfiguration(resolved); + return validated; +} diff --git a/bin/utils/config/structures/base.ts b/bin/utils/config/structures/base.ts new file mode 100644 index 0000000..a0b4e9b --- /dev/null +++ b/bin/utils/config/structures/base.ts @@ -0,0 +1,50 @@ +import { prettifyError, type ZodType } from 'zod'; + +export interface ResolvedActorConfig { + actorFullName: string; + folder: string; + tokenEnvVar: string; + overrideActorContext?: string[]; +} + +export enum CONFIG_FILE_STRATEGY { + LEGACY = 'legacy', +} + +export type StrategyParser = { + mode: CONFIG_FILE_STRATEGY; + parse: (body: Record) => ResolvedActorConfig[]; +}; + +export function defineStrategy, Mode extends CONFIG_FILE_STRATEGY>( + // mode is transparent so it can be validated with & { mode: Mode } + mode: Mode, + schema: ZodType, + resolve: (body: T) => ResolvedActorConfig[], +) { + return { + mode, + /** + * + * @param body raw config file to be read + * @returns ResolvedActorConfig[] if the config file is valid, otherwise throws an error with a description of the issues. + * @throws Error if the config file is invalid. + * @example + * ```ts + * const strategy = defineStrategy(CONFIG_FILE_STRATEGY.LEGACY, schema, resolve); + * const resolvedActors = strategy.parse(configFileContents); + * ``` + * + * This function is used to parse the config file according to the strategy defined by the user. + * It uses the schema defined in the strategy to validate the config file and resolve it into a list of ResolvedActorConfig. + * If the config file is invalid, it throws an error with a description of the issues. + */ + parse: (body: Record) => { + const parsed = schema.safeParse(body); + if (parsed.success) { + return resolve(parsed.data); + } + throw new Error(prettifyError(parsed.error)); + }, + }; +} diff --git a/bin/utils/config/structures/legacy.ts b/bin/utils/config/structures/legacy.ts new file mode 100644 index 0000000..8f4666c --- /dev/null +++ b/bin/utils/config/structures/legacy.ts @@ -0,0 +1,20 @@ +import z from 'zod'; + +import { CONFIG_FILE_STRATEGY, defineStrategy } from './base.js'; + +const schema = z.object({ + actors: z + .array( + z.object({ + folder: z.string(), + actorFullName: z.string().regex(/^[a-z0-9_.-]+\/[a-z0-9_.-]+$/), + tokenEnvVar: z.string(), + overrideActorContext: z.array(z.string()).optional(), + }), + ) + .min(1), +}); + +type LegacyConfig = z.infer; + +export const LEGACY_PARSER = defineStrategy(CONFIG_FILE_STRATEGY.LEGACY, schema, (body: LegacyConfig) => body.actors); diff --git a/test/unit/bin/utils/actor-config.test.ts b/test/unit/bin/utils/config/load-config.test.ts similarity index 69% rename from test/unit/bin/utils/actor-config.test.ts rename to test/unit/bin/utils/config/load-config.test.ts index b0a4b15..6300324 100644 --- a/test/unit/bin/utils/actor-config.test.ts +++ b/test/unit/bin/utils/config/load-config.test.ts @@ -4,18 +4,15 @@ import path from 'node:path'; import { afterEach, beforeEach, describe, expect, it } from 'vitest'; -import { - CONFIG_FILE_NAME, - loadActorConfig, - parseConfigFile, - readConfigFile, - resolveRawConfig, -} from '../../../../bin/utils/config/load-config.js'; +import { CONFIG_FILE_NAME, loadActorConfig, readConfigFile } from '../../../../../bin/utils/config/load-config.js'; // `readConfigFile` resolves every path against the process's working directory, so these tests give it // a real one: a throwaway repo in a temp dir. Reading actual files rather than a mocked // `node:fs/promises` is what makes the path handling (the ".actor/" hop, a dockerContextDir escaping // the repo root) worth asserting on — against a mock those assertions only describe the mock. +// +// The shape of the config file itself is the parser's business; see ./parser.test.ts and +// ./structures/legacy.test.ts. let repoDir: string; let originalCwd: string; @@ -45,41 +42,8 @@ const writeFiles = async (files: Record) => }), ); -// Stages 2-4 are callable on their own, which is the point: a differently shaped config file only -// has to reach `resolveRawConfig`'s output to work with everything downstream. -describe('the parse/resolve/load seam', () => { - it('parseConfigFile validates the file shape without touching the filesystem', () => { - const entry = { folder: 'actors/shopify', actorFullName: 'myteam/shopify', tokenEnvVar: 'APIFY_TOKEN' }; - - expect(parseConfigFile({ actors: [entry] })).toEqual({ actors: [entry] }); - expect(() => parseConfigFile({ actors: [{ ...entry, folder: 123 }] })).toThrow(/Invalid "folder"/); - }); - - it('resolveRawConfig is where the repo root becomes "" and trailing slashes go', () => { - const result = resolveRawConfig({ - actors: [ - { folder: '.', actorFullName: 'myteam/root', tokenEnvVar: 'APIFY_TOKEN' }, - { - folder: 'actors/shopify/', - actorFullName: 'myteam/shopify', - tokenEnvVar: 'APIFY_TOKEN', - overrideActorContext: ['packages/'], - }, - ], - }); - - expect(result).toEqual([ - { folder: '', actorFullName: 'myteam/root', tokenEnvVar: 'APIFY_TOKEN', overrideActorContext: undefined }, - { - folder: 'actors/shopify', - actorFullName: 'myteam/shopify', - tokenEnvVar: 'APIFY_TOKEN', - overrideActorContext: ['packages'], - }, - ]); - }); - - it('loadActorConfig resolves data coming from actor.json', async () => { +describe('loadActorConfig', () => { + it('resolves data coming from actor.json', async () => { await writeFiles({ 'actors/shopify/.actor/actor.json': actorJson({ dockerContextDir: '..' }) }); expect( @@ -213,16 +177,18 @@ describe('readConfigFile', () => { await expect(readConfigFile(emptyActorSelection)).rejects.toThrow('invalid JSON'); }); - it('throws when actors array is missing', async () => { - await writeFiles({ [CONFIG_FILE_NAME]: JSON.stringify({ notActors: [] }) }); - await expect(readConfigFile(emptyActorSelection)).rejects.toThrow('"actors" array'); - }); - it('throws when the config file is not a JSON object', async () => { await writeFiles({ [CONFIG_FILE_NAME]: JSON.stringify([{ folder: 'actors/shopify' }]) }); await expect(readConfigFile(emptyActorSelection)).rejects.toThrow('"actors" array'); }); + // The wording of a schema failure belongs to the strategy; this only checks that the failure + // reaches the caller rather than being swallowed on the way out of readConfigFile. + it('surfaces a schema failure from the parser', async () => { + await writeFiles({ [CONFIG_FILE_NAME]: JSON.stringify({ notActors: [] }) }); + await expect(readConfigFile(emptyActorSelection)).rejects.toThrow(/at actors/); + }); + it('throws on duplicate folders', async () => { await writeFiles({ [CONFIG_FILE_NAME]: validConfig([ @@ -247,116 +213,27 @@ describe('readConfigFile', () => { await expect(readConfigFile(emptyActorSelection)).rejects.toThrow('Duplicate folder'); }); - it('throws when actor.json is missing', async () => { - await writeFiles({ - [CONFIG_FILE_NAME]: validConfig([ - { folder: 'actors/shopify', actorFullName: 'apify/shopify', tokenEnvVar: 'APIFY_TOKEN_APIFY' }, - ]), - }); - - await expect(readConfigFile(emptyActorSelection)).rejects.toThrow('Cannot read'); - }); - - it('throws when folder is missing', async () => { - await writeFiles({ - [CONFIG_FILE_NAME]: validConfig([{ actorFullName: 'apify/shopify', tokenEnvVar: 'APIFY_TOKEN_APIFY' }]), - }); - - await expect(readConfigFile(emptyActorSelection)).rejects.toThrow(/Invalid "folder"/); - }); - - it('throws when folder is not a string', async () => { - await writeFiles({ - [CONFIG_FILE_NAME]: validConfig([ - { folder: 123, actorFullName: 'apify/shopify', tokenEnvVar: 'APIFY_TOKEN_APIFY' }, - ]), - }); - - await expect(readConfigFile(emptyActorSelection)).rejects.toThrow(/Invalid "folder"/); - }); - - it('throws when actorFullName is missing', async () => { - await writeFiles({ - [CONFIG_FILE_NAME]: validConfig([{ folder: 'actors/shopify', tokenEnvVar: 'APIFY_TOKEN_APIFY' }]), - 'actors/shopify/.actor/actor.json': actorJson({}), - }); - - await expect(readConfigFile(emptyActorSelection)).rejects.toThrow('Invalid "actorFullName"'); - }); - - it('throws when actorFullName has no slash', async () => { - await writeFiles({ - [CONFIG_FILE_NAME]: validConfig([ - { folder: 'actors/shopify', actorFullName: 'shopify-scraper', tokenEnvVar: 'APIFY_TOKEN_APIFY' }, - ]), - 'actors/shopify/.actor/actor.json': actorJson({}), - }); - - await expect(readConfigFile(emptyActorSelection)).rejects.toThrow('Invalid "actorFullName"'); - }); - - it('throws when actorFullName has empty parts', async () => { + it('throws when two folders declare the same actor', async () => { await writeFiles({ [CONFIG_FILE_NAME]: validConfig([ - { folder: 'actors/shopify', actorFullName: '/shopify', tokenEnvVar: 'APIFY_TOKEN_APIFY' }, + { folder: 'actors/one', actorFullName: 'apify/shopify', tokenEnvVar: 'APIFY_TOKEN_APIFY' }, + { folder: 'actors/two', actorFullName: 'apify/shopify', tokenEnvVar: 'APIFY_TOKEN_APIFY' }, ]), - 'actors/shopify/.actor/actor.json': actorJson({}), - }); - - await expect(readConfigFile(emptyActorSelection)).rejects.toThrow('Invalid "actorFullName"'); - }); - - it('throws when tokenEnvVar is missing', async () => { - await writeFiles({ - [CONFIG_FILE_NAME]: validConfig([{ folder: 'actors/shopify', actorFullName: 'apify/shopify' }]), - 'actors/shopify/.actor/actor.json': actorJson({}), + 'actors/one/.actor/actor.json': actorJson({}), + 'actors/two/.actor/actor.json': actorJson({}), }); - await expect(readConfigFile(emptyActorSelection)).rejects.toThrow('Invalid "tokenEnvVar"'); + await expect(readConfigFile(emptyActorSelection)).rejects.toThrow('Duplicate actor'); }); - it('reports every invalid field at once instead of only the first', async () => { - await writeFiles({ - [CONFIG_FILE_NAME]: validConfig([{ folder: 123, actorFullName: 'shopify', overrideActorContext: 'nope' }]), - }); - - const promise = readConfigFile(emptyActorSelection); - await expect(promise).rejects.toThrow(/Invalid "folder"/); - await expect(promise).rejects.toThrow(/Invalid "actorFullName"/); - await expect(promise).rejects.toThrow(/Invalid "tokenEnvVar"/); - await expect(promise).rejects.toThrow(/Invalid "overrideActorContext"/); - }); - - it('throws when overrideActorContext is not an array', async () => { - await writeFiles({ - [CONFIG_FILE_NAME]: validConfig([ - { - folder: 'actors/shopify', - actorFullName: 'myteam/shopify', - tokenEnvVar: 'APIFY_TOKEN', - overrideActorContext: 'packages', - }, - ]), - 'actors/shopify/.actor/actor.json': actorJson({}), - }); - - await expect(readConfigFile(emptyActorSelection)).rejects.toThrow('Invalid "overrideActorContext"'); - }); - - it('throws when overrideActorContext contains non-strings', async () => { + it('throws when actor.json is missing', async () => { await writeFiles({ [CONFIG_FILE_NAME]: validConfig([ - { - folder: 'actors/shopify', - actorFullName: 'myteam/shopify', - tokenEnvVar: 'APIFY_TOKEN', - overrideActorContext: [123], - }, + { folder: 'actors/shopify', actorFullName: 'apify/shopify', tokenEnvVar: 'APIFY_TOKEN_APIFY' }, ]), - 'actors/shopify/.actor/actor.json': actorJson({}), }); - await expect(readConfigFile(emptyActorSelection)).rejects.toThrow('Invalid "overrideActorContext"'); + await expect(readConfigFile(emptyActorSelection)).rejects.toThrow('Cannot read'); }); it('throws when overrideActorContext entries overlap (one is a prefix of another)', async () => { @@ -447,8 +324,8 @@ describe('readConfigFile', () => { const twoActors = async () => writeFiles({ [CONFIG_FILE_NAME]: validConfig([ - { folder: 'actors/a', actorFullName: 'team/a', tokenEnvVar: 'TOKEN' }, - { folder: 'actors/b', actorFullName: 'team/b', tokenEnvVar: 'TOKEN' }, + { folder: 'actors/a', actorFullName: 'team/actor-a', tokenEnvVar: 'TOKEN' }, + { folder: 'actors/b', actorFullName: 'team/actor-b', tokenEnvVar: 'TOKEN' }, ]), 'actors/a/.actor/actor.json': actorJson({}), 'actors/b/.actor/actor.json': actorJson({}), @@ -458,17 +335,17 @@ describe('readConfigFile', () => { it('returns all actors when the selection is empty', async () => { await twoActors(); - expect(fullNames(await readConfigFile(emptyActorSelection))).toEqual(['team/a', 'team/b']); + expect(fullNames(await readConfigFile(emptyActorSelection))).toEqual(['team/actor-a', 'team/actor-b']); }); it('keeps only the selected actors', async () => { await twoActors(); - expect(fullNames(await readConfigFile({ actors: ['team/a'], ignore: [] }))).toEqual(['team/a']); + expect(fullNames(await readConfigFile({ actors: ['team/actor-a'], ignore: [] }))).toEqual(['team/actor-a']); }); it('drops ignored actors', async () => { await twoActors(); - expect(fullNames(await readConfigFile({ actors: [], ignore: ['team/a'] }))).toEqual(['team/b']); + expect(fullNames(await readConfigFile({ actors: [], ignore: ['team/actor-a'] }))).toEqual(['team/actor-b']); }); it('throws on an unknown actor name', async () => { diff --git a/test/unit/bin/utils/config/parser.test.ts b/test/unit/bin/utils/config/parser.test.ts new file mode 100644 index 0000000..aee9f91 --- /dev/null +++ b/test/unit/bin/utils/config/parser.test.ts @@ -0,0 +1,138 @@ +import { describe, expect, it } from 'vitest'; + +import { _privates, parseConfigFile } from '../../../../../bin/utils/config/parser.js'; +import { CONFIG_FILE_STRATEGY } from '../../../../../bin/utils/config/structures/base.js'; +import { LEGACY_PARSER } from '../../../../../bin/utils/config/structures/legacy.js'; + +const { selectStrategy, verifyConfiguration } = _privates; + +const actor = (fields: Record = {}) => ({ + folder: 'actors/shopify', + actorFullName: 'myteam/shopify', + tokenEnvVar: 'APIFY_TOKEN', + ...fields, +}); + +describe('selectStrategy', () => { + it('falls back to the legacy strategy when no mode is declared', () => { + expect(selectStrategy({ actors: [] })).toBe(LEGACY_PARSER); + }); + + it('honours an explicit mode', () => { + expect(selectStrategy({ mode: CONFIG_FILE_STRATEGY.LEGACY, actors: [] })).toBe(LEGACY_PARSER); + }); + + it('throws on a mode no strategy is registered for', () => { + expect(() => selectStrategy({ mode: 'brand-new' })).toThrow(/at mode/); + }); +}); + +describe('verifyConfiguration', () => { + it('resolves the repo root to "" and gets rid of trailing slashes', () => { + const result = verifyConfiguration([ + { folder: '.', actorFullName: 'myteam/root', tokenEnvVar: 'APIFY_TOKEN' }, + { + folder: 'actors/shopify/', + actorFullName: 'myteam/shopify', + tokenEnvVar: 'APIFY_TOKEN', + overrideActorContext: ['packages/'], + }, + ]); + + expect(result).toEqual([ + { folder: '', actorFullName: 'myteam/root', tokenEnvVar: 'APIFY_TOKEN', overrideActorContext: undefined }, + { + folder: 'actors/shopify', + actorFullName: 'myteam/shopify', + tokenEnvVar: 'APIFY_TOKEN', + overrideActorContext: ['packages'], + }, + ]); + }); + + it.each([ + ['"." and ""', '.', ''], + ['weird roots', '', './//'], + ['a trailing slash', 'actors/shopify', 'actors/shopify/'], + ])('rejects folders that collide once normalized: %s', (_name, first, second) => { + expect(() => + verifyConfiguration([ + actor({ folder: first, actorFullName: 'myteam/actor-a' }), + actor({ folder: second, actorFullName: 'myteam/actor-b' }), + ]), + ).toThrow(/Duplicate folder/); + }); + + it('quotes a duplicate folder the way it was written, not the way it normalized', () => { + expect(() => + verifyConfiguration([ + actor({ folder: '.', actorFullName: 'myteam/actor-a' }), + actor({ folder: './', actorFullName: 'myteam/actor-b' }), + ]), + ).toThrow('Duplicate folder "./"'); + }); + + it('rejects two entries pointing at the same actor', () => { + expect(() => verifyConfiguration([actor({ folder: 'actors/a' }), actor({ folder: 'actors/b' })])).toThrow( + /Duplicate actor "myteam\/shopify"/, + ); + }); + + it('treats actorFullName case-sensitively — "myteam/A" is not "myteam/a"', () => { + expect(() => + verifyConfiguration([ + actor({ folder: 'actors/a', actorFullName: 'myteam/abc' }), + actor({ folder: 'actors/b', actorFullName: 'myteam/ABC' }), + ]), + ).not.toThrow(); + }); + + it('reports every collision at once instead of stopping at the first', () => { + const message = (() => { + try { + verifyConfiguration([ + actor({ folder: 'actors/a', actorFullName: 'myteam/actor-a' }), + actor({ folder: 'actors/a', actorFullName: 'myteam/actor-b' }), + actor({ folder: 'actors/c', actorFullName: 'myteam/actor-c' }), + actor({ folder: 'actors/d', actorFullName: 'myteam/actor-c' }), + ]); + return ''; + } catch (err) { + return (err as Error).message; + } + })(); + + expect(message.split('\n')).toEqual([ + 'Duplicate folder "actors/a". Each actor must have a unique folder.', + 'Duplicate actor "myteam/actor-c". Each entry must point at a different actor.', + ]); + }); + + it('reports both problems for an entry that duplicates a folder and an actor at once', () => { + expect(() => verifyConfiguration([actor(), actor()])).toThrow(/Duplicate folder[\s\S]*Duplicate actor/); + }); + + it('accepts an empty configuration', () => { + expect(verifyConfiguration([])).toEqual([]); + }); +}); + +// The seam that matters: a differently shaped config file only has to reach this function's output +// to work with everything downstream. +describe('parseConfigFile', () => { + it('validates and normalizes a plain object without touching the filesystem', () => { + expect(parseConfigFile({ actors: [actor({ folder: 'actors/shopify/' })] })).toEqual([ + actor({ folder: 'actors/shopify', overrideActorContext: undefined }), + ]); + }); + + it('surfaces schema violations from the selected strategy', () => { + expect(() => parseConfigFile({ actors: [actor({ folder: 123 })] })).toThrow(/at actors\[0\]\.folder/); + }); + + it('surfaces cross-entry violations the schema cannot see', () => { + expect(() => parseConfigFile({ actors: [actor({ actorFullName: 'myteam/actor-a' }), actor()] })).toThrow( + /Duplicate folder/, + ); + }); +}); diff --git a/test/unit/bin/utils/config/structures/legacy.test.ts b/test/unit/bin/utils/config/structures/legacy.test.ts new file mode 100644 index 0000000..0b5b500 --- /dev/null +++ b/test/unit/bin/utils/config/structures/legacy.test.ts @@ -0,0 +1,78 @@ +import { describe, expect, it } from 'vitest'; + +import { LEGACY_PARSER } from '../../../../../../bin/utils/config/structures/legacy.js'; + +const entry = (fields: Record = {}) => ({ + folder: 'actors/shopify', + actorFullName: 'myteam/shopify', + tokenEnvVar: 'APIFY_TOKEN', + ...fields, +}); + +describe('LEGACY_PARSER', () => { + it("returns the entries as written — normalizing is not this layer's job", () => { + const raw = entry({ folder: 'actors/shopify/', overrideActorContext: ['packages/'] }); + + expect(LEGACY_PARSER.parse({ actors: [raw] })).toEqual([raw]); + }); + + it('drops the "mode" discriminator rather than choking on it', () => { + expect(LEGACY_PARSER.parse({ mode: 'legacy', actors: [entry()] })).toEqual([entry()]); + }); + + it('rejects a file with no "actors" array', () => { + expect(() => LEGACY_PARSER.parse({ notActors: [] })).toThrow(/expected array[\s\S]*at actors/); + }); + + it('rejects an empty "actors" array', () => { + expect(() => LEGACY_PARSER.parse({ actors: [] })).toThrow(/Too small/); + }); + + it.each([ + ['folder is missing', { folder: undefined }, /at actors\[0\]\.folder/], + ['folder is not a string', { folder: 123 }, /at actors\[0\]\.folder/], + ['tokenEnvVar is missing', { tokenEnvVar: undefined }, /at actors\[0\]\.tokenEnvVar/], + ['overrideActorContext is not an array', { overrideActorContext: 'packages' }, /expected array/], + ['overrideActorContext holds non-strings', { overrideActorContext: [123] }, /overrideActorContext\[0\]/], + ])('rejects an entry where %s', (_name, fields, expected) => { + expect(() => LEGACY_PARSER.parse({ actors: [entry(fields)] })).toThrow(expected); + }); + + // The "owner/name" halves are checked against the platform's own username/actor-name rules. + it.each([ + ['no slash', 'shopify-scraper'], + ['an empty owner', '/shopify'], + ['an empty name', 'myteam/'], + ['an uppercase owner', 'MyTeam/shopify'], + ['more than two halves', 'myteam/shopify/extra'], + ])('rejects an actorFullName with %s', (_name, actorFullName) => { + expect(() => LEGACY_PARSER.parse({ actors: [entry({ actorFullName })] })).toThrow( + /at actors\[0\]\.actorFullName/, + ); + }); + + it.each([['myteam/shopify'], ['my.team/web-scraper'], ['my_team/a1'], ['apify/web-scraper']])( + 'accepts the actorFullName %s', + (actorFullName) => { + expect(() => LEGACY_PARSER.parse({ actors: [entry({ actorFullName })] })).not.toThrow(); + }, + ); + + it('reports every invalid field at once instead of only the first', () => { + const message = (() => { + try { + LEGACY_PARSER.parse({ + actors: [{ folder: 123, actorFullName: 'shopify', overrideActorContext: 'nope' }], + }); + throw new Error('Function should have thrown'); + } catch (err) { + return (err as Error).message; + } + })(); + + expect(message).toContain('actors[0].folder'); + expect(message).toContain('actors[0].actorFullName'); + expect(message).toContain('actors[0].tokenEnvVar'); + expect(message).toContain('actors[0].overrideActorContext'); + }); +}); From 5bf924e77831026a718e82d57d7f4ff7ad788153 Mon Sep 17 00:00:00 2001 From: JuanGalilea Date: Mon, 21 Sep 2026 14:52:09 +0200 Subject: [PATCH 2/4] remove redundant tests and test closer to functionality --- bin/utils/config/load-config.ts | 6 - .../unit/bin/utils/config/load-config.test.ts | 343 +++++------------- test/unit/bin/utils/config/parser.test.ts | 19 +- 3 files changed, 91 insertions(+), 277 deletions(-) diff --git a/bin/utils/config/load-config.ts b/bin/utils/config/load-config.ts index 70d6a8d..6f4c138 100644 --- a/bin/utils/config/load-config.ts +++ b/bin/utils/config/load-config.ts @@ -15,10 +15,6 @@ export const CONFIG_FILE_NAME = 'apify-test-tools.config.json'; // 2. plain object -> ResolvedActorConfig[] (parseConfigFile — picks a strategy, validates // and normalizes; see ./parser.ts) // 3. ResolvedActorConfig -> ActorConfig (loadActorConfig — merges in .actor/actor.json) -// -// Stages 1-2 are the "what did the user write" half and stage 3 the "what does the repo look like" -// half. A differently shaped config file only has to reach stage 2's output — a new strategy under -// ./structures/ — to work with the rest of the tool. // #region utils @@ -66,8 +62,6 @@ const readConfigFileContents = async (): Promise> => { } }; -// Stage 2 lives in ./parser.ts. - /** * Stage 3 — resolves one normalized entry against the repo, reading the actor's `.actor/actor.json` * for its `dockerContextDir` and deciding which paths count as the actor's context. diff --git a/test/unit/bin/utils/config/load-config.test.ts b/test/unit/bin/utils/config/load-config.test.ts index 6300324..771cd67 100644 --- a/test/unit/bin/utils/config/load-config.test.ts +++ b/test/unit/bin/utils/config/load-config.test.ts @@ -5,14 +5,8 @@ import path from 'node:path'; import { afterEach, beforeEach, describe, expect, it } from 'vitest'; import { CONFIG_FILE_NAME, loadActorConfig, readConfigFile } from '../../../../../bin/utils/config/load-config.js'; +import type { ResolvedActorConfig } from '../../../../../bin/utils/config/structures/base.js'; -// `readConfigFile` resolves every path against the process's working directory, so these tests give it -// a real one: a throwaway repo in a temp dir. Reading actual files rather than a mocked -// `node:fs/promises` is what makes the path handling (the ".actor/" hop, a dockerContextDir escaping -// the repo root) worth asserting on — against a mock those assertions only describe the mock. -// -// The shape of the config file itself is the parser's business; see ./parser.test.ts and -// ./structures/legacy.test.ts. let repoDir: string; let originalCwd: string; @@ -43,12 +37,20 @@ const writeFiles = async (files: Record) => ); describe('loadActorConfig', () => { + const entry = (fields: Partial = {}): ResolvedActorConfig => ({ + folder: 'actors/shopify', + actorFullName: 'myteam/shopify', + tokenEnvVar: 'APIFY_TOKEN', + ...fields, + }); + + const writeActorJson = async (fields: Record = {}) => + writeFiles({ 'actors/shopify/.actor/actor.json': actorJson(fields) }); + it('resolves data coming from actor.json', async () => { - await writeFiles({ 'actors/shopify/.actor/actor.json': actorJson({ dockerContextDir: '..' }) }); + await writeActorJson({ dockerContextDir: '..' }); - expect( - loadActorConfig({ folder: 'actors/shopify', actorFullName: 'myteam/shopify', tokenEnvVar: 'APIFY_TOKEN' }), - ).toEqual({ + expect(loadActorConfig(entry())).toEqual({ actorFullName: 'myteam/shopify', folder: 'actors/shopify', tokenEnvVar: 'APIFY_TOKEN', @@ -56,238 +58,75 @@ describe('loadActorConfig', () => { contextPaths: ['actors/shopify'], }); }); -}); - -describe('readConfigFile', () => { - it('returns correct ActorConfig[] for a valid config', async () => { - await writeFiles({ - [CONFIG_FILE_NAME]: validConfig([ - { - folder: 'actors/shopify', - actorFullName: 'myteam/shopify-scraper', - tokenEnvVar: 'APIFY_TOKEN_MYTEAM', - }, - ]), - 'actors/shopify/.actor/actor.json': actorJson({ dockerContextDir: '../../..' }), - }); - - const result = await readConfigFile(emptyActorSelection); - expect(result).toEqual([ - { - actorFullName: 'myteam/shopify-scraper', - folder: 'actors/shopify', - tokenEnvVar: 'APIFY_TOKEN_MYTEAM', - dockerContextDir: '', - contextPaths: [''], - }, - ]); - }); - - it('normalizes folder "." to ""', async () => { - await writeFiles({ - [CONFIG_FILE_NAME]: validConfig([ - { folder: '.', actorFullName: 'apify/my-actor', tokenEnvVar: 'APIFY_TOKEN_APIFY' }, - ]), - '.actor/actor.json': actorJson({}), - }); - - const result = await readConfigFile(emptyActorSelection); - expect(result[0].folder).toBe(''); - }); - it('defaults dockerContextDir to actor folder when absent from actor.json', async () => { - await writeFiles({ - [CONFIG_FILE_NAME]: validConfig([ - { folder: 'actors/web-scraper', actorFullName: 'apify/web-scraper', tokenEnvVar: 'APIFY_TOKEN_APIFY' }, - ]), - 'actors/web-scraper/.actor/actor.json': actorJson({}), - }); + it('defaults dockerContextDir to the actor folder when actor.json does not set it', async () => { + await writeActorJson(); - const result = await readConfigFile(emptyActorSelection); - expect(result[0].dockerContextDir).toBe('actors/web-scraper'); - expect(result[0].contextPaths).toEqual(['actors/web-scraper']); + expect(loadActorConfig(entry()).dockerContextDir).toBe('actors/shopify'); }); - it('resolves dockerContextDir relative to .actor/ folder', async () => { - await writeFiles({ - [CONFIG_FILE_NAME]: validConfig([ - { folder: 'actors/shopify', actorFullName: 'myteam/shopify', tokenEnvVar: 'APIFY_TOKEN' }, - ]), - 'actors/shopify/.actor/actor.json': actorJson({ dockerContextDir: '../../..' }), - }); + it('resolves dockerContextDir relative to the .actor/ folder, not the actor folder', async () => { + await writeActorJson({ dockerContextDir: '../../..' }); - const result = await readConfigFile(emptyActorSelection); - expect(result[0].dockerContextDir).toBe(''); + expect(loadActorConfig(entry()).dockerContextDir).toBe(''); }); it('throws when dockerContextDir resolves outside the repository root', async () => { - await writeFiles({ - [CONFIG_FILE_NAME]: validConfig([ - { folder: 'actors/shopify', actorFullName: 'myteam/shopify', tokenEnvVar: 'APIFY_TOKEN' }, - ]), - 'actors/shopify/.actor/actor.json': actorJson({ dockerContextDir: '../../../..' }), - }); + await writeActorJson({ dockerContextDir: '../../../..' }); - await expect(readConfigFile(emptyActorSelection)).rejects.toThrow(/resolves outside the repository root/); + expect(() => loadActorConfig(entry())).toThrow(/resolves outside the repository root/); }); - it('resolves contextPaths from overrideActorContext', async () => { - await writeFiles({ - [CONFIG_FILE_NAME]: validConfig([ - { - folder: 'actors/shopify', - actorFullName: 'myteam/shopify', - tokenEnvVar: 'APIFY_TOKEN', - overrideActorContext: ['actors/shopify', 'packages'], - }, - ]), - 'actors/shopify/.actor/actor.json': actorJson({ dockerContextDir: '../../..' }), - }); - - const result = await readConfigFile(emptyActorSelection); - expect(result[0].contextPaths).toEqual(['actors/shopify', 'packages']); - }); - - it('handles multiple actors', async () => { - await writeFiles({ - [CONFIG_FILE_NAME]: validConfig([ - { folder: 'actors/web-scraper', actorFullName: 'apify/web-scraper', tokenEnvVar: 'APIFY_TOKEN_APIFY' }, - { - folder: 'actors/email-sender', - actorFullName: 'other-team/email-sender', - tokenEnvVar: 'APIFY_TOKEN_OTHER_TEAM', - }, - ]), - 'actors/web-scraper/.actor/actor.json': actorJson({}), - 'actors/email-sender/.actor/actor.json': actorJson({}), - }); - - const result = await readConfigFile(emptyActorSelection); - expect(result).toHaveLength(2); - expect(result[0].actorFullName).toBe('apify/web-scraper'); - expect(result[1].actorFullName).toBe('other-team/email-sender'); - }); - - it('throws when config file is missing', async () => { - await expect(readConfigFile(emptyActorSelection)).rejects.toThrow('not found'); - }); - - it('throws when config file contains invalid JSON', async () => { - await writeFiles({ [CONFIG_FILE_NAME]: '{bad json' }); - await expect(readConfigFile(emptyActorSelection)).rejects.toThrow('invalid JSON'); + it('throws when actor.json is missing', () => { + expect(() => loadActorConfig(entry())).toThrow('Cannot read'); }); - it('throws when the config file is not a JSON object', async () => { - await writeFiles({ [CONFIG_FILE_NAME]: JSON.stringify([{ folder: 'actors/shopify' }]) }); - await expect(readConfigFile(emptyActorSelection)).rejects.toThrow('"actors" array'); - }); - - // The wording of a schema failure belongs to the strategy; this only checks that the failure - // reaches the caller rather than being swallowed on the way out of readConfigFile. - it('surfaces a schema failure from the parser', async () => { - await writeFiles({ [CONFIG_FILE_NAME]: JSON.stringify({ notActors: [] }) }); - await expect(readConfigFile(emptyActorSelection)).rejects.toThrow(/at actors/); - }); - - it('throws on duplicate folders', async () => { - await writeFiles({ - [CONFIG_FILE_NAME]: validConfig([ - { folder: 'actors/shopify', actorFullName: 'apify/shopify', tokenEnvVar: 'APIFY_TOKEN_APIFY' }, - { folder: 'actors/shopify', actorFullName: 'other/shopify', tokenEnvVar: 'APIFY_TOKEN_OTHER' }, - ]), - 'actors/shopify/.actor/actor.json': actorJson({}), - }); - - await expect(readConfigFile(emptyActorSelection)).rejects.toThrow('Duplicate folder'); - }); - - it('throws on duplicate folders after normalization ("." and "")', async () => { - await writeFiles({ - [CONFIG_FILE_NAME]: validConfig([ - { folder: '.', actorFullName: 'apify/actor-a', tokenEnvVar: 'APIFY_TOKEN_APIFY' }, - { folder: '', actorFullName: 'other/actor-b', tokenEnvVar: 'APIFY_TOKEN_OTHER' }, - ]), - '.actor/actor.json': actorJson({}), - }); + it('resolves contextPaths from overrideActorContext', async () => { + await writeActorJson({ dockerContextDir: '../../..' }); - await expect(readConfigFile(emptyActorSelection)).rejects.toThrow('Duplicate folder'); + expect(loadActorConfig(entry({ overrideActorContext: ['actors/shopify', 'packages'] })).contextPaths).toEqual([ + 'actors/shopify', + 'packages', + ]); }); - it('throws when two folders declare the same actor', async () => { - await writeFiles({ - [CONFIG_FILE_NAME]: validConfig([ - { folder: 'actors/one', actorFullName: 'apify/shopify', tokenEnvVar: 'APIFY_TOKEN_APIFY' }, - { folder: 'actors/two', actorFullName: 'apify/shopify', tokenEnvVar: 'APIFY_TOKEN_APIFY' }, - ]), - 'actors/one/.actor/actor.json': actorJson({}), - 'actors/two/.actor/actor.json': actorJson({}), - }); + it('adds the actor own folder automatically when overrideActorContext does not cover it', async () => { + await writeActorJson(); - await expect(readConfigFile(emptyActorSelection)).rejects.toThrow('Duplicate actor'); + expect(loadActorConfig(entry({ overrideActorContext: ['code', 'shared'] })).contextPaths).toEqual([ + 'code', + 'shared', + 'actors/shopify', + ]); }); - it('throws when actor.json is missing', async () => { - await writeFiles({ - [CONFIG_FILE_NAME]: validConfig([ - { folder: 'actors/shopify', actorFullName: 'apify/shopify', tokenEnvVar: 'APIFY_TOKEN_APIFY' }, - ]), - }); + it('allows overrideActorContext with disjoint sibling paths that all reach the actor folder via one entry', async () => { + await writeActorJson(); - await expect(readConfigFile(emptyActorSelection)).rejects.toThrow('Cannot read'); + expect( + loadActorConfig(entry({ overrideActorContext: ['actors/shopify', 'code', 'shared'] })).contextPaths, + ).toEqual(['actors/shopify', 'code', 'shared']); }); it('throws when overrideActorContext entries overlap (one is a prefix of another)', async () => { - await writeFiles({ - [CONFIG_FILE_NAME]: validConfig([ - { - folder: 'actors/shopify', - actorFullName: 'myteam/shopify', - tokenEnvVar: 'APIFY_TOKEN', - overrideActorContext: ['actors/shopify', 'actors'], - }, - ]), - 'actors/shopify/.actor/actor.json': actorJson({}), - }); + await writeActorJson(); - await expect(readConfigFile(emptyActorSelection)).rejects.toThrow(/overlap/); + expect(() => loadActorConfig(entry({ overrideActorContext: ['actors/shopify', 'actors'] }))).toThrow(/overlap/); }); it('throws when overrideActorContext contains the repo root alongside another entry', async () => { - await writeFiles({ - [CONFIG_FILE_NAME]: validConfig([ - { - folder: 'actors/shopify', - actorFullName: 'myteam/shopify', - tokenEnvVar: 'APIFY_TOKEN', - overrideActorContext: ['', 'actors/shopify'], - }, - ]), - 'actors/shopify/.actor/actor.json': actorJson({}), - }); - - await expect(readConfigFile(emptyActorSelection)).rejects.toThrow(/overlap/); - }); - - it('adds the actor own folder automatically when overrideActorContext does not cover it', async () => { - await writeFiles({ - [CONFIG_FILE_NAME]: validConfig([ - { - folder: 'actors/shopify', - actorFullName: 'myteam/shopify', - tokenEnvVar: 'APIFY_TOKEN', - overrideActorContext: ['code', 'shared'], - }, - ]), - 'actors/shopify/.actor/actor.json': actorJson({}), - }); + await writeActorJson(); - const result = await readConfigFile(emptyActorSelection); - expect(result[0].contextPaths).toEqual(['code', 'shared', 'actors/shopify']); + expect(() => loadActorConfig(entry({ overrideActorContext: ['', 'actors/shopify'] }))).toThrow(/overlap/); }); +}); - it('strips trailing slashes from folder and overrideActorContext entries', async () => { +describe('readConfigFile', () => { + // Golden output massaging the whole config reading process + it("carries the parser's normalization through to ActorConfig[]", async () => { await writeFiles({ [CONFIG_FILE_NAME]: validConfig([ + { folder: '.', actorFullName: 'apify/root', tokenEnvVar: 'APIFY_TOKEN_APIFY' }, { folder: 'actors/shopify/', actorFullName: 'myteam/shopify', @@ -295,64 +134,46 @@ describe('readConfigFile', () => { overrideActorContext: ['actors/shopify/', 'packages/'], }, ]), + '.actor/actor.json': actorJson({}), 'actors/shopify/.actor/actor.json': actorJson({}), }); - const result = await readConfigFile(emptyActorSelection); - expect(result[0].folder).toBe('actors/shopify'); - expect(result[0].contextPaths).toEqual(['actors/shopify', 'packages']); + expect(await readConfigFile(emptyActorSelection)).toEqual([ + { + actorFullName: 'apify/root', + folder: '', + tokenEnvVar: 'APIFY_TOKEN_APIFY', + dockerContextDir: '', + contextPaths: [''], + }, + { + actorFullName: 'myteam/shopify', + folder: 'actors/shopify', + tokenEnvVar: 'APIFY_TOKEN', + dockerContextDir: 'actors/shopify', + contextPaths: ['actors/shopify', 'packages'], + }, + ]); }); - it('allows overrideActorContext with disjoint sibling paths that all reach the actor folder via one entry', async () => { - await writeFiles({ - [CONFIG_FILE_NAME]: validConfig([ - { - folder: 'actors/shopify', - actorFullName: 'myteam/shopify', - tokenEnvVar: 'APIFY_TOKEN', - overrideActorContext: ['actors/shopify', 'code', 'shared'], - }, - ]), - 'actors/shopify/.actor/actor.json': actorJson({}), - }); - - const result = await readConfigFile(emptyActorSelection); - expect(result[0].contextPaths).toEqual(['actors/shopify', 'code', 'shared']); + it('throws when config file is missing', async () => { + await expect(readConfigFile(emptyActorSelection)).rejects.toThrow('not found'); }); - describe('actor selection', () => { - const twoActors = async () => - writeFiles({ - [CONFIG_FILE_NAME]: validConfig([ - { folder: 'actors/a', actorFullName: 'team/actor-a', tokenEnvVar: 'TOKEN' }, - { folder: 'actors/b', actorFullName: 'team/actor-b', tokenEnvVar: 'TOKEN' }, - ]), - 'actors/a/.actor/actor.json': actorJson({}), - 'actors/b/.actor/actor.json': actorJson({}), - }); - - const fullNames = (result: { actorFullName: string }[]) => result.map((c) => c.actorFullName); - - it('returns all actors when the selection is empty', async () => { - await twoActors(); - expect(fullNames(await readConfigFile(emptyActorSelection))).toEqual(['team/actor-a', 'team/actor-b']); - }); - - it('keeps only the selected actors', async () => { - await twoActors(); - expect(fullNames(await readConfigFile({ actors: ['team/actor-a'], ignore: [] }))).toEqual(['team/actor-a']); - }); + it('throws when config file contains invalid JSON', async () => { + await writeFiles({ [CONFIG_FILE_NAME]: '{bad json' }); + await expect(readConfigFile(emptyActorSelection)).rejects.toThrow('invalid JSON'); + }); - it('drops ignored actors', async () => { - await twoActors(); - expect(fullNames(await readConfigFile({ actors: [], ignore: ['team/actor-a'] }))).toEqual(['team/actor-b']); - }); + it('throws when the config file is not a JSON object', async () => { + await writeFiles({ [CONFIG_FILE_NAME]: JSON.stringify([{ folder: 'actors/shopify' }]) }); + await expect(readConfigFile(emptyActorSelection)).rejects.toThrow('"actors" array'); + }); - it('throws on an unknown actor name', async () => { - await twoActors(); - await expect(readConfigFile({ actors: ['team/nope'], ignore: [] })).rejects.toThrow( - 'do not exist: team/nope', - ); - }); + // The wording of a schema failure belongs to the strategy; this only checks that the failure + // reaches the caller rather than being swallowed on the way out of readConfigFile. + it('surfaces a schema failure from the parser', async () => { + await writeFiles({ [CONFIG_FILE_NAME]: JSON.stringify({ notActors: [] }) }); + await expect(readConfigFile(emptyActorSelection)).rejects.toThrow(/at actors/); }); }); diff --git a/test/unit/bin/utils/config/parser.test.ts b/test/unit/bin/utils/config/parser.test.ts index aee9f91..5054260 100644 --- a/test/unit/bin/utils/config/parser.test.ts +++ b/test/unit/bin/utils/config/parser.test.ts @@ -117,8 +117,6 @@ describe('verifyConfiguration', () => { }); }); -// The seam that matters: a differently shaped config file only has to reach this function's output -// to work with everything downstream. describe('parseConfigFile', () => { it('validates and normalizes a plain object without touching the filesystem', () => { expect(parseConfigFile({ actors: [actor({ folder: 'actors/shopify/' })] })).toEqual([ @@ -126,13 +124,14 @@ describe('parseConfigFile', () => { ]); }); - it('surfaces schema violations from the selected strategy', () => { - expect(() => parseConfigFile({ actors: [actor({ folder: 123 })] })).toThrow(/at actors\[0\]\.folder/); - }); - - it('surfaces cross-entry violations the schema cannot see', () => { - expect(() => parseConfigFile({ actors: [actor({ actorFullName: 'myteam/actor-a' }), actor()] })).toThrow( - /Duplicate folder/, - ); + it('surfaces cross-entry violations from verifyConfiguration', () => { + expect(() => + parseConfigFile({ + actors: [ + actor({ actorFullName: 'myteam/actor-a', folder: 'collide/collide' }), + actor({ actorFullName: 'myteam/actor-b', folder: 'collide/collide' }), + ], + }), + ).toThrow(/Duplicate folder/); }); }); From 71a9a4f970ea3a041dbfdd22e2745b4a3b4ee248 Mon Sep 17 00:00:00 2001 From: JuanGalilea Date: Mon, 21 Sep 2026 16:06:35 +0200 Subject: [PATCH 3/4] regex as requested by luigi --- bin/utils/config/structures/base.ts | 11 +++ bin/utils/config/structures/legacy.ts | 4 +- .../bin/utils/config/structures/base.test.ts | 83 +++++++++++++++++++ .../utils/config/structures/legacy.test.ts | 4 +- 4 files changed, 98 insertions(+), 4 deletions(-) create mode 100644 test/unit/bin/utils/config/structures/base.test.ts diff --git a/bin/utils/config/structures/base.ts b/bin/utils/config/structures/base.ts index a0b4e9b..afe8264 100644 --- a/bin/utils/config/structures/base.ts +++ b/bin/utils/config/structures/base.ts @@ -1,5 +1,16 @@ import { prettifyError, type ZodType } from 'zod'; +import { ACTOR_NAME, USERNAME } from '@apify/consts'; + +function stripRegexAnchor(regex: string): string { + return regex.replace(/^\^/, '').replace(/\$$/, ''); +} + +export const ACTOR_FULL_NAME_REGEX = new RegExp( + `^${stripRegexAnchor(USERNAME.REGEX.source)}/${stripRegexAnchor(ACTOR_NAME.REGEX.source)}$`, + USERNAME.REGEX.flags + ACTOR_NAME.REGEX.flags, +); + export interface ResolvedActorConfig { actorFullName: string; folder: string; diff --git a/bin/utils/config/structures/legacy.ts b/bin/utils/config/structures/legacy.ts index 8f4666c..8cea1ca 100644 --- a/bin/utils/config/structures/legacy.ts +++ b/bin/utils/config/structures/legacy.ts @@ -1,13 +1,13 @@ import z from 'zod'; -import { CONFIG_FILE_STRATEGY, defineStrategy } from './base.js'; +import { ACTOR_FULL_NAME_REGEX, CONFIG_FILE_STRATEGY, defineStrategy } from './base.js'; const schema = z.object({ actors: z .array( z.object({ folder: z.string(), - actorFullName: z.string().regex(/^[a-z0-9_.-]+\/[a-z0-9_.-]+$/), + actorFullName: z.string().regex(ACTOR_FULL_NAME_REGEX), tokenEnvVar: z.string(), overrideActorContext: z.array(z.string()).optional(), }), diff --git a/test/unit/bin/utils/config/structures/base.test.ts b/test/unit/bin/utils/config/structures/base.test.ts new file mode 100644 index 0000000..9aa1456 --- /dev/null +++ b/test/unit/bin/utils/config/structures/base.test.ts @@ -0,0 +1,83 @@ +import { describe, expect, it } from 'vitest'; + +import { ACTOR_NAME, USERNAME } from '@apify/consts'; + +import { ACTOR_FULL_NAME_REGEX } from '../../../../../../bin/utils/config/structures/base.js'; + +// ACTOR_FULL_NAME_REGEX is spliced together from two platform constants by stripping their anchors +// and joining them with a "/". These tests are about the splice, not about re-testing the platform's +// own rules: an anchor left behind, an anchor stripped too eagerly, or the actor-name alternation +// losing its parentheses would all produce a regex that still looks plausible and matches the wrong +// things. The cases below are the ones that tell those failures apart. +describe('ACTOR_FULL_NAME_REGEX', () => { + it.each([ + ['apify/web-scraper'], + ['myteam/shopify'], + ['my.team/web-scraper'], + ['my-team/shopify'], + ['my_team/a1'], + // ACTOR_NAME's first alternative — a single character — is a legal actor name on its own. + ['myteam/a'], + ['myteam/a--b'], + ])('accepts %s', (fullName) => { + expect(ACTOR_FULL_NAME_REGEX.test(fullName)).toBe(true); + }); + + it.each([ + ['there is no slash', 'shopify'], + ['the owner half is empty', '/shopify'], + ['the actor half is empty', 'myteam/'], + ['there are two slashes', 'myteam//shopify'], + ['there is a third segment', 'myteam/shopify/extra'], + ['the owner holds a space', 'my team/shopify'], + ['the actor name starts with a hyphen', 'myteam/-shopify'], + ['the actor name ends with a hyphen', 'myteam/shopify-'], + ['the actor name holds an underscore', 'myteam/web_scraper'], + ['the actor name holds a dot', 'myteam/web.scraper'], + ])('rejects a name where %s', (_name, fullName) => { + expect(ACTOR_FULL_NAME_REGEX.test(fullName)).toBe(false); + }); + + // Both halves arrive anchored and get de-anchored so they can be joined; the composed regex has to + // put the anchors back. Without them these all match on a substring. + it.each([ + ['leading text', 'see myteam/shopify'], + ['trailing text', 'myteam/shopify here'], + ['a leading newline', '\nmyteam/shopify'], + ['a trailing newline', 'myteam/shopify\n'], + ['surrounding whitespace', ' myteam/shopify '], + ])('matches the whole string, so it rejects %s', (_name, fullName) => { + expect(ACTOR_FULL_NAME_REGEX.test(fullName)).toBe(false); + }); + + // ACTOR_NAME's source is an alternation, "(A|B)". It survives the splice only because the platform + // wrote it parenthesized — lose those parens and the regex becomes "^owner/A|B$", where the "B" + // branch matches a bare actor name with no owner at all. + it('keeps the actor-name alternation from swallowing the owner half', () => { + expect(ACTOR_NAME.REGEX.source).toMatch(/^\^\(.*\)\$$/); + expect(ACTOR_FULL_NAME_REGEX.test('shopify')).toBe(false); + expect(ACTOR_FULL_NAME_REGEX.test('a')).toBe(false); + }); + + // The owner half carries a {min,max} quantifier, the one part of either source that a sloppy strip + // could eat. Bounds come from the constants so this stays honest if the platform moves them. + it("enforces the platform's username length bounds", () => { + const owner = (length: number) => `${'a'.repeat(length)}/shopify`; + + expect(ACTOR_FULL_NAME_REGEX.test(owner(USERNAME.MIN_LENGTH))).toBe(true); + expect(ACTOR_FULL_NAME_REGEX.test(owner(USERNAME.MIN_LENGTH - 1))).toBe(false); + expect(ACTOR_FULL_NAME_REGEX.test(owner(USERNAME.MAX_LENGTH))).toBe(true); + expect(ACTOR_FULL_NAME_REGEX.test(owner(USERNAME.MAX_LENGTH + 1))).toBe(false); + }); + + // `.source` carries the pattern and none of the flags, so the flags have to be re-applied by hand. + // USERNAME.REGEX is declared /i and the platform does accept "MyTeam" as a username; drop that on + // the way here and the owner half silently becomes stricter than the thing it was copied from. + it('accepts either half in any case, the way the platform does', () => { + expect(USERNAME.REGEX.flags).toContain('i'); + expect(ACTOR_FULL_NAME_REGEX.flags).toContain('i'); + + expect(ACTOR_FULL_NAME_REGEX.test('myteam/Shopify')).toBe(true); + expect(ACTOR_FULL_NAME_REGEX.test('MyTeam/shopify')).toBe(true); + }); +}); diff --git a/test/unit/bin/utils/config/structures/legacy.test.ts b/test/unit/bin/utils/config/structures/legacy.test.ts index 0b5b500..dab7dbb 100644 --- a/test/unit/bin/utils/config/structures/legacy.test.ts +++ b/test/unit/bin/utils/config/structures/legacy.test.ts @@ -43,7 +43,6 @@ describe('LEGACY_PARSER', () => { ['no slash', 'shopify-scraper'], ['an empty owner', '/shopify'], ['an empty name', 'myteam/'], - ['an uppercase owner', 'MyTeam/shopify'], ['more than two halves', 'myteam/shopify/extra'], ])('rejects an actorFullName with %s', (_name, actorFullName) => { expect(() => LEGACY_PARSER.parse({ actors: [entry({ actorFullName })] })).toThrow( @@ -51,7 +50,8 @@ describe('LEGACY_PARSER', () => { ); }); - it.each([['myteam/shopify'], ['my.team/web-scraper'], ['my_team/a1'], ['apify/web-scraper']])( + // Uppercase owners are legal on the platform; ./base.test.ts covers why that is easy to lose. + it.each([['myteam/shopify'], ['my.team/web-scraper'], ['my_team/a1'], ['apify/web-scraper'], ['MyTeam/shopify']])( 'accepts the actorFullName %s', (actorFullName) => { expect(() => LEGACY_PARSER.parse({ actors: [entry({ actorFullName })] })).not.toThrow(); From 82c91b7df5500b14a29522547097b10dbbfaf0b8 Mon Sep 17 00:00:00 2001 From: JuanGalilea Date: Mon, 21 Sep 2026 16:45:52 +0200 Subject: [PATCH 4/4] easier mode handling --- bin/utils/config/parser.ts | 17 ++++++++--------- test/unit/bin/utils/config/parser.test.ts | 6 +++--- 2 files changed, 11 insertions(+), 12 deletions(-) diff --git a/bin/utils/config/parser.ts b/bin/utils/config/parser.ts index 7d3fc80..7ec1c1a 100644 --- a/bin/utils/config/parser.ts +++ b/bin/utils/config/parser.ts @@ -10,18 +10,15 @@ const CONFIG_FILE_STRATEGIES: { [CONFIG_FILE_STRATEGY.LEGACY]: LEGACY_PARSER, } as const; -const ModeSelectionSchema = z.object({ - mode: z.enum(CONFIG_FILE_STRATEGY).default(CONFIG_FILE_STRATEGY.LEGACY), -}); +const ModeSelectionSchema = z.enum(CONFIG_FILE_STRATEGY).default(CONFIG_FILE_STRATEGY.LEGACY); -function selectStrategy(body: Record): StrategyParser { - const parsed = ModeSelectionSchema.safeParse(body); +function selectStrategy(mode: unknown): StrategyParser { + const parsed = ModeSelectionSchema.safeParse(mode); if (!parsed.success) { throw new Error(z.prettifyError(parsed.error)); } - const { mode } = parsed.data; - return CONFIG_FILE_STRATEGIES[mode]; + return CONFIG_FILE_STRATEGIES[parsed.data]; } // Strips a trailing slash so config-declared paths ("actors/shopify/" vs "actors/shopify") compare equal. @@ -82,8 +79,10 @@ export const _privates = { }; export function parseConfigFile(body: Record): ResolvedActorConfig[] { - const strategy = selectStrategy(body); - const resolved = strategy.parse(body); + const { mode, ...rest } = body; + const strategy = selectStrategy(mode); + + const resolved = strategy.parse(rest); const validated = verifyConfiguration(resolved); return validated; } diff --git a/test/unit/bin/utils/config/parser.test.ts b/test/unit/bin/utils/config/parser.test.ts index 5054260..5daa7bf 100644 --- a/test/unit/bin/utils/config/parser.test.ts +++ b/test/unit/bin/utils/config/parser.test.ts @@ -15,15 +15,15 @@ const actor = (fields: Record = {}) => ({ describe('selectStrategy', () => { it('falls back to the legacy strategy when no mode is declared', () => { - expect(selectStrategy({ actors: [] })).toBe(LEGACY_PARSER); + expect(selectStrategy(undefined)).toBe(LEGACY_PARSER); }); it('honours an explicit mode', () => { - expect(selectStrategy({ mode: CONFIG_FILE_STRATEGY.LEGACY, actors: [] })).toBe(LEGACY_PARSER); + expect(selectStrategy(CONFIG_FILE_STRATEGY.LEGACY)).toBe(LEGACY_PARSER); }); it('throws on a mode no strategy is registered for', () => { - expect(() => selectStrategy({ mode: 'brand-new' })).toThrow(/at mode/); + expect(() => selectStrategy('some-nonexistent-mode')).toThrow(/Invalid input/); }); });