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.

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

Fixes providing value for required option. Closes #6518 #6520

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
51 changes: 48 additions & 3 deletions src/cli/cli.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -280,6 +280,25 @@ class MockCommandWithSchemaAndRequiredOptions extends AnonymousCommand {
}
}

class MockCommandWithSchemaAndBoolRequiredOption extends AnonymousCommand {
public get name(): string {
return 'cli mock schema bool required';
}
public get description(): string {
return 'Mock command with schema and required options';
}
public get schema(): z.ZodTypeAny {
return globalOptionsZod
.extend({
url: z.string(),
bool: z.boolean()
})
.strict();
}
public async commandAction(): Promise<void> {
}
}

describe('cli', () => {
let rootFolder: string;
let cliLogStub: sinon.SinonStub;
Expand All @@ -295,6 +314,7 @@ describe('cli', () => {
let mockCommandWithValidation: Command;
let mockCommandWithSchema: Command;
let mockCommandWithSchemaAndRequiredOptions: Command;
let mockCommandWithSchemaAndBoolRequiredOption: Command;
let log: string[] = [];
let mockCommandWithBooleanRewrite: Command;

Expand All @@ -318,6 +338,7 @@ describe('cli', () => {
mockCommandWithValidation = new MockCommandWithValidation();
mockCommandWithSchema = new MockCommandWithSchema();
mockCommandWithSchemaAndRequiredOptions = new MockCommandWithSchemaAndRequiredOptions();
mockCommandWithSchemaAndBoolRequiredOption = new MockCommandWithSchemaAndBoolRequiredOption();
mockCommandWithOptionSets = new MockCommandWithOptionSets();
mockCommandActionSpy = sinon.spy(mockCommand, 'action');

Expand All @@ -339,6 +360,7 @@ describe('cli', () => {
cli.getCommandInfo(mockCommandWithValidation, 'cli-validation-mock.js', 'help.mdx'),
cli.getCommandInfo(mockCommandWithSchema, 'cli-schema-mock.js', 'help.mdx'),
cli.getCommandInfo(mockCommandWithSchemaAndRequiredOptions, 'cli-schema-mock.js', 'help.mdx'),
cli.getCommandInfo(mockCommandWithSchemaAndBoolRequiredOption, 'cli-schema-mock.js', 'help.mdx'),
cli.getCommandInfo(cliCompletionUpdateCommand, 'cli/commands/completion/completion-clink-update.js', 'cli/completion/completion-clink-update.mdx'),
cli.getCommandInfo(mockCommandWithBooleanRewrite, 'cli-boolean-rewrite-mock.js', 'help.mdx')
];
Expand Down Expand Up @@ -1093,6 +1115,29 @@ describe('cli', () => {
});
});

it(`throws error if coercing value fails when validating schema`, (done) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let use async/await for new tests.

cli.commandToExecute = cli.commands.find(c => c.name === 'cli mock schema bool required');
sinon.stub(cli, 'getSettingWithDefaultValue').callsFake((settingName, defaultValue) => {
if (settingName === settingsNames.prompt) {
return true;
}
return defaultValue;
});
sinon.stub(cli, 'promptForValue').resolves('nonbool');
const executeCommandSpy = sinon.spy(cli, 'executeCommand');
cli
.execute(['cli', 'mock', 'schema', 'bool', 'required', '--url', 'https://contoso.sharepoint.com'])
.then(_ => done('Promise fulfilled while error expected'), _ => {
try {
assert(executeCommandSpy.notCalled);
done();
}
catch (e) {
done(e);
}
});
});

it(`throws an error when command's schema-based validation failed`, (done) => {
cli.commandToExecute = cli.commands.find(c => c.name === 'cli mock schema');
const mockCommandWithSchemaActionSpy: sinon.SinonSpy = sinon.spy(mockCommandWithSchema, 'action');
Expand Down Expand Up @@ -1689,7 +1734,7 @@ describe('cli', () => {
await cli.loadCommandFromArgs(['spo', 'site', 'list']);
cli.printAvailableCommands();

assert(cliLogStub.calledWith(' cli * 10 commands'));
assert(cliLogStub.calledWith(' cli * 11 commands'));
});

it(`prints commands from the specified group`, async () => {
Expand All @@ -1702,7 +1747,7 @@ describe('cli', () => {
};
cli.printAvailableCommands();

assert(cliLogStub.calledWith(' cli mock * 7 commands'));
assert(cliLogStub.calledWith(' cli mock * 8 commands'));
});

it(`prints commands from the root group when the specified string doesn't match any group`, async () => {
Expand All @@ -1715,7 +1760,7 @@ describe('cli', () => {
};
cli.printAvailableCommands();

assert(cliLogStub.calledWith(' cli * 10 commands'));
assert(cliLogStub.calledWith(' cli * 11 commands'));
});

it(`runs properly when context file not found`, async () => {
Expand Down
12 changes: 10 additions & 2 deletions src/cli/cli.ts
Original file line number Diff line number Diff line change
Expand Up @@ -194,9 +194,17 @@ async function execute(rawArgs: string[]): Promise<void> {
await cli.error('🌶️ Provide values for the following parameters:');

for (const error of result.error.errors) {
const optionInfo = cli.commandToExecute!.options.find(o => o.name === error.path.join('.'));
const optionName = error.path.join('.');
const optionInfo = cli.commandToExecute.options.find(o => o.name === optionName);
const answer = await cli.promptForValue(optionInfo!);
cli.optionsFromArgs!.options[error.path.join('.')] = answer;
// coerce the answer to the correct type
try {
const parsed = getCommandOptionsFromArgs([`--${optionName}`, answer], cli.commandToExecute);
cli.optionsFromArgs.options[optionName] = parsed[optionName];
}
catch (e: any) {
return cli.closeWithError(e.message, cli.optionsFromArgs, true);
}
}
}
else {
Expand Down
Loading