Skip to content

[Tests] Split the 32-hex secret test into one test per assignment form - #8928

Merged
gonzaloriestra merged 1 commit into
mainfrom
tests-maintenance-38829079667
Oct 23, 2026
Merged

gonzaloriestra merged 1 commit into
mainfrom
tests-maintenance-38829079667

Conversation

@github-actions

Copy link
Copy Markdown
Contributor

WHY are these changes introduced?

The tests-main.yml Windows job failed on main on 2026-10-15 with:

FAIL @shopify/app src/cli/services/app-security-engine/tests/secret-safety.test.ts >
  committed secret classification >
  does not score a 32-hex value under a secret-sounding name, quoted or not
Error: Test timed out in 13000ms.

Representative job: [Main] Node 24.1.0 in windows-latest — https://github.com/Shopify/cli/actions/runs/38766274961/job/116530178563

The test body loops over two assignment forms, and each iteration builds a real temporary app, runs git init / add / commit, and then performs a full directory scan. Both iterations are charged to one test timeout, so the test costs roughly twice what a single case costs while getting the same 13s budget as its single-case siblings.

The measurements support that reading. Across every windows-latest job in tests-main.yml created between 2026-10-09 and 2026-10-16 UTC (48 runs, 144 Windows jobs, all 144 reporting a duration for this test), it ran in 534ms at the fastest and 15508ms at the slowest, with a median of 960ms and a p90 of 1406ms. The single-case sibling does not score a value identified only by a secret-sounding key name ran over the same jobs with a median of 497ms and a max of 9955ms. In the failing job specifically, the two single-case siblings took 9955ms and 8793ms while this two-case test took 15508ms — on a runner where a single git-and-scan case costs about 9s, two of them cannot fit in 13s.

WHAT is this pull request doing?

Splits the test into one test per assignment form, driven by a small [label, assignment] table. Each case now gets its own timeout, so a runner slow enough to make one case take 9s no longer fails the file.

Both assertions are preserved exactly — the quoted and unquoted forms are still each committed to a real git repository and scanned, and each still asserts that no COMMITTED_SECRET issue is produced. Nothing is skipped, no assertion is weakened, and no timeout is raised: the fix removes the doubled work from a single budget rather than widening the budget to accommodate it. The file count goes from 47 to 48 tests because the one test became two. The table-driven shape matches the samples loop already used higher up in the same file, and dropping the loop also removes the no-await-in-loop suppression it needed.

Validation: the file passes locally on Linux Node 26.1.0 (48 passed), five times under --sequence.shuffle, and alongside all 25 app-security test files (570 passed, 2 skipped — the two skipIf-gated win32 shell tests). nx type-check app and ESLint on the changed file both pass.

Remaining uncertainty: the originally failing platform is windows-latest, which I could not run locally, so the cause is established from CI duration measurements rather than by reproducing the timeout on Windows. This also does not address the other failure in that same job, the PowerShell record test in app-security-commands.test.ts, which is a separate flake already covered by #8832.

How to manually test your changes?

shopify app security check

Checklist

  • I've considered possible cross-platform impacts (Mac, Linux, Windows)
  • I've considered possible documentation changes
  • I've considered analytics changes to measure impact
  • The change is user-facing — I've identified the correct bump type (patch for bug fixes · minor for new features · major for breaking changes) and added a changeset with pnpm changeset add

The test ran two full git-repo-and-scan cases inside a single test body, so
it paid twice the per-case cost against one 13s timeout and timed out on the
windows-latest matrix. Each form is now its own test with its own budget.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown
Contributor Author

Differences in type declarations

We detected differences in the type declarations generated by Typescript for this branch compared to the baseline ('main' branch). Please, review them to ensure they are backward-compatible. Here are some important things to keep in mind:

  • Some seemingly private modules might be re-exported through public modules.
  • If the branch is behind main you might see odd diffs, rebase main into this branch.

New type declarations

packages/cli-kit/dist/private/node/ui/components/TextAnimation.d.ts
import React from 'react';
interface TextAnimationProps {
    text: string;
    maxWidth?: number;
}
/**
 * `TextAnimation` applies a rainbow animation to text.
 */
declare const TextAnimation: React.MemoExoticComponent<({ text, maxWidth }: TextAnimationProps) => JSX.Element>;
export { TextAnimation };

Existing type declarations

packages/cli-kit/dist/private/node/api.d.ts
@@ -60,8 +60,6 @@ export declare function simpleRequestWithDebugLog<T extends {
  * Makes a HTTP request to some API, retrying if response headers indicate a retryable error.
  *
  * If a request fails with a 429, the retry-after header determines a delay before an automatic retry is performed.
- * Without a valid header, rate-limit retries use exponential backoff with jitter, capped at 8 seconds per delay.
- * No new rate-limit retry starts after 30 seconds. A server-requested delay beyond that budget is not shortened.
  *
  * If unauthorizedHandler is provided, then it will be called in the case of a 401 and a retry performed. This allows
  * for a token refresh for instance.
packages/cli-kit/dist/public/node/base-command.d.ts
@@ -1,5 +1,4 @@
 import { type JsonOutputSchema } from './json-output-schema.js';
-import { JsonMap } from '../../private/common/json.js';
 import { Command } from '@oclif/core';
 import { OutputFlags, Input, ParserOutput, FlagInput, OutputArgs } from '@oclif/core/parser';
 export type ArgOutput = OutputArgs<any>;
@@ -21,7 +20,6 @@ declare abstract class BaseCommand extends Command {
     static descriptionWithoutMarkdown(): string | undefined;
     static analyticsNameOverride(): string | undefined;
     static analyticsStopCommand(): string | undefined;
-    private jsonOutputFromFlags;
     catch(error: Error & {
         skipOclifErrorHandling: boolean;
     }): Promise<void>;
@@ -37,10 +35,7 @@ declare abstract class BaseCommand extends Command {
     }, TGlobalFlags extends FlagOutput, TArgs extends ArgOutput>(options?: Input<TFlags, TGlobalFlags, TArgs>, argv?: string[]): Promise<ParserOutput<TFlags, TGlobalFlags, TArgs> & {
         argv: string[];
     }>;
-    protected runWithParsedOutput<T>(flags: FlagOutput, execute: () => T): T;
     protected environmentsFilename(): string | undefined;
-    protected parseEnvironmentFlags<TFlags extends FlagOutput, TGlobalFlags extends FlagOutput, TArgs extends ArgOutput>(environment: JsonMap, options: Input<TFlags, TGlobalFlags, TArgs>, explicitFlags: FlagOutput): Promise<FlagOutput>;
-    protected validateNonTTYFlags(flags: FlagOutput): void;
     protected failMissingNonTTYFlags(flags: FlagOutput, requiredFlags: string[]): void;
     private failMissingNonTTYFlagRequirements;
     private applicableNonTTYFlagRequirements;
packages/cli-kit/dist/public/node/system.d.ts
@@ -8,8 +8,6 @@ export interface ExecOptions {
     stderr?: Writable | 'inherit';
     stdio?: 'inherit';
     input?: string;
-    /** Whether to remove the final newline from captured stdout/stderr (default: true). */
-    stripFinalNewline?: boolean;
     signal?: AbortSignal;
     externalErrorHandler?: (error: unknown) => Promise<void>;
     background?: boolean;
@@ -44,7 +42,6 @@ export interface CaptureOutputResult {
 /**
  * Runs a command asynchronously and returns stdout, stderr, and exit code.
  * Unlike captureOutput, this function does NOT throw on non-zero exit codes.
- * Startup failures and signal termination still throw an ExternalError.
  *
  * @param command - Command to be executed.
  * @param args - Arguments to pass to the command.
@@ -64,7 +61,6 @@ export declare function captureOutputWithExitCode(command: string, args: string[
  * Runs a command string asynchronously and returns stdout, stderr, and exit code.
  * Parses the command string into command and arguments (handles quoted strings).
  * Unlike captureOutput, this function does NOT throw on non-zero exit codes.
- * Startup failures and signal termination still throw an ExternalError.
  *
  * @param command - Full command string to be executed (e.g., 'ls -la "my folder"').
  * @param options - Optional settings for how to run the command.
packages/cli-kit/dist/public/node/ui.d.ts
@@ -325,9 +325,9 @@ export interface RenderSingleTaskOptions<T> {
     renderOptions?: RenderOptions;
 }
 /**
- * Awaits a single task and displays a loading indicator while it's in progress. The task's result is returned.
+ * Awaits a single task and displays a loading bar while it's in progress. The task's result is returned.
  * @param options - Configuration object
- * @param options.title - The initial title to display with the loading indicator
+ * @param options.title - The initial title to display with the loading bar
  * @param options.task - The async task to execute. Receives an updateStatus callback to change the displayed title.
  * @param options.retry - The number of additional attempts after a failure. Defaults to zero.
  * @param options.renderOptions - Optional render configuration
@@ -375,30 +375,10 @@ export interface RenderDangerousConfirmationPromptOptions extends Omit<Dangerous
  *
  */
 export declare function renderDangerousConfirmationPrompt({ renderOptions, ...props }: RenderDangerousConfirmationPromptOptions, uiDebugOptions?: UIDebugOptions): Promise<boolean>;
-interface KeypressOptions {
-    /**
-     * The keys that end the wait, for example `['\r']` for Enter. Other keys are ignored.
-     * Without this option, any key ends the wait.
-     */
-    keys?: string[];
-}
-/**
- * Waits for a key to be pressed. Ctrl+C rejects with AbortSilentError, which terminates the process.
- *
- * Raw mode is set back to the state it had before the call. A command that keeps stdin raw for
- * its own single-key shortcuts (for example `theme dev`) keeps them.
- *
- * Chunks are read as text: an active Ink renderer calls `stdin.setEncoding('utf8')`, and then
- * stdin emits strings instead of Buffers.
- *
- * @param stdin - The stream to read from.
- * @param uiDebugOptions - Debug options; `skipTTYCheck` allows a non-TTY stdin.
- * @param options - The keys that end the wait.
- * @returns A promise that resolves when an accepted key is pressed.
- */
+/** Waits for any key to be pressed except Ctrl+C which will terminate the process. */
 export declare const keypress: (stdin?: NodeJS.ReadStream & {
     fd: 0;
-}, uiDebugOptions?: UIDebugOptions, { keys }?: KeypressOptions) => Promise<void>;
+}, uiDebugOptions?: UIDebugOptions) => Promise<unknown>;
 interface IsTTYOptions {
     stdin?: NodeJS.ReadStream;
     uiDebugOptions?: UIDebugOptions;
packages/cli-kit/dist/private/node/api/graphql.d.ts
@@ -1,4 +1,4 @@
-import { ClientError, Variables } from 'graphql-request';
+import { Variables } from 'graphql-request';
 export declare function debugLogRequestInfo(api: string, query: string, url: string, variables?: Variables, headers?: Record<string, string>): void;
 export declare function sanitizeVariables(variables: Variables): string;
 /**
@@ -11,5 +11,4 @@ export declare function sanitizeVariables(variables: Variables): string;
  * the full JSON representation if no messages can be extracted.
  */
 export declare function extractGraphQLErrorMessages(errors: any[] | undefined): string | undefined;
-export declare function errorHandler(api: string): (error: unknown, requestId?: string) => unknown;
-export declare function extractGraphQLErrors(error: unknown): NonNullable<ClientError['response']['errors']>;
\ No newline at end of file
+export declare function errorHandler(api: string): (error: unknown, requestId?: string) => unknown;
\ No newline at end of file
packages/cli-kit/dist/public/node/themes/api.d.ts
@@ -34,15 +34,6 @@ export declare function metafieldDefinitionsByOwnerType(type: MetafieldOwnerType
     };
 }[]>;
 export declare function passwordProtected(session: AdminSession): Promise<boolean>;
-/**
- * Whether the error ends a Trust Battery challenge on a theme request. A caller that
- * retries theme requests must rethrow it, or the user is asked to verify again for
- * each attempt.
- *
- * @param error - The error to check.
- * @returns True when the error ends a Trust Battery challenge.
- */
-export declare function isTrustChallengeError(error: unknown): boolean;
 type OnlineStoreThemeFileBody = {
     __typename: 'OnlineStoreThemeFileBodyBase64';
     contentBase64: string;
packages/cli-kit/dist/public/node/themes/theme-manager.d.ts
@@ -11,12 +11,5 @@ export declare abstract class ThemeManager {
     findOrCreate(name?: string, role?: Role): Promise<Theme>;
     fetch(name?: string, role?: Role): Promise<Theme | undefined>;
     generateThemeName(context: string): string;
-    create(themeRole?: Role, themeName?: string): Promise<{
-        createdAtRuntime: boolean;
-        id: number;
-        name: string;
-        processing: boolean;
-        role: string;
-        src?: string;
-    }>;
+    create(themeRole?: Role, themeName?: string): Promise<Theme>;
 }
\ No newline at end of file
packages/cli-kit/dist/public/node/ui/components.d.ts
@@ -1,5 +1,4 @@
 export { ConcurrentOutput, ConcurrentOutputContext, useConcurrentOutputContext, } from '../../../private/node/ui/components/ConcurrentOutput.js';
 export { Alert } from '../../../private/node/ui/components/Alert.js';
 export { Link } from '../../../private/node/ui/components/Link.js';
-export { LoadingIndicator } from '../../../private/node/ui/components/LoadingIndicator.js';
 export { TabularData } from '../../../private/node/ui/components/TabularData.js';
\ No newline at end of file
packages/cli-kit/dist/cli/api/graphql/admin/generated/metafield_definitions_by_owner_type.d.ts
@@ -2,14 +2,9 @@ import * as Types from './types.js';
 import { TypedDocumentNode as DocumentNode } from '@graphql-typed-document-node/core';
 export type MetafieldDefinitionsByOwnerTypeQueryVariables = Types.Exact<{
     ownerType: Types.MetafieldOwnerType;
-    after?: Types.InputMaybe<Types.Scalars['String']['input']>;
 }>;
 export type MetafieldDefinitionsByOwnerTypeQuery = {
     metafieldDefinitions: {
-        pageInfo: {
-            hasNextPage: boolean;
-            endCursor?: string | null;
-        };
         nodes: {
             key: string;
             name: string;
packages/cli-kit/dist/private/node/ui/components/ConcurrentOutput.d.ts
@@ -1,4 +1,3 @@
-import { ConcurrentOutputContext, getConcurrentOutputContext, useConcurrentOutputContext } from '../../output-context.js';
 import { type OutputProcess } from '../../../../public/node/output.js';
 import { AbortSignal } from '../../../../public/node/abort.js';
 import { FunctionComponent } from 'react';
@@ -14,6 +13,12 @@ export interface ConcurrentOutputProps {
     keepRunningAfterProcessesResolve?: boolean;
     useAlternativeColorPalette?: boolean;
 }
+interface ConcurrentOutputContext {
+    outputPrefix?: string;
+    /** Controls ANSI stripping for terminal output. JSON diagnostics are always unstyled. */
+    stripAnsi?: boolean;
+}
+declare function useConcurrentOutputContext<T>(context: ConcurrentOutputContext, callback: () => T): T;
 /** Runs finite processes concurrently and routes their output through the shared diagnostic context. */
 export declare function runConcurrentProcessesForJson({ processes, abortSignal, }: Pick<ConcurrentOutputProps, 'processes' | 'abortSignal'>): Promise<void>;
 /**
@@ -50,4 +55,4 @@ export declare function runConcurrentProcessesForJson({ processes, abortSignal,
  * ```
  */
 declare const ConcurrentOutput: FunctionComponent<ConcurrentOutputProps>;
-export { ConcurrentOutput, ConcurrentOutputContext, getConcurrentOutputContext, useConcurrentOutputContext };
\ No newline at end of file
+export { ConcurrentOutput, ConcurrentOutputContext, useConcurrentOutputContext };
\ No newline at end of file

@gonzaloriestra
gonzaloriestra marked this pull request as ready for review October 23, 2026 10:00
@gonzaloriestra
gonzaloriestra requested a review from a team as a code owner October 23, 2026 10:00
@gonzaloriestra
gonzaloriestra added this pull request to the merge queue Oct 23, 2026
Merged via the queue into main with commit 1a8c51c Oct 23, 2026
28 of 29 checks passed
@gonzaloriestra
gonzaloriestra deleted the tests-maintenance-38829079667 branch October 23, 2026 10:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-changelog This PR doesn't include a changeset entry. Is an internal only change not relevant to end users.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant