Skip to content

Commit dcd0183

Browse files
Fil Majmwbrooks
Fil Maj
and
mwbrooks
authored
feat!(cli-test): Use child_process spawn arguments properly, fixing JSON encoding on the command line on Windows (#2090)
Co-authored-by: Michael Brooks <[email protected]>
1 parent 806d2fc commit dcd0183

28 files changed

+602
-311
lines changed

‎packages/cli-test/package.json‎

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -4,16 +4,10 @@
44
"description": "Node.js bindings for the Slack CLI for use in automated testing",
55
"author": "Salesforce, Inc.",
66
"license": "MIT",
7-
"keywords": [
8-
"slack",
9-
"cli",
10-
"test"
11-
],
7+
"keywords": ["slack", "cli", "test"],
128
"main": "dist/index.js",
139
"types": "dist/index.d.ts",
14-
"files": [
15-
"dist/**/*"
16-
],
10+
"files": ["dist/**/*"],
1711
"engines": {
1812
"node": ">=18.15.5"
1913
},

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

Lines changed: 93 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ describe('SlackCLIProcess class', () => {
2121
const orig = process.env.SLACK_CLI_PATH;
2222
process.env.SLACK_CLI_PATH = '';
2323
assert.throws(() => {
24-
new SlackCLIProcess('help');
24+
new SlackCLIProcess(['help']);
2525
});
2626
process.env.SLACK_CLI_PATH = orig;
2727
});
@@ -30,108 +30,156 @@ describe('SlackCLIProcess class', () => {
3030
describe('CLI flag handling', () => {
3131
describe('global options', () => {
3232
it('should map dev option to --slackdev', async () => {
33-
let cmd = new SlackCLIProcess('help', { dev: true });
33+
let cmd = new SlackCLIProcess(['help'], { dev: true });
3434
await cmd.execAsync();
35-
sandbox.assert.calledWithMatch(spawnProcessSpy, '--slackdev');
35+
sandbox.assert.calledWith(spawnProcessSpy, sinon.match.string, sinon.match.array.contains(['--slackdev']));
3636
spawnProcessSpy.resetHistory();
37-
cmd = new SlackCLIProcess('help');
37+
cmd = new SlackCLIProcess(['help']);
3838
await cmd.execAsync();
39-
sandbox.assert.neverCalledWithMatch(spawnProcessSpy, '--slackdev');
39+
sandbox.assert.neverCalledWith(spawnProcessSpy, sinon.match.string, sinon.match.array.contains(['--slackdev']));
4040
spawnProcessSpy.resetHistory();
4141
});
4242
it('should map qa option to QA host', async () => {
43-
let cmd = new SlackCLIProcess('help', { qa: true });
43+
let cmd = new SlackCLIProcess(['help'], { qa: true });
4444
await cmd.execAsync();
45-
sandbox.assert.calledWithMatch(spawnProcessSpy, '--apihost qa.slack.com');
45+
sandbox.assert.calledWith(
46+
spawnProcessSpy,
47+
sinon.match.string,
48+
sinon.match.array.contains(['--apihost', 'qa.slack.com']),
49+
);
4650
spawnProcessSpy.resetHistory();
47-
cmd = new SlackCLIProcess('help');
51+
cmd = new SlackCLIProcess(['help']);
4852
await cmd.execAsync();
49-
sandbox.assert.neverCalledWithMatch(spawnProcessSpy, '--apihost qa.slack.com');
53+
sandbox.assert.neverCalledWith(
54+
spawnProcessSpy,
55+
sinon.match.string,
56+
sinon.match.array.contains(['--apihost', 'qa.slack.com']),
57+
);
5058
spawnProcessSpy.resetHistory();
5159
});
5260
it('should map apihost option to provided host', async () => {
53-
let cmd = new SlackCLIProcess('help', { apihost: 'dev123.slack.com' });
61+
let cmd = new SlackCLIProcess(['help'], { apihost: 'dev123.slack.com' });
5462
await cmd.execAsync();
55-
sandbox.assert.calledWithMatch(spawnProcessSpy, '--apihost dev123.slack.com');
63+
sandbox.assert.calledWith(
64+
spawnProcessSpy,
65+
sinon.match.string,
66+
sinon.match.array.contains(['--apihost', 'dev123.slack.com']),
67+
);
5668
spawnProcessSpy.resetHistory();
57-
cmd = new SlackCLIProcess('help');
69+
cmd = new SlackCLIProcess(['help']);
5870
await cmd.execAsync();
59-
sandbox.assert.neverCalledWithMatch(spawnProcessSpy, '--apihost dev123.slack.com');
71+
sandbox.assert.neverCalledWith(
72+
spawnProcessSpy,
73+
sinon.match.string,
74+
sinon.match.array.contains(['--apihost', 'dev123.slack.com']),
75+
);
6076
spawnProcessSpy.resetHistory();
6177
});
6278
it('should default to passing --skip-update but allow overriding that', async () => {
63-
let cmd = new SlackCLIProcess('help');
79+
let cmd = new SlackCLIProcess(['help']);
6480
await cmd.execAsync();
65-
sandbox.assert.calledWithMatch(spawnProcessSpy, '--skip-update');
81+
sandbox.assert.calledWith(spawnProcessSpy, sinon.match.string, sinon.match.array.contains(['--skip-update']));
6682
spawnProcessSpy.resetHistory();
67-
cmd = new SlackCLIProcess('help', { skipUpdate: false });
83+
cmd = new SlackCLIProcess(['help'], { skipUpdate: false });
6884
await cmd.execAsync();
69-
sandbox.assert.neverCalledWithMatch(spawnProcessSpy, '--skip-update');
85+
sandbox.assert.neverCalledWith(
86+
spawnProcessSpy,
87+
sinon.match.string,
88+
sinon.match.array.contains(['--skip-update']),
89+
);
7090
spawnProcessSpy.resetHistory();
71-
cmd = new SlackCLIProcess('help', { skipUpdate: true });
91+
cmd = new SlackCLIProcess(['help'], { skipUpdate: true });
7292
await cmd.execAsync();
73-
sandbox.assert.calledWithMatch(spawnProcessSpy, '--skip-update');
93+
sandbox.assert.calledWith(spawnProcessSpy, sinon.match.string, sinon.match.array.contains(['--skip-update']));
7494
spawnProcessSpy.resetHistory();
75-
cmd = new SlackCLIProcess('help', {}); // empty global options; so undefined skipUpdate option
95+
cmd = new SlackCLIProcess(['help'], {}); // empty global options; so undefined skipUpdate option
7696
await cmd.execAsync();
77-
sandbox.assert.calledWithMatch(spawnProcessSpy, '--skip-update');
97+
sandbox.assert.calledWith(spawnProcessSpy, sinon.match.string, sinon.match.array.contains(['--skip-update']));
7898
});
7999
it('should default to `--app deployed` but allow overriding that via the `app` parameter', async () => {
80-
let cmd = new SlackCLIProcess('help');
100+
let cmd = new SlackCLIProcess(['help']);
81101
await cmd.execAsync();
82-
sandbox.assert.calledWithMatch(spawnProcessSpy, '--app deployed');
102+
sandbox.assert.calledWith(
103+
spawnProcessSpy,
104+
sinon.match.string,
105+
sinon.match.array.contains(['--app', 'deployed']),
106+
);
83107
spawnProcessSpy.resetHistory();
84-
cmd = new SlackCLIProcess('help', { app: 'local' });
108+
cmd = new SlackCLIProcess(['help'], { app: 'local' });
85109
await cmd.execAsync();
86-
sandbox.assert.calledWithMatch(spawnProcessSpy, '--app local');
110+
sandbox.assert.calledWith(spawnProcessSpy, sinon.match.string, sinon.match.array.contains(['--app', 'local']));
87111
});
88112
it('should default to `--force` but allow overriding that via the `force` parameter', async () => {
89-
let cmd = new SlackCLIProcess('help');
113+
let cmd = new SlackCLIProcess(['help']);
90114
await cmd.execAsync();
91-
sandbox.assert.calledWithMatch(spawnProcessSpy, '--force');
115+
sandbox.assert.calledWith(spawnProcessSpy, sinon.match.string, sinon.match.array.contains(['--force']));
92116
spawnProcessSpy.resetHistory();
93-
cmd = new SlackCLIProcess('help', { force: true });
117+
cmd = new SlackCLIProcess(['help'], { force: true });
94118
await cmd.execAsync();
95-
sandbox.assert.calledWithMatch(spawnProcessSpy, '--force');
119+
sandbox.assert.calledWithMatch(spawnProcessSpy, sinon.match.string, sinon.match.array.contains(['--force']));
96120
spawnProcessSpy.resetHistory();
97-
cmd = new SlackCLIProcess('help', { force: false });
121+
cmd = new SlackCLIProcess(['help'], { force: false });
98122
await cmd.execAsync();
99-
sandbox.assert.neverCalledWithMatch(spawnProcessSpy, '--force');
123+
sandbox.assert.neverCalledWith(spawnProcessSpy, sinon.match.string, sinon.match.array.contains(['--force']));
100124
});
101125
it('should map token option to `--token`', async () => {
102-
let cmd = new SlackCLIProcess('help', { token: 'xoxb-1234' });
126+
let cmd = new SlackCLIProcess(['help'], { token: 'xoxb-1234' });
103127
await cmd.execAsync();
104-
sandbox.assert.calledWithMatch(spawnProcessSpy, '--token xoxb-1234');
128+
sandbox.assert.calledWith(
129+
spawnProcessSpy,
130+
sinon.match.string,
131+
sinon.match.array.contains(['--token', 'xoxb-1234']),
132+
);
105133
spawnProcessSpy.resetHistory();
106-
cmd = new SlackCLIProcess('help');
134+
cmd = new SlackCLIProcess(['help']);
107135
await cmd.execAsync();
108-
sandbox.assert.neverCalledWithMatch(spawnProcessSpy, '--token xoxb-1234');
136+
sandbox.assert.neverCalledWith(
137+
spawnProcessSpy,
138+
sinon.match.string,
139+
sinon.match.array.contains(['--token', 'xoxb-1234']),
140+
);
109141
spawnProcessSpy.resetHistory();
110142
});
111143
});
112144
describe('command options', () => {
113145
it('should pass command-level key/value options to command in the form `--<key> value`', async () => {
114-
const cmd = new SlackCLIProcess('help', {}, { '--awesome': 'yes' });
146+
const cmd = new SlackCLIProcess(['help'], {}, { '--awesome': 'yes' });
115147
await cmd.execAsync();
116-
sandbox.assert.calledWithMatch(spawnProcessSpy, '--awesome yes');
148+
sandbox.assert.calledWith(
149+
spawnProcessSpy,
150+
sinon.match.string,
151+
sinon.match.array.contains(['--awesome', 'yes']),
152+
);
117153
});
118154
it('should only pass command-level key option if value is true in the form `--key`', async () => {
119-
const cmd = new SlackCLIProcess('help', {}, { '--no-prompt': true });
155+
const cmd = new SlackCLIProcess(['help'], {}, { '--no-prompt': true });
120156
await cmd.execAsync();
121-
sandbox.assert.calledWithMatch(spawnProcessSpy, '--no-prompt');
157+
sandbox.assert.calledWith(spawnProcessSpy, sinon.match.string, sinon.match.array.contains(['--no-prompt']));
122158
});
123159
it('should not pass command-level key option if value is falsy', async () => {
124-
let cmd = new SlackCLIProcess('help', {}, { '--no-prompt': false });
160+
let cmd = new SlackCLIProcess(['help'], {}, { '--no-prompt': false });
125161
await cmd.execAsync();
126-
sandbox.assert.neverCalledWithMatch(spawnProcessSpy, '--no-prompt');
162+
sandbox.assert.neverCalledWith(
163+
spawnProcessSpy,
164+
sinon.match.string,
165+
sinon.match.array.contains(['--no-prompt']),
166+
);
127167
spawnProcessSpy.resetHistory();
128-
cmd = new SlackCLIProcess('help', {}, { '--no-prompt': '' });
168+
cmd = new SlackCLIProcess(['help'], {}, { '--no-prompt': '' });
129169
await cmd.execAsync();
130-
sandbox.assert.neverCalledWithMatch(spawnProcessSpy, '--no-prompt');
170+
sandbox.assert.neverCalledWith(
171+
spawnProcessSpy,
172+
sinon.match.string,
173+
sinon.match.array.contains(['--no-prompt']),
174+
);
131175
spawnProcessSpy.resetHistory();
132-
cmd = new SlackCLIProcess('help', {}, { '--no-prompt': undefined });
176+
cmd = new SlackCLIProcess(['help'], {}, { '--no-prompt': undefined });
133177
await cmd.execAsync();
134-
sandbox.assert.neverCalledWithMatch(spawnProcessSpy, '--no-prompt');
178+
sandbox.assert.neverCalledWith(
179+
spawnProcessSpy,
180+
sinon.match.string,
181+
sinon.match.array.contains(['--no-prompt']),
182+
);
135183
});
136184
});
137185
});

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

Lines changed: 27 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,7 @@ export class SlackCLIProcess {
4949
/**
5050
* @description The CLI command to invoke
5151
*/
52-
public command: string;
52+
public command: string[];
5353

5454
/**
5555
* @description The global CLI options to pass to the command
@@ -61,7 +61,11 @@ export class SlackCLIProcess {
6161
*/
6262
public commandOptions: SlackCLICommandOptions | undefined;
6363

64-
public constructor(command: string, globalOptions?: SlackCLIGlobalOptions, commandOptions?: SlackCLICommandOptions) {
64+
public constructor(
65+
command: string[],
66+
globalOptions?: SlackCLIGlobalOptions,
67+
commandOptions?: SlackCLICommandOptions,
68+
) {
6569
if (!process.env.SLACK_CLI_PATH) {
6670
throw new Error('`SLACK_CLI_PATH` environment variable not found! Aborting!');
6771
}
@@ -75,7 +79,8 @@ export class SlackCLIProcess {
7579
*/
7680
public async execAsync(shellOpts?: Partial<SpawnOptionsWithoutStdio>): Promise<ShellProcess> {
7781
const cmd = this.assembleShellInvocation();
78-
const proc = shell.spawnProcess(cmd, shellOpts);
82+
// biome-ignore lint/style/noNonNullAssertion: the constructor checks for the truthiness of this environment variable
83+
const proc = shell.spawnProcess(process.env.SLACK_CLI_PATH!, cmd, shellOpts);
7984
await shell.checkIfFinished(proc);
8085
return proc;
8186
}
@@ -88,7 +93,8 @@ export class SlackCLIProcess {
8893
shellOpts?: Partial<SpawnOptionsWithoutStdio>,
8994
): Promise<ShellProcess> {
9095
const cmd = this.assembleShellInvocation();
91-
const proc = shell.spawnProcess(cmd, shellOpts);
96+
// biome-ignore lint/style/noNonNullAssertion: the constructor checks for the truthiness of this environment variable
97+
const proc = shell.spawnProcess(process.env.SLACK_CLI_PATH!, cmd, shellOpts);
9298
await shell.waitForOutput(output, proc, {
9399
timeout: shellOpts?.timeout,
94100
});
@@ -100,53 +106,54 @@ export class SlackCLIProcess {
100106
*/
101107
public execSync(shellOpts?: Partial<SpawnOptionsWithoutStdio>): string {
102108
const cmd = this.assembleShellInvocation();
103-
return shell.runCommandSync(cmd, shellOpts);
109+
// biome-ignore lint/style/noNonNullAssertion: the constructor checks for the truthiness of this environment variable
110+
return shell.runCommandSync(process.env.SLACK_CLI_PATH!, cmd, shellOpts);
104111
}
105112

106-
private assembleShellInvocation(): string {
107-
let cmd = `${process.env.SLACK_CLI_PATH}`;
113+
private assembleShellInvocation(): string[] {
114+
let cmd: string[] = [];
108115
if (this.globalOptions) {
109116
const opts = this.globalOptions;
110117
// Determine API host target
111118
if (opts.apihost) {
112-
cmd += ` --apihost ${opts.apihost}`;
119+
cmd = cmd.concat(['--apihost', opts.apihost]);
113120
} else if (opts.qa) {
114-
cmd += ' --apihost qa.slack.com';
121+
cmd = cmd.concat(['--apihost', 'qa.slack.com']);
115122
} else if (opts.dev) {
116-
cmd += ' --slackdev';
123+
cmd = cmd.concat(['--slackdev']);
117124
}
118125
// Always skip update unless explicitly set to something falsy
119126
if (opts.skipUpdate || opts.skipUpdate === undefined) {
120-
cmd += ' --skip-update';
127+
cmd = cmd.concat(['--skip-update']);
121128
}
122129
// Target team
123130
if (opts.team) {
124-
cmd += ` --team ${opts.team}`;
131+
cmd = cmd.concat(['--team', opts.team]);
125132
}
126133
// App instance; defaults to `deployed`
127134
if (opts.app) {
128-
cmd += ` --app ${opts.app}`;
135+
cmd = cmd.concat(['--app', opts.app]);
129136
} else {
130-
cmd += ' --app deployed';
137+
cmd = cmd.concat(['--app', 'deployed']);
131138
}
132139
// Ignore warnings via --force; defaults to true
133140
if (opts.force || typeof opts.force === 'undefined') {
134-
cmd += ' --force';
141+
cmd = cmd.concat(['--force']);
135142
}
136143
// Specifying custom token
137144
if (opts.token) {
138-
cmd += ` --token ${opts.token}`;
145+
cmd = cmd.concat(['--token', opts.token]);
139146
}
140147
} else {
141-
cmd += ' --skip-update --force --app deployed';
148+
cmd = cmd.concat(['--skip-update', '--force', '--app', 'deployed']);
142149
}
143-
cmd += ` ${this.command}`;
150+
cmd = cmd.concat(this.command);
144151
if (this.commandOptions) {
145152
for (const [key, value] of Object.entries(this.commandOptions)) {
146153
if (key && value) {
147-
cmd += ` ${key}`;
154+
cmd.push(key);
148155
if (value !== true) {
149-
cmd += ` ${value}`;
156+
cmd.push(String(value));
150157
}
151158
}
152159
}

‎packages/cli-test/src/cli/commands/app.spec.ts‎

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -25,28 +25,27 @@ describe('app commands', () => {
2525
describe('delete method', () => {
2626
it('should invoke `app delete` and default force=true', async () => {
2727
await app.delete({ appPath: '/some/path' });
28-
sandbox.assert.calledWith(spawnSpy, sinon.match('--force'));
29-
sandbox.assert.calledWith(spawnSpy, sinon.match('app delete'));
28+
sandbox.assert.calledWith(spawnSpy, sinon.match.string, sinon.match.array.contains(['--force', 'app', 'delete']));
3029
});
3130
it('should invoke with `--force` if force=true', async () => {
3231
await app.delete({ appPath: '/some/path', force: true });
33-
sandbox.assert.calledWith(spawnSpy, sinon.match('--force'));
32+
sandbox.assert.calledWith(spawnSpy, sinon.match.string, sinon.match.array.contains(['--force']));
3433
});
3534
it('should invoke without `--force` if force=false', async () => {
3635
await app.delete({ appPath: '/some/path', force: false });
37-
sandbox.assert.neverCalledWith(spawnSpy, sinon.match('--force'));
36+
sandbox.assert.neverCalledWith(spawnSpy, sinon.match.string, sinon.match.array.contains(['--force']));
3837
});
3938
});
4039
describe('install method', () => {
4140
it('should invoke a CLI process with `app install`', async () => {
4241
await app.install({ appPath: '/some/path' });
43-
sandbox.assert.calledWith(spawnSpy, sinon.match('app install'));
42+
sandbox.assert.calledWith(spawnSpy, sinon.match.string, sinon.match.array.contains(['app', 'install']));
4443
});
4544
});
4645
describe('list method', () => {
4746
it('should invoke a CLI process with `app list`', async () => {
4847
await app.list({ appPath: '/some/path' });
49-
sandbox.assert.calledWith(spawnSpy, sinon.match('app list'));
48+
sandbox.assert.calledWith(spawnSpy, sinon.match.string, sinon.match.array.contains(['app', 'list']));
5049
});
5150
});
5251
});

0 commit comments

Comments
 (0)