Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 6 commits into from
Nov 4, 2024
Merged

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
48087e4
feat!(cli-test): use child_process spawn arguments properly, and esca…
filmaj Oct 31, 2024
48ac92b
revert version
filmaj Nov 1, 2024
3235029
add tests for windows vs other shell spawning behaviour
filmaj Nov 1, 2024
4f41569
Update packages/cli-test/src/cli/shell.ts
filmaj Nov 2, 2024
bcbc1fd
elaborate on explanation for windows cmd.exe spawning workaround
filmaj Nov 4, 2024
4ec7f60
document escapeJSON datastore helper function
filmaj Nov 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 2 additions & 8 deletions packages/cli-test/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,16 +4,10 @@
"description": "Node.js bindings for the Slack CLI for use in automated testing",
"author": "Salesforce, Inc.",
"license": "MIT",
"keywords": [
"slack",
"cli",
"test"
],
"keywords": ["slack", "cli", "test"],
"main": "dist/index.js",
"types": "dist/index.d.ts",
"files": [
"dist/**/*"
],
"files": ["dist/**/*"],
"engines": {
"node": ">=18.15.5"
},
Expand Down
138 changes: 93 additions & 45 deletions packages/cli-test/src/cli/cli-process.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ describe('SlackCLIProcess class', () => {
const orig = process.env.SLACK_CLI_PATH;
process.env.SLACK_CLI_PATH = '';
assert.throws(() => {
new SlackCLIProcess('help');
new SlackCLIProcess(['help']);
});
process.env.SLACK_CLI_PATH = orig;
});
Expand All @@ -30,108 +30,156 @@ describe('SlackCLIProcess class', () => {
describe('CLI flag handling', () => {
describe('global options', () => {
it('should map dev option to --slackdev', async () => {
let cmd = new SlackCLIProcess('help', { dev: true });
let cmd = new SlackCLIProcess(['help'], { dev: true });
await cmd.execAsync();
sandbox.assert.calledWithMatch(spawnProcessSpy, '--slackdev');
sandbox.assert.calledWith(spawnProcessSpy, sinon.match.string, sinon.match.array.contains(['--slackdev']));
spawnProcessSpy.resetHistory();
cmd = new SlackCLIProcess('help');
cmd = new SlackCLIProcess(['help']);
await cmd.execAsync();
sandbox.assert.neverCalledWithMatch(spawnProcessSpy, '--slackdev');
sandbox.assert.neverCalledWith(spawnProcessSpy, sinon.match.string, sinon.match.array.contains(['--slackdev']));
spawnProcessSpy.resetHistory();
});
it('should map qa option to QA host', async () => {
let cmd = new SlackCLIProcess('help', { qa: true });
let cmd = new SlackCLIProcess(['help'], { qa: true });
await cmd.execAsync();
sandbox.assert.calledWithMatch(spawnProcessSpy, '--apihost qa.slack.com');
sandbox.assert.calledWith(
spawnProcessSpy,
sinon.match.string,
sinon.match.array.contains(['--apihost', 'qa.slack.com']),
);
spawnProcessSpy.resetHistory();
cmd = new SlackCLIProcess('help');
cmd = new SlackCLIProcess(['help']);
await cmd.execAsync();
sandbox.assert.neverCalledWithMatch(spawnProcessSpy, '--apihost qa.slack.com');
sandbox.assert.neverCalledWith(
spawnProcessSpy,
sinon.match.string,
sinon.match.array.contains(['--apihost', 'qa.slack.com']),
);
spawnProcessSpy.resetHistory();
});
it('should map apihost option to provided host', async () => {
let cmd = new SlackCLIProcess('help', { apihost: 'dev123.slack.com' });
let cmd = new SlackCLIProcess(['help'], { apihost: 'dev123.slack.com' });
await cmd.execAsync();
sandbox.assert.calledWithMatch(spawnProcessSpy, '--apihost dev123.slack.com');
sandbox.assert.calledWith(
spawnProcessSpy,
sinon.match.string,
sinon.match.array.contains(['--apihost', 'dev123.slack.com']),
);
spawnProcessSpy.resetHistory();
cmd = new SlackCLIProcess('help');
cmd = new SlackCLIProcess(['help']);
await cmd.execAsync();
sandbox.assert.neverCalledWithMatch(spawnProcessSpy, '--apihost dev123.slack.com');
sandbox.assert.neverCalledWith(
spawnProcessSpy,
sinon.match.string,
sinon.match.array.contains(['--apihost', 'dev123.slack.com']),
);
spawnProcessSpy.resetHistory();
});
it('should default to passing --skip-update but allow overriding that', async () => {
let cmd = new SlackCLIProcess('help');
let cmd = new SlackCLIProcess(['help']);
await cmd.execAsync();
sandbox.assert.calledWithMatch(spawnProcessSpy, '--skip-update');
sandbox.assert.calledWith(spawnProcessSpy, sinon.match.string, sinon.match.array.contains(['--skip-update']));
spawnProcessSpy.resetHistory();
cmd = new SlackCLIProcess('help', { skipUpdate: false });
cmd = new SlackCLIProcess(['help'], { skipUpdate: false });
await cmd.execAsync();
sandbox.assert.neverCalledWithMatch(spawnProcessSpy, '--skip-update');
sandbox.assert.neverCalledWith(
spawnProcessSpy,
sinon.match.string,
sinon.match.array.contains(['--skip-update']),
);
spawnProcessSpy.resetHistory();
cmd = new SlackCLIProcess('help', { skipUpdate: true });
cmd = new SlackCLIProcess(['help'], { skipUpdate: true });
await cmd.execAsync();
sandbox.assert.calledWithMatch(spawnProcessSpy, '--skip-update');
sandbox.assert.calledWith(spawnProcessSpy, sinon.match.string, sinon.match.array.contains(['--skip-update']));
spawnProcessSpy.resetHistory();
cmd = new SlackCLIProcess('help', {}); // empty global options; so undefined skipUpdate option
cmd = new SlackCLIProcess(['help'], {}); // empty global options; so undefined skipUpdate option
await cmd.execAsync();
sandbox.assert.calledWithMatch(spawnProcessSpy, '--skip-update');
sandbox.assert.calledWith(spawnProcessSpy, sinon.match.string, sinon.match.array.contains(['--skip-update']));
});
it('should default to `--app deployed` but allow overriding that via the `app` parameter', async () => {
let cmd = new SlackCLIProcess('help');
let cmd = new SlackCLIProcess(['help']);
await cmd.execAsync();
sandbox.assert.calledWithMatch(spawnProcessSpy, '--app deployed');
sandbox.assert.calledWith(
spawnProcessSpy,
sinon.match.string,
sinon.match.array.contains(['--app', 'deployed']),
);
spawnProcessSpy.resetHistory();
cmd = new SlackCLIProcess('help', { app: 'local' });
cmd = new SlackCLIProcess(['help'], { app: 'local' });
await cmd.execAsync();
sandbox.assert.calledWithMatch(spawnProcessSpy, '--app local');
sandbox.assert.calledWith(spawnProcessSpy, sinon.match.string, sinon.match.array.contains(['--app', 'local']));
});
it('should default to `--force` but allow overriding that via the `force` parameter', async () => {
let cmd = new SlackCLIProcess('help');
let cmd = new SlackCLIProcess(['help']);
await cmd.execAsync();
sandbox.assert.calledWithMatch(spawnProcessSpy, '--force');
sandbox.assert.calledWith(spawnProcessSpy, sinon.match.string, sinon.match.array.contains(['--force']));
spawnProcessSpy.resetHistory();
cmd = new SlackCLIProcess('help', { force: true });
cmd = new SlackCLIProcess(['help'], { force: true });
await cmd.execAsync();
sandbox.assert.calledWithMatch(spawnProcessSpy, '--force');
sandbox.assert.calledWithMatch(spawnProcessSpy, sinon.match.string, sinon.match.array.contains(['--force']));
spawnProcessSpy.resetHistory();
cmd = new SlackCLIProcess('help', { force: false });
cmd = new SlackCLIProcess(['help'], { force: false });
await cmd.execAsync();
sandbox.assert.neverCalledWithMatch(spawnProcessSpy, '--force');
sandbox.assert.neverCalledWith(spawnProcessSpy, sinon.match.string, sinon.match.array.contains(['--force']));
});
it('should map token option to `--token`', async () => {
let cmd = new SlackCLIProcess('help', { token: 'xoxb-1234' });
let cmd = new SlackCLIProcess(['help'], { token: 'xoxb-1234' });
await cmd.execAsync();
sandbox.assert.calledWithMatch(spawnProcessSpy, '--token xoxb-1234');
sandbox.assert.calledWith(
spawnProcessSpy,
sinon.match.string,
sinon.match.array.contains(['--token', 'xoxb-1234']),
);
spawnProcessSpy.resetHistory();
cmd = new SlackCLIProcess('help');
cmd = new SlackCLIProcess(['help']);
await cmd.execAsync();
sandbox.assert.neverCalledWithMatch(spawnProcessSpy, '--token xoxb-1234');
sandbox.assert.neverCalledWith(
spawnProcessSpy,
sinon.match.string,
sinon.match.array.contains(['--token', 'xoxb-1234']),
);
spawnProcessSpy.resetHistory();
});
});
describe('command options', () => {
it('should pass command-level key/value options to command in the form `--<key> value`', async () => {
const cmd = new SlackCLIProcess('help', {}, { '--awesome': 'yes' });
const cmd = new SlackCLIProcess(['help'], {}, { '--awesome': 'yes' });
await cmd.execAsync();
sandbox.assert.calledWithMatch(spawnProcessSpy, '--awesome yes');
sandbox.assert.calledWith(
spawnProcessSpy,
sinon.match.string,
sinon.match.array.contains(['--awesome', 'yes']),
);
});
it('should only pass command-level key option if value is true in the form `--key`', async () => {
const cmd = new SlackCLIProcess('help', {}, { '--no-prompt': true });
const cmd = new SlackCLIProcess(['help'], {}, { '--no-prompt': true });
await cmd.execAsync();
sandbox.assert.calledWithMatch(spawnProcessSpy, '--no-prompt');
sandbox.assert.calledWith(spawnProcessSpy, sinon.match.string, sinon.match.array.contains(['--no-prompt']));
});
it('should not pass command-level key option if value is falsy', async () => {
let cmd = new SlackCLIProcess('help', {}, { '--no-prompt': false });
let cmd = new SlackCLIProcess(['help'], {}, { '--no-prompt': false });
await cmd.execAsync();
sandbox.assert.neverCalledWithMatch(spawnProcessSpy, '--no-prompt');
sandbox.assert.neverCalledWith(
spawnProcessSpy,
sinon.match.string,
sinon.match.array.contains(['--no-prompt']),
);
spawnProcessSpy.resetHistory();
cmd = new SlackCLIProcess('help', {}, { '--no-prompt': '' });
cmd = new SlackCLIProcess(['help'], {}, { '--no-prompt': '' });
await cmd.execAsync();
sandbox.assert.neverCalledWithMatch(spawnProcessSpy, '--no-prompt');
sandbox.assert.neverCalledWith(
spawnProcessSpy,
sinon.match.string,
sinon.match.array.contains(['--no-prompt']),
);
spawnProcessSpy.resetHistory();
cmd = new SlackCLIProcess('help', {}, { '--no-prompt': undefined });
cmd = new SlackCLIProcess(['help'], {}, { '--no-prompt': undefined });
await cmd.execAsync();
sandbox.assert.neverCalledWithMatch(spawnProcessSpy, '--no-prompt');
sandbox.assert.neverCalledWith(
spawnProcessSpy,
sinon.match.string,
sinon.match.array.contains(['--no-prompt']),
);
});
});
});
Expand Down
47 changes: 27 additions & 20 deletions packages/cli-test/src/cli/cli-process.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@
/**
* @description The CLI command to invoke
*/
public command: string;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

public command: string[];

/**
* @description The global CLI options to pass to the command
Expand All @@ -61,7 +61,11 @@
*/
public commandOptions: SlackCLICommandOptions | undefined;

public constructor(command: string, globalOptions?: SlackCLIGlobalOptions, commandOptions?: SlackCLICommandOptions) {
public constructor(
command: string[],
globalOptions?: SlackCLIGlobalOptions,
commandOptions?: SlackCLICommandOptions,
) {
if (!process.env.SLACK_CLI_PATH) {
throw new Error('`SLACK_CLI_PATH` environment variable not found! Aborting!');
}
Expand All @@ -75,7 +79,8 @@
*/
public async execAsync(shellOpts?: Partial<SpawnOptionsWithoutStdio>): Promise<ShellProcess> {
const cmd = this.assembleShellInvocation();
const proc = shell.spawnProcess(cmd, shellOpts);
// biome-ignore lint/style/noNonNullAssertion: the constructor checks for the truthiness of this environment variable
const proc = shell.spawnProcess(process.env.SLACK_CLI_PATH!, cmd, shellOpts);
await shell.checkIfFinished(proc);
return proc;
}
Expand All @@ -88,7 +93,8 @@
shellOpts?: Partial<SpawnOptionsWithoutStdio>,
): Promise<ShellProcess> {
const cmd = this.assembleShellInvocation();
const proc = shell.spawnProcess(cmd, shellOpts);
// biome-ignore lint/style/noNonNullAssertion: the constructor checks for the truthiness of this environment variable
const proc = shell.spawnProcess(process.env.SLACK_CLI_PATH!, cmd, shellOpts);
await shell.waitForOutput(output, proc, {
timeout: shellOpts?.timeout,
});
Expand All @@ -100,53 +106,54 @@
*/
public execSync(shellOpts?: Partial<SpawnOptionsWithoutStdio>): string {
const cmd = this.assembleShellInvocation();
return shell.runCommandSync(cmd, shellOpts);
// biome-ignore lint/style/noNonNullAssertion: the constructor checks for the truthiness of this environment variable
return shell.runCommandSync(process.env.SLACK_CLI_PATH!, cmd, shellOpts);

Check warning on line 110 in packages/cli-test/src/cli/cli-process.ts

View check run for this annotation

Codecov / codecov/patch

packages/cli-test/src/cli/cli-process.ts#L109-L110 

Added lines #L109 - L110 were not covered by tests
}

private assembleShellInvocation(): string {
let cmd = `${process.env.SLACK_CLI_PATH}`;
private assembleShellInvocation(): string[] {
let cmd: string[] = [];
if (this.globalOptions) {
const opts = this.globalOptions;
// Determine API host target
if (opts.apihost) {
cmd += ` --apihost ${opts.apihost}`;
cmd = cmd.concat(['--apihost', opts.apihost]);
} else if (opts.qa) {
cmd += ' --apihost qa.slack.com';
cmd = cmd.concat(['--apihost', 'qa.slack.com']);
} else if (opts.dev) {
cmd += ' --slackdev';
cmd = cmd.concat(['--slackdev']);
}
// Always skip update unless explicitly set to something falsy
if (opts.skipUpdate || opts.skipUpdate === undefined) {
cmd += ' --skip-update';
cmd = cmd.concat(['--skip-update']);
}
// Target team
if (opts.team) {
cmd += ` --team ${opts.team}`;
cmd = cmd.concat(['--team', opts.team]);
}
// App instance; defaults to `deployed`
if (opts.app) {
cmd += ` --app ${opts.app}`;
cmd = cmd.concat(['--app', opts.app]);
} else {
cmd += ' --app deployed';
cmd = cmd.concat(['--app', 'deployed']);
}
// Ignore warnings via --force; defaults to true
if (opts.force || typeof opts.force === 'undefined') {
cmd += ' --force';
cmd = cmd.concat(['--force']);
}
// Specifying custom token
if (opts.token) {
cmd += ` --token ${opts.token}`;
cmd = cmd.concat(['--token', opts.token]);
}
} else {
cmd += ' --skip-update --force --app deployed';
cmd = cmd.concat(['--skip-update', '--force', '--app', 'deployed']);
}
cmd += ` ${this.command}`;
cmd = cmd.concat(this.command);
if (this.commandOptions) {
for (const [key, value] of Object.entries(this.commandOptions)) {
if (key && value) {
cmd += ` ${key}`;
cmd.push(key);
if (value !== true) {
cmd += ` ${value}`;
cmd.push(String(value));
}
}
}
Expand Down
11 changes: 5 additions & 6 deletions packages/cli-test/src/cli/commands/app.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,28 +25,27 @@ describe('app commands', () => {
describe('delete method', () => {
it('should invoke `app delete` and default force=true', async () => {
await app.delete({ appPath: '/some/path' });
sandbox.assert.calledWith(spawnSpy, sinon.match('--force'));
sandbox.assert.calledWith(spawnSpy, sinon.match('app delete'));
sandbox.assert.calledWith(spawnSpy, sinon.match.string, sinon.match.array.contains(['--force', 'app', 'delete']));
});
it('should invoke with `--force` if force=true', async () => {
await app.delete({ appPath: '/some/path', force: true });
sandbox.assert.calledWith(spawnSpy, sinon.match('--force'));
sandbox.assert.calledWith(spawnSpy, sinon.match.string, sinon.match.array.contains(['--force']));
});
it('should invoke without `--force` if force=false', async () => {
await app.delete({ appPath: '/some/path', force: false });
sandbox.assert.neverCalledWith(spawnSpy, sinon.match('--force'));
sandbox.assert.neverCalledWith(spawnSpy, sinon.match.string, sinon.match.array.contains(['--force']));
});
});
describe('install method', () => {
it('should invoke a CLI process with `app install`', async () => {
await app.install({ appPath: '/some/path' });
sandbox.assert.calledWith(spawnSpy, sinon.match('app install'));
sandbox.assert.calledWith(spawnSpy, sinon.match.string, sinon.match.array.contains(['app', 'install']));
});
});
describe('list method', () => {
it('should invoke a CLI process with `app list`', async () => {
await app.list({ appPath: '/some/path' });
sandbox.assert.calledWith(spawnSpy, sinon.match('app list'));
sandbox.assert.calledWith(spawnSpy, sinon.match.string, sinon.match.array.contains(['app', 'list']));
});
});
});
Loading