Conversation
Coverage report
Show new covered files 🐣
Show files with reduced coverage 🔻
Test suite run success3724 tests passing in 1443 suites. Report generated by 🧪jest coverage report action from 18a9b6d |
EvilGenius13
force-pushed
the
jf-audit-proto
branch
5 times, most recently
from
9ecb998 to
440b7c9
Compare
EvilGenius13
force-pushed
the
jf-audit-proto
branch
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/[email protected]Caution 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.
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 declarationsWe found no new type declarations in this PR Existing type declarationspackages/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.
*
|
|
/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/[email protected]Caution 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 |
3 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