Add doctor command
#6798
Add doctor command
#6798
Conversation
Coverage report
Test suite run success3756 tests passing in 1453 suites. Report generated by 🧪jest coverage report action from 5597fcc |
EvilGenius13
force-pushed
the
jf-audit-proto
branch
6 times, most recently
from
440b7c9 to
ea4eb79
Compare
|
We detected some changes at Caution DO NOT create changesets for features which you do not wish to be included in the public changelog of the next CLI release. |
karreiro
left a comment
karreiro
left a comment
There was a problem hiding this comment.
Thank you for this PR, @EvilGenius13!
I’ve left a few comments and suggestions. I also think it’d be worth adding unit tests around the test framework to make sure the modules behave as intended.
Thanks again for the PR!
| command: string, | ||
| options?: {cwd?: string; env?: {[key: string]: string}}, | ||
| ): Promise<CommandResult> { | ||
| const parts = command.split(' ').filter(Boolean) |
There was a problem hiding this comment.
It seems like we have an issue here for flags with spaces. For example:
const command = 'shopify theme push --theme "My Theme Name"';
const parts = command.split(' ').filter(Boolean);
// Expected: ['shopify', 'theme', 'push', '--theme', 'My Theme Name']
// Actual: ['shopify', 'theme', 'push', '--theme', '"My', 'Theme', 'Name"']In the packages/cli/src/cli/services/audit/theme/tests/init.ts file, tests stop working if the API user updates the theme name to this:
this.themeName = `My audit-theme-${getRandomName('creative')}`Since the theme name can include spaces, the current split(' ') parsing ends up breaking the --theme value into multiple parts.
There was a problem hiding this comment.
Fixed parsing to gather themes with spaces.
| private themeName = '' | ||
| private themePath = '' | ||
|
|
||
| // eslint-disable-next-line @typescript-eslint/naming-convention |
There was a problem hiding this comment.
I don't think we should promote the pattern of always eslint-ignoring all test cases.
We may solve this by adopting a pattern like this for test definition:
test('test init creates theme directory', async () => {
…
})Or, we may ignore this warning globally in this module (still, I personally prefer the test(...) API as it follows a more well-established convention when it comes to unit tests in JS/TS frameworks).
There was a problem hiding this comment.
switched to the test method!
| if (error instanceof Error) { | ||
| stderr = error.message | ||
| // Try to extract exit code from error | ||
| const match = /exit code (\d+)/i.exec(error.message) |
There was a problem hiding this comment.
It seems a bit fragile to extract the error code based on the output string.
The captureOutput function calls buildExec that results the output and the exit code.
I wonder if we may update packages/cli-kit/src/public/node/system.ts with a new function that returns the output and the exit status code.
|
/snapit |
|
🫰✨ Thanks @karreiro! Your snapshot has been published to npm. Test the snapshot by installing your package globally: npm i -g --@shopify:registry=https://registry.npmjs.org @shopify/cli@0.0.0-snapshot-20260204112710Caution After installing, validate the version by running just |
karreiro
left a comment
karreiro
left a comment
There was a problem hiding this comment.
Thank you for this PR, @EvilGenius13!
I left a few minor comments as next steps, but I think we’re in a good spot to merge this as a first iteration 🚀
| body: [ | ||
| 'Available doctor commands:', | ||
| '', | ||
| ' shopify doctor theme -e <environment> Run all theme command tests', | ||
| '', | ||
| 'The -e/--environment flag is required to specify the store configuration.', | ||
| 'Use --help with any command for more options.', | ||
| ], |
There was a problem hiding this comment.
It doesn't seem like the output here is formatted as expected (we can use TokenItem to break lines and highlight commands):
(we may handle this in follow-up PR)
There was a problem hiding this comment.
Yes absolutely, will handle in a follow up PR.
| for (const file of essentialFiles) { | ||
| // eslint-disable-next-line no-await-in-loop | ||
| await this.assertFile(joinPath(this.themePath, file)) | ||
| } |
There was a problem hiding this comment.
May we resolve promises here to avoid the the eslint ignore?
There was a problem hiding this comment.
Originally I had them this way in case of consistent failures, the results would come back in order (such as missing files) but considering this is handling the order of a missing file or folder on an error, I switched to Promise.all
| const directories = ['sections', 'snippets', 'assets', 'locales'] | ||
|
|
||
| for (const dir of directories) { | ||
| // eslint-disable-next-line no-await-in-loop |
There was a problem hiding this comment.
May we resolve promises here too?
There was a problem hiding this comment.
Same as above comment
| if (this.context.environment) { | ||
| cmd += ` -e ${this.context.environment}` | ||
| } | ||
| if (this.context.store) { | ||
| cmd += ` -s ${this.context.store}` | ||
| } | ||
| if (this.context.password) { | ||
| cmd += ` --password ${this.context.password}` | ||
| } |
There was a problem hiding this comment.
Given the framework requires the -e flag to run the test suite, and that this is something that's going to happen with all commands that require authentication, I believe we should try to extract this early to another place.
(we may apply this in a follow-up PR tho)
There was a problem hiding this comment.
I can add this in a follow up.
|
/snapit |
|
🫰✨ Thanks @EvilGenius13! Your snapshot has been published to npm. Test the snapshot by installing your package globally: npm i -g --@shopify:registry=https://registry.npmjs.org @shopify/cli@0.0.0-snapshot-20260204185116Caution After installing, validate the version by running just |
|
Added an extra commit changing how we handle filepaths in the command as it would fail on windows. The latest commit is working cross OS |
|
hmm, should we consider some command naming that is more aligned with its internal use, even though it is hidden? People do discover these things and I'm not sure we want people running our tests? Any additional knee-high walls? Requiring 1P login, or setting an environment variable for the tests to execute? |
Absolutely. We will rename to |
EvilGenius13
force-pushed
the
jf-audit-proto
branch
3 times, most recently
from
39be584 to
78247d9
Compare
|
/snapit |
|
🫰✨ Thanks @EvilGenius13! Your snapshot has been published to npm. Test the snapshot by installing your package globally: npm i -g --@shopify:registry=https://registry.npmjs.org @shopify/cli@0.0.0-snapshot-20260206185509Caution After installing, validate the version by running just |
gonzaloriestra
left a comment
gonzaloriestra
left a comment
There was a problem hiding this comment.
This is so cool, thanks! It will be useful for the apps as well 🎉
| * @param command - Full command string to be executed (e.g., 'ls -la "my folder"'). | ||
| * @param options - Optional settings for how to run the command. | ||
| */ | ||
| export async function execCommand(command: string, options?: ExecOptions): Promise<void> { |
There was a problem hiding this comment.
I wonder why we didn't do this from the beginning with exec 😄
There was a problem hiding this comment.
As there are other CLIs building on top of cli-kit (like hydrogen), what about moving these files there so they can use it?
There was a problem hiding this comment.
Let me look into doing that! This might end up being a follow up PR.
There was a problem hiding this comment.
I take it back, I'll move it before merging.
| static hidden = true | ||
|
|
||
| async run(): Promise<void> { | ||
| if (!firstPartyDev()) { |
There was a problem hiding this comment.
For now it's an added piece of security. Even though the command is hidden, in the event someone stumbled on the command, they would still need to run with the additional environment variable just to make sure they were intentional about using the command. (It's mostly to protect anyone as the command will start to run more commands that will change remote themes in a person's shop (like running theme delete, theme dev etc)
There was a problem hiding this comment.
What about checking isShopify() instead? Or alternatively, creating a new env var for this (they are free!)
There was a problem hiding this comment.
I went with a new env var SHOPIFY_CLI_DOCTOR
EvilGenius13
force-pushed
the
jf-audit-proto
branch
3 times, most recently
from
91e6e2a to
d6f7d90
Compare
EvilGenius13
force-pushed
the
jf-audit-proto
branch
from
d6f7d90 to
5597fcc
Compare
Differences in type declarationsWe 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:
New type declarationspackages/cli-kit/dist/public/node/doctor/framework.d.tsimport type { DoctorContext, TestResult } from './types.js';
/**
* Result from running a CLI command.
*/
interface CommandResult {
/** The full command that was run */
command: string;
/** Exit code (0 = success) */
exitCode: number;
/** Standard output */
stdout: string;
/** Standard error */
stderr: string;
/** Combined output (stdout + stderr) */
output: string;
/** Whether the command succeeded (exitCode === 0) */
success: boolean;
}
/**
* Base class for doctor test suites.
*
* Write tests using the test() method:.
*
* ```typescript
* export default class MyTests extends DoctorSuite {
* static description = 'My test suite'
*
* tests() {
* this.test('basic case', async () => {
* const result = await this.run('shopify theme init')
* this.assertSuccess(result)
* })
*
* this.test('error case', async () => {
* const result = await this.run('shopify theme init --invalid')
* this.assertError(result, /unknown flag/)
* })
* }
* }
* ```
*/
export declare abstract class DoctorSuite<TContext extends DoctorContext = DoctorContext> {
static description: string;
protected context: TContext;
private assertions;
private registeredTests;
/**
* Run the entire test suite.
*
* @param context - The doctor context for this suite run.
*/
runSuite(context: TContext): Promise<TestResult[]>;
/**
* Register a test with a name and function.
*
* @param name - The test name.
* @param fn - The async test function.
*/
protected test(name: string, fn: () => Promise<void>): void;
/**
* Override this method to register tests using this.test().
*/
protected tests(): void;
/**
* Run a CLI command and return the result.
*
* @param command - The CLI command to run.
* @param options - Optional cwd and env overrides.
* @example
* const result = await this.run('shopify theme init my-theme')
* const result = await this.run('shopify theme push --json')
*/
protected run(command: string, options?: {
cwd?: string;
env?: {
[key: string]: string;
};
}): Promise<CommandResult>;
/**
* Run a command without capturing output (for interactive commands).
* Returns only success/failure.
*
* @param command - The CLI command to run.
* @param options - Optional cwd and env overrides.
*/
protected runInteractive(command: string, options?: {
cwd?: string;
env?: {
[key: string]: string;
};
}): Promise<CommandResult>;
/**
* Assert that a command succeeded (exit code 0).
*
* @param result - The command result to check.
* @param message - Optional custom assertion message.
*/
protected assertSuccess(result: CommandResult, message?: string): void;
/**
* Assert that a command failed with an error matching the pattern.
*
* @param result - The command result to check.
* @param pattern - Optional regex or string pattern to match against output.
* @param message - Optional custom assertion message.
*/
protected assertError(result: CommandResult, pattern?: RegExp | string, message?: string): void;
/**
* Assert that a file exists and optionally matches content.
*
* @param path - The file path to check.
* @param contentPattern - Optional regex or string to match file content.
* @param message - Optional custom assertion message.
*/
protected assertFile(path: string, contentPattern?: RegExp | string, message?: string): Promise<void>;
/**
* Assert that a file does not exist.
*
* @param path - The file path to check.
* @param message - Optional custom assertion message.
*/
protected assertNoFile(path: string, message?: string): Promise<void>;
/**
* Assert that a directory exists.
*
* @param path - The directory path to check.
* @param message - Optional custom assertion message.
*/
protected assertDirectory(path: string, message?: string): Promise<void>;
/**
* Assert that output contains a pattern.
*
* @param result - The command result to check.
* @param pattern - Regex or string pattern to match against output.
* @param message - Optional custom assertion message.
*/
protected assertOutput(result: CommandResult, pattern: RegExp | string, message?: string): void;
/**
* Assert that output contains valid JSON and optionally validate it.
*
* @param result - The command result to parse.
* @param validator - Optional function to validate the parsed JSON.
* @param message - Optional custom assertion message.
*/
protected assertJson<T = unknown>(result: CommandResult, validator?: (json: T) => boolean, message?: string): T | undefined;
/**
* Assert a boolean condition.
*
* @param condition - The boolean condition to assert.
* @param message - The assertion description.
*/
protected assert(condition: boolean, message: string): void;
/**
* Assert two values are equal.
*
* @param actual - The actual value.
* @param expected - The expected value.
* @param message - The assertion description.
*/
protected assertEqual<T>(actual: T, expected: T, message: string): void;
private hasFailures;
}
export {};
packages/cli-kit/dist/public/node/doctor/reporter.d.tsimport type { TestResult } from './types.js';
/**
* Initialize the reporter with a base path for truncating file paths in output.
* Call this before running tests to enable path truncation.
*/
export declare function initReporter(basePath: string): void;
export declare function reportSuiteStart(suiteName: string, description: string): void;
export declare function reportTestStart(testName: string): void;
export declare function reportTestResult(result: TestResult): void;
export declare function reportSummary(results: TestResult[]): void;
packages/cli-kit/dist/public/node/doctor/types.d.tstype TestStatus = 'passed' | 'failed' | 'skipped';
export interface AssertionResult {
description: string;
passed: boolean;
expected?: unknown;
actual?: unknown;
}
export interface TestResult {
name: string;
status: TestStatus;
duration: number;
assertions: AssertionResult[];
error?: Error;
}
export interface DoctorContext {
workingDirectory: string;
data: {
[key: string]: unknown;
};
}
export {};
Existing type declarationspackages/cli-kit/dist/private/node/constants.d.ts@@ -3,6 +3,7 @@ export declare const environmentVariables: {
alwaysLogAnalytics: string;
alwaysLogMetrics: string;
deviceAuth: string;
+ doctor: string;
enableCliRedirect: string;
env: string;
firstPartyDev: string;
packages/cli-kit/dist/public/node/system.d.ts@@ -30,6 +30,50 @@ export declare function openURL(url: string): Promise<boolean>;
* @returns A promise that resolves with the aggregatted stdout of the command.
*/
export declare function captureOutput(command: string, args: string[], options?: ExecOptions): Promise<string>;
+/**
+ * Result from running a command with captureOutputWithExitCode.
+ */
+export interface CaptureOutputResult {
+ /** Standard output. */
+ stdout: string;
+ /** Standard error. */
+ stderr: string;
+ /** Exit code (0 = success). */
+ exitCode: number;
+}
+/**
+ * Runs a command asynchronously and returns stdout, stderr, and exit code.
+ * Unlike captureOutput, this function does NOT throw on non-zero exit codes.
+ *
+ * @param command - Command to be executed.
+ * @param args - Arguments to pass to the command.
+ * @param options - Optional settings for how to run the command.
+ * @returns A promise that resolves with stdout, stderr, and exitCode.
+ *
+ * @example
+ *
+ */
+export declare function captureOutputWithExitCode(command: string, args: string[], options?: ExecOptions): Promise<CaptureOutputResult>;
+/**
+ * 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.
+ *
+ * @param command - Full command string to be executed (e.g., 'ls -la "my folder"').
+ * @param options - Optional settings for how to run the command.
+ * @returns A promise that resolves with stdout, stderr, and exitCode.
+ *
+ * @example
+ *
+ */
+export declare function captureCommandWithExitCode(command: string, options?: ExecOptions): Promise<CaptureOutputResult>;
+/**
+ * Runs a command string asynchronously (parses command and arguments from the string).
+ *
+ * @param command - Full command string to be executed (e.g., 'ls -la "my folder"').
+ * @param options - Optional settings for how to run the command.
+ */
+export declare function execCommand(command: string, options?: ExecOptions): Promise<void>;
/**
* Runs a command asynchronously.
*
packages/cli-kit/dist/public/node/context/local.d.ts@@ -70,6 +70,13 @@ export declare function alwaysLogMetrics(env?: NodeJS.ProcessEnv): boolean;
* @returns True if SHOPIFY_CLI_1P is truthy.
*/
export declare function firstPartyDev(env?: NodeJS.ProcessEnv): boolean;
+/**
+ * Returns true if the CLI can run the "doctor-release" command.
+ *
+ * @param env - The environment variables from the environment of the current process.
+ * @returns True if the CLI can run the "doctor-release" command.
+ */
+export declare function canRunDoctorRelease(env?: NodeJS.ProcessEnv): boolean;
/**
* Return gitpodURL if we are running in gitpod.
* Https://www.gitpod.io/docs/environment-variables#default-environment-variables.
|
|
/snapit |
|
🫰✨ Thanks @EvilGenius13! Your snapshot has been published to npm. Test the snapshot by installing your package globally: npm i -g --@shopify:registry=https://registry.npmjs.org @shopify/cli@0.0.0-snapshot-20260210195945Caution After installing, validate the version by running just |
4 participants
WHY are these changes introduced?
We need an automated way to validate that CLI commands work correctly after changes or new releases. Currently, there's no easy way to:
This doctor command provides the foundation for automated command testing.
WHAT is this pull request doing?
Introduces the
shopify doctorcommand with initial theme command tests:Core Framework: - Adds shopify doctor theme command for testing theme-related commands - Implements test framework with suites, assertions, and context management - Sets up structure for future expansion (app commands)
Initial Tests: - We are testing
theme initandtheme pushcommands. Follow up PR's will add the remaining commands and update existing tests and assertions.Note: - Tests run in order. If a test relies on another command, that command should run first (Ex:
theme initto create a theme and then runningtheme push.How to test your changes?
Pull down the branch
Build the branch
Run
shopify doctor theme -e <your env>in a folder that has ashopify.theme.tomlfile.Post-release steps
Much like the CLI multi-env command work, the doctor command will be built up over many additional PR's.
Measuring impact
How do we know this change was effective? Please choose one:
Checklist