feat(cli-test): include a verbose global option for cli commands #2147
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2147 +/- ##
=======================================
Coverage 91.93% 91.94%
=======================================
Files 38 38
Lines 10322 10330 +8
Branches 651 652 +1
=======================================
+ Hits 9490 9498 +8
Misses 820 820
Partials 12 12
Flags with carried forward coverage won't be shown. Click here to find out more. |
WilliamBergamin
left a comment
WilliamBergamin
left a comment
There was a problem hiding this comment.
lgtm thanks for working on this 💯
| cmd = cmd.concat(['--token', opts.token]); | ||
| } | ||
| if (opts.verbose) { | ||
| cmd = cmd.concat(['--verbose']); |
There was a problem hiding this comment.
Using concat alines with the syntax of this function and makes it readable, but looking at this line alone made me ask why push was not used instead 🤔
There was a problem hiding this comment.
IIRC push and concat are doing similar things but with different returns and underlying happenings - instead of adding to the existing array, concat creates a new one and returns this as a value while push returns the new length instead 📚 ✨
FWIW I think push is a reasonable substitution, but concat gives me confidence in fewer side effects.
|
@WilliamBergamin and thanks for a fast review! I'm looking into implementing this in our testing suites now - once I'm feeling good about that I'll merge this and will follow up with a release of |
zimeg
left a comment
zimeg
left a comment
There was a problem hiding this comment.
@WilliamBergamin @mwbrooks thank y'all both for the reviews! 🙏 ✨
I made one more change after some findings in local testing that would be helpful to include, but I'm not certain on the approach. If this still looks good I'll plan on merging and following up with a release soon!
| } | ||
|
|
||
| export type SlackCLIHostTargetOptions = Pick<SlackCLIGlobalOptions, 'qa' | 'dev' | 'apihost'>; | ||
| export type SlackCLIHostTargetOptions = Pick<SlackCLIGlobalOptions, 'qa' | 'dev' | 'apihost' | 'verbose'>; |
There was a problem hiding this comment.
📝 This was added to support the login helper commands:
const { authTicket, authTicketSlashCommand, output } = await SlackCLI.auth.loginNoPrompt({
apihost,
+ verbose: true,
});There was a problem hiding this comment.
I'm not thrilled with having this added to the SlackCLIHostTargetOptions type, but I think it makes sense as a possible argument.
Super open to suggestions here, but will plan on merging this soon-ish otherwise!
There was a problem hiding this comment.
Hmm this does feel off 🤔 Maybe we can rename SlackCLIHostTargetOptions to AuthLoginNoPromptOptions
Or create a new type named AuthLoginNoPromptGlobalOptions that is Pick<SlackCLIGlobalOptions, 'verbose'> and make the args of loginNoPrompt be something like
type AuthLoginNoPromptGlobalOptions = Pick<SlackCLIGlobalOptions, 'verbose'>;
type AuthLoginNoPromptOptions = SlackCLIHostTargetOptions & AuthLoginNoPromptGlobalOptions;
loginNoPrompt: async function loginNoPrompt(args?: AuthLoginNoPromptOptions): Promise<{...There was a problem hiding this comment.
@WilliamBergamin Nice call! When making this update I noticed a few other commands have a similar pattern for specific, additional, arguments.
The d689fa3 change creates similar arguments for each of the login helpers, which seems better to me 🎉
Let me know what you think though! I left the logout arguments unchanged, but these don't need to be updated to use verbose 😉
There was a problem hiding this comment.
Looks good 💯 ship it!
|
@WilliamBergamin Sweeeeeeet! Thanks for checking this out again! 🚀 And @mwbrooks thank you too for reviewing this! I'm excited to follow up with our testing improvements and a |
Summary
This PR adds a verbose option to
@slack/cli-testto add additional output information to specific commands.Preview
const createOutput = await SlackCLI.create({ template: 'slack-samples/deno-starter-template', branch: 'main', appPath, + verbose: true });Requirements