feat!(cli-test): Use child_process spawn arguments properly, fixing JSON encoding on the command line on Windows #2090

filmaj · 2024-11-01T19:47:04Z

Summary

(FYI a majority of changes in this PR are relatively cosmetic - will post comments for areas of particular interest for the kind reviewer)

There is a (technically) breaking change in this PR: the string arguments passed to SlackCLIProcess should be arranged in an array now, and these are passed as the second argument into child_process.spawn. This, in combination with the following changes, also fixes the datastore commands on Windows:

On Windows, shelling out to the CLI process is now wrapped in a cmd.exe /c /s invocation, which helps to manage how node will strip quotes out of arguments on Windows when passed to child_process APIs.
Additional post-processing of JSON string arguments is done (currently only in the datastore commands) to ensure double quotes within JSON arguments are escaped.

TODO:

check that Windows CI tests pass

…pe JSON for datastore commands.

codecov · 2024-11-01T19:49:33Z

Codecov Report

Attention: Patch coverage is 82.96296% with 23 lines in your changes missing coverage. Please review.

Project coverage is 91.65%. Comparing base (1be7a9e) to head (4ec7f60).
Report is 11 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2090      +/-   ##
==========================================
- Coverage   91.83%   91.65%   -0.18%     
==========================================
  Files          38       38              
  Lines       10264    10311      +47     
  Branches      646      647       +1     
==========================================
+ Hits         9426     9451      +25     
- Misses        826      848      +22     
  Partials       12       12

Flag	Coverage Δ
cli-hooks	`95.23% <ø> (ø)`
cli-test	`94.47% <82.96%> (-1.23%)`	⬇️
oauth	`77.39% <ø> (ø)`
socket-mode	`58.22% <ø> (ø)`
web-api	`96.88% <ø> (-0.01%)`	⬇️
webhook	`96.65% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

filmaj · 2024-11-01T20:02:25Z

packages/cli-test/src/cli/cli-process.ts

@@ -49,7 +49,7 @@ export class SlackCLIProcess {
  /**
   * @description The CLI command to invoke
   */
-  public command: string;


One part of the breaking change: commands are now composed of an array of strings, representing the space-delimited shell invocation.

I think this is a learning for all of our back-pockets. When dealing with CLI arguments in code, it's best to represent them as an array rather than a single string. Command-line frameworks like Cobra also always parse the raw command string into an array as well and it's likely because it's a more reliable way to interact with CLI arguments in code.

filmaj · 2024-11-01T20:03:47Z

packages/cli-test/src/cli/commands/datastore.ts

@@ -14,6 +14,10 @@ export interface DatastoreCommandArguments {
  queryExpressionValues: object;
 }

+function escapeJSON(obj: Record<string, unknown>): string {
+  return `"${JSON.stringify(obj).replace(/"/g, '\\"')}"`;


This in combination with using using child_process spawn et al's args array of arguments helped with dealing with JSON encoding of command line arguments in a cross-platform way.

It may be worthwhile leaving a code comment to explain that this is to help support cross-platform JSON encoding

filmaj · 2024-11-01T20:05:01Z

packages/cli-test/src/cli/shell.ts

+    // In windows, we actually spawn a command prompt and tell it to invoke the CLI command.
+    // The combination of windows and node's child_process spawning is complicated: on windows, child_process strips quotes from arguments. This makes passing JSON difficult.
+    // As a workaround, by telling Windows Command Prompt (cmd.exe) to execute a command to completion (/c) and leave spaces intact (/c), combined with feeding arguments as an argument array into child_process, we can get around this mess.
+    const windowsArgs = ['/s', '/c'].concat([command]).concat(args);


This is the root of the Windows compatibility hack

What are /s and /c? (What do they mean / do?)

It's described in the comment - let me know if you have a suggestion on wording or formatting this differently to make it possibly clearer

Oh I see 🤡 execute a command to completion (/c) and leave spaces intact (/s)

I found it really helpful to see it spelled out that this means Windows will execute cmd.exe /c /s "slack cli command here" and would suggest that get added to the comment as well; for a while I thought it was slack \c \s datastore etc., which didn't work when I tried it on my Windows laptop 😅

filmaj · 2024-11-01T20:11:33Z

packages/cli-test/src/cli/shell.spec.ts

      }, /this is bat country/);
    });
+    if (process.platform === 'win32') {


This fork in the test code is to test stuff on Windows vs. non-Windows properly, since we now include Windows-specific code.

This also explains why the code coverage dropped: since the code coverage is generated on a GitHub ubuntu runner, it won't exercise the Windows-specific code path, so the coverage reporter will report a lower coverage score. But, since our CI now runs tests on Windows, this code path is exercised (you can see that in the GitHub Action output for the windows check here)

mwbrooks

Pfff... amazing work @filmaj! That's some incredible detective work 🕵🏻 to uncover the right combination of flags to keep JSON intact on Windows Command Prompt.

I've left a few minor comments and suggestions from the code review. I'll let the others throw a proper ✅ on it but it looks good on my side.

mwbrooks · 2024-11-01T21:43:07Z

packages/cli-test/src/cli/commands/datastore.spec.ts

+      await datastore.datastoreQuery({
+        appPath: '/some/path',
+        datastoreName: 'datastore',
+        queryExpression: 'id = :id',
+        queryExpressionValues: expressObj,
+      });


:bliss: Much more readable

mwbrooks · 2024-11-01T21:44:44Z

packages/cli-test/src/cli/commands/datastore.ts

@@ -14,6 +14,10 @@ export interface DatastoreCommandArguments {
  queryExpressionValues: object;
 }

+function escapeJSON(obj: Record<string, unknown>): string {
+  return `"${JSON.stringify(obj).replace(/"/g, '\\"')}"`;


It may be worthwhile leaving a code comment to explain that this is to help support cross-platform JSON encoding

mwbrooks · 2024-11-01T21:47:24Z

packages/cli-test/src/cli/cli-process.ts

@@ -49,7 +49,7 @@ export class SlackCLIProcess {
  /**
   * @description The CLI command to invoke
   */
-  public command: string;


I think this is a learning for all of our back-pockets. When dealing with CLI arguments in code, it's best to represent them as an array rather than a single string. Command-line frameworks like Cobra also always parse the raw command string into an array as well and it's likely because it's a more reliable way to interact with CLI arguments in code.

packages/cli-test/src/cli/shell.ts

Co-authored-by: Michael Brooks <[email protected]>

vegeris · 2024-11-04T15:31:15Z

Just wondering, how were the Windows e2e tests verified? (Are we able to run them in CircleCI with a given version of the cli-test package?)

Edit: Ah, I see: https://github.com/slackapi/platform-devxp-test/pull/151

filmaj · 2024-11-04T15:35:58Z

@vegeris I have a PR using this branch of cli-test in the relevant repo up here: https://github.com/slackapi/platform-devxp-test/pull/151

The two most recent CircleCI runs for this branch show both the non-windows (e2e-test) and windows (windows-e2e-test) results: https://app.circleci.com/pipelines/github/slackapi/platform-devxp-test?branch=cli-test-pshell

The windows-e2e-test job I triggered manually via the CircleCI UI from the same link: https://app.circleci.com/pipelines/github/slackapi/platform-devxp-test?branch=cli-test-pshell

vegeris

This is great!

I take it we're fine to merge this despite the general restrictions this week as an update to the testing package that won't impact customer-facing behaviour?

filmaj added 2 commits November 1, 2024 15:40

feat!(cli-test): use child_process spawn arguments properly, and esca…

48087e4

…pe JSON for datastore commands.

revert version

48ac92b

filmaj added semver:major enhancement M-T: A feature request for new functionality pkg:cli-test applies to `@slack/cli-test` labels Nov 1, 2024

filmaj requested review from cchensh, vegeris and a team November 1, 2024 19:47

filmaj self-assigned this Nov 1, 2024

add tests for windows vs other shell spawning behaviour

3235029

filmaj added this to the [email protected] milestone Nov 1, 2024

filmaj mentioned this pull request Nov 1, 2024

feat(cli-test): use powershell.exe as shell on windows #2082

Closed

filmaj commented Nov 1, 2024

View reviewed changes

mwbrooks reviewed Nov 1, 2024

View reviewed changes

Update packages/cli-test/src/cli/shell.ts

4f41569

Co-authored-by: Michael Brooks <[email protected]>

vegeris approved these changes Nov 4, 2024

View reviewed changes

filmaj added 2 commits November 4, 2024 10:58

elaborate on explanation for windows cmd.exe spawning workaround

bcbc1fd

document escapeJSON datastore helper function

4ec7f60

filmaj merged commit dcd0183 into main Nov 4, 2024
55 of 57 checks passed

filmaj deleted the windows-spawn-args branch November 4, 2024 17:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!(cli-test): Use child_process spawn arguments properly, fixing JSON encoding on the command line on Windows #2090

feat!(cli-test): Use child_process spawn arguments properly, fixing JSON encoding on the command line on Windows #2090

filmaj commented Nov 1, 2024 •

edited

Loading

codecov bot commented Nov 1, 2024 •

edited

Loading

filmaj Nov 1, 2024

mwbrooks Nov 1, 2024

filmaj Nov 1, 2024

mwbrooks Nov 1, 2024

filmaj Nov 1, 2024

vegeris Nov 4, 2024 •

edited

Loading

filmaj Nov 4, 2024

vegeris Nov 4, 2024

vegeris Nov 4, 2024

filmaj Nov 1, 2024

mwbrooks left a comment

mwbrooks Nov 1, 2024

mwbrooks Nov 1, 2024

mwbrooks Nov 1, 2024

vegeris commented Nov 4, 2024 •

edited

Loading

filmaj commented Nov 4, 2024

vegeris left a comment •

edited

Loading

feat!(cli-test): Use child_process spawn arguments properly, fixing JSON encoding on the command line on Windows #2090

feat!(cli-test): Use child_process spawn arguments properly, fixing JSON encoding on the command line on Windows #2090

Conversation

filmaj commented Nov 1, 2024 • edited Loading

Summary

TODO:

codecov bot commented Nov 1, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vegeris Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mwbrooks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vegeris commented Nov 4, 2024 • edited Loading

filmaj commented Nov 4, 2024

vegeris left a comment • edited Loading

Choose a reason for hiding this comment

filmaj commented Nov 1, 2024 •

edited

Loading

codecov bot commented Nov 1, 2024 •

edited

Loading

vegeris Nov 4, 2024 •

edited

Loading

vegeris commented Nov 4, 2024 •

edited

Loading

vegeris left a comment •

edited

Loading