slackapi / node-slack-sdk

Slack Developer Kit for Node.js
https://tools.slack.dev/node-slack-sdk/
MIT License
3.28k stars 662 forks source link

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

Closed filmaj closed 2 weeks ago

filmaj commented 2 weeks ago

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:

TODO:

codecov[bot] commented 2 weeks ago

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 ```diff @@ 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](https://app.codecov.io/gh/slackapi/node-slack-sdk/pull/2090/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=slackapi) | Coverage Δ | | |---|---|---| | [cli-hooks](https://app.codecov.io/gh/slackapi/node-slack-sdk/pull/2090/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=slackapi) | `95.23% <ø> (ø)` | | | [cli-test](https://app.codecov.io/gh/slackapi/node-slack-sdk/pull/2090/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=slackapi) | `94.47% <82.96%> (-1.23%)` | :arrow_down: | | [oauth](https://app.codecov.io/gh/slackapi/node-slack-sdk/pull/2090/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=slackapi) | `77.39% <ø> (ø)` | | | [socket-mode](https://app.codecov.io/gh/slackapi/node-slack-sdk/pull/2090/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=slackapi) | `58.22% <ø> (ø)` | | | [web-api](https://app.codecov.io/gh/slackapi/node-slack-sdk/pull/2090/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=slackapi) | `96.88% <ø> (-0.01%)` | :arrow_down: | | [webhook](https://app.codecov.io/gh/slackapi/node-slack-sdk/pull/2090/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=slackapi) | `96.65% <ø> (ø)` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=slackapi#carryforward-flags-in-the-pull-request-comment) to find out more.
vegeris commented 2 weeks ago

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 commented 2 weeks ago

@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