Skip to content

Conversation

@zimeg
Copy link
Member

@zimeg zimeg commented Feb 12, 2025

Summary

This PR removes the trailing newline found in CLI outputs before logging to error or verbose since the logger adds a newline itself!

No change is made to the captured shell outputs, besides the existing removeANSIcolors function, to avoid breaking outputs that expect certain linebreaks 🙏 ✨

Preview

The following example uses changes in #2147 to showcase linebreaks:

Before changes

before

After changes

after

Requirements

@zimeg zimeg added bug M-T: A confirmed bug report. Issues are confirmed when the reproduction steps are documented semver:patch pkg:cli-test applies to `@slack/cli-test` labels Feb 12, 2025
@zimeg zimeg added this to the [email protected] milestone Feb 12, 2025
@zimeg zimeg self-assigned this Feb 12, 2025
Copy link
Member

@mwbrooks mwbrooks left a comment

Choose a reason for hiding this comment

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

💅🏻 So much nicer!

logger.verbose(`Output: ${this.removeANSIcolors(data.toString())}`);
const output = this.removeANSIcolors(data.toString());
sh.output += output;
logger.verbose(`Output: ${output.replace(/\r?\n$/, '')}`);
Copy link
Member

Choose a reason for hiding this comment

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

Taking it to the next level with \r? 🪟

Copy link
Member Author

Choose a reason for hiding this comment

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

Of course - always keeping the cartridge return in mind! But only sometimes 😉

@codecov
Copy link

codecov bot commented Feb 12, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.93%. Comparing base (8e5b06d) to head (f247cc5).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2148      +/-   ##
==========================================
+ Coverage   91.89%   91.93%   +0.04%     
==========================================
  Files          38       38              
  Lines       10320    10322       +2     
  Branches      649      651       +2     
==========================================
+ Hits         9484     9490       +6     
+ Misses        824      820       -4     
  Partials       12       12              
Flag Coverage Δ
cli-hooks 95.24% <ø> (ø)
cli-test 94.73% <100.00%> (+0.25%) ⬆️
oauth 77.39% <ø> (ø)
socket-mode 61.82% <ø> (ø)
web-api 96.88% <ø> (ø)
webhook 96.65% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@zimeg
Copy link
Member Author

zimeg commented Feb 12, 2025

@mwbrooks Thanks a ton for another fast review!

I noticed test coverage dropped a bit with this initial change, but I'm glad that called out a gap in the tests! A few cases were added to cover how outputs are handled with a focus on these not being adjusted (aside from ANSI removals).

With that, I think now's a nice time to merge! I'll follow up later with #2147 and then the promised release and then the actual updates to our tests 🎁 ✨

@zimeg zimeg merged commit 2874654 into main Feb 12, 2025
57 checks passed
@zimeg zimeg deleted the zimeg-cli-test-fix-extra-cli-newline branch February 12, 2025 00:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug M-T: A confirmed bug report. Issues are confirmed when the reproduction steps are documented pkg:cli-test applies to `@slack/cli-test` semver:patch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants