refactor(readConfigFile): rework config file reading and testing by JuanGalilea · Pull Request #139 · apify/apify-test-tools

JuanGalilea · 2026-09-17T12:59:15Z

PR number one of the setup for config file reworks.

Featuring:

Tons of fixtures
clear cut point for new config file parsing
rework of readConfigFile passing previous mock-based test battery and new fixture-based one
some config fixes like .vscode and tsconfig misconfigurations

ruocco-l

Just some questions, but looks ok to me, thanks!

ruocco-l · 2026-09-17T13:25:42Z

+ * 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 denormalizeConfig = (config: ActorConfigFile): DenormalizedActor[] => {


Why is it called denormalize if it normalizes (according to the JSDoc)?

it normalizes the paths but the action as a whole is denormalizing (tbh i didn't read the jsdoc my bad my bad).
Will fix it

ruocco-l · 2026-09-17T13:32:20Z

+    const seenFolders = new Set<string>();
+
+    return config.actors.map((entry) => {
+        const folder = entry.folder === '.' ? '' : stripTrailingSlash(entry.folder);


I know that this was already here, but i realized that you can trick this by using "./" in the config file. Can you fix it now, since we are touching on it?

i'll fix it in the path resolution pr (PR number 3)
Since there is quite a bit of dubious stuff around paths

ruocco-l · 2026-09-17T13:54:01Z

    });
-};
+
+    it('loadActorConfig resolves an entry that never came from a config file', async () => {


I don't understand what this is supposed to mean

me neither, fixed

metalwarrior665

The flow is nice, seems like something we could stick to long term. Just few polishing suggestions.

metalwarrior665 · 2026-09-17T15:13:29Z

+ * trace left of which config flavour produced it. Whatever replaces or extends
+ * {@link parseConfigFile} only has to produce this.
+ */
+export interface DenormalizedActor {


I think the naming will need some work. One is Actor and other is ActorConfig, so shouldn't this be DenormalizedActorConfig?

Do we need this ugly term? We could also do something like ActorConfigBase -> ActorConfigComplete (or similar)? Or perhaps ActorConfigWithSchemas as that is what it does for now

true

i mean it is the right term. Maybe ResolvedActorConfig?

sounds good

metalwarrior665 · 2026-09-17T15:15:19Z

+
+// 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 || '.';


I would inline this or at least define it closer to where used.

metalwarrior665 · 2026-09-17T15:16:40Z

+// would have written it.
+const displayFolder = (folder: string): string => folder || '.';
+
+type ConfigIssue = z.ZodError<ActorConfigFile>['issues'][number];


Is issue an official term? I would think more about validationError or at least validationIssue

set to ConfigParsingIssue since its parsing* not validation whats going on

metalwarrior665 · 2026-09-17T15:18:23Z

+
+    switch (file.reason) {
+        case 'invalid-json':
+            throw new Error(`Config file "${CONFIG_FILE_NAME}" contains invalid JSON.`);


Add a comment we might want to support jsonc in the future

metalwarrior665 · 2026-09-17T15:22:18Z

+ */
+export async function safeReadJsonObjectFile(path: string): Promise<JsonObjectReadResult> {
+    // `stat`, not `lstat`: a symlink must resolve to its target, matching what `readFile` does below.
+    const stats = await stat(path).catch(() => null);


Let's use sync IO, so we don't have to propagate async everywhere. From outside async looks like we will do some network calls.

This is a sequential step in CLI, we don't need wide concurrency like a web server.

YES!!! i did not touch it due to chesterton's fence. but it bugged me the whole time

metalwarrior665 · 2026-09-17T15:24:17Z

+    // replace these two when enabling different file structures
+    // some kind of strategy pattern seems appropriate here
+    const parsedConfigFile = parseConfigFile(rawConfigFile);
+    const entries = denormalizeConfig(parsedConfigFile);


Again the variables don't follow. parsedConfigFile -> entries -> actorConfigs. They should derive from one another.

metalwarrior665 · 2026-09-17T15:25:04Z

@@ -0,0 +1,57 @@
+import { readFile, stat } from 'node:fs/promises';


files.ts is too generic. How about json-object.ts. We can make more files for other things later

metalwarrior665 · 2026-09-17T15:26:00Z

@@ -0,0 +1,251 @@
+import path from 'node:path';


Instead of having a utils dump, we could just have bin/config-parsers/. We will also add the actor.json parsing to it is already 3 related files.

i was about to do this now since i started doing groundwork for the multiple config structures and noticed it as an issue.
Will solve it here.

metalwarrior665 · 2026-09-17T15:27:18Z

+// 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 => {


Next time it would be good to add PR comment that this function was moved as is so I don't have to read it. Not sure if there is some easy hack for me to do this...

sometimes github detects it, but its really hit or miss

refactor(readConfigFile): rework config file reading and testing

7c6590c

JuanGalilea self-assigned this Sep 17, 2026

JuanGalilea requested review from metalwarrior665 and ruocco-l as code owners September 17, 2026 12:59

ruocco-l approved these changes Sep 17, 2026

View reviewed changes

metalwarrior665 requested changes Sep 17, 2026

View reviewed changes

pr comments

87e99f9

JuanGalilea requested a review from metalwarrior665 September 18, 2026 11:02

metalwarrior665 approved these changes Sep 18, 2026

View reviewed changes

JuanGalilea merged commit 3299da9 into master Sep 18, 2026
9 checks passed

JuanGalilea deleted the refactor/read-config branch September 21, 2026 08:47

		@@ -0,0 +1,57 @@
		import { readFile, stat } from 'node:fs/promises';

Conversation

JuanGalilea commented Sep 17, 2026

Uh oh!

ruocco-l left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

metalwarrior665 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JuanGalilea Sep 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

JuanGalilea Sep 18, 2026 •

edited

Loading