Skip to content

test(web-api): use channel_id instead of channels with files upload v2#2197

Merged
zimeg merged 2 commits intomainfrom
zimeg-test-web-api-files-upload-v2-channel-id
Mar 24, 2025
Merged

test(web-api): use channel_id instead of channels with files upload v2#2197
zimeg merged 2 commits intomainfrom
zimeg-test-web-api-files-upload-v2-channel-id

Conversation

@zimeg
Copy link
Copy Markdown
Member

@zimeg zimeg commented Mar 20, 2025 •
edited
Loading

Summary

This PR replaces channels with channel_id in tests testing the filesUploadV2 method to avoid warnings in test output 👾

Preview

Before changes: https://github.com/slackapi/node-slack-sdk/actions/runs/13958988507/job/39077186721#step:9:239

    getAllFileUploads
[WARN]  web-api:WebClient:0 Although the 'channels' parameter is still supported for smoother migration from legacy files.upload, we recommend using the new channel_id parameter with a single str value instead (e.g. 'C12345').
      ✔ adds a single file data to uploads with content supplied
[WARN]  web-api:WebClient:0 Although the 'channels' parameter is still supported for smoother migration from legacy files.upload, we recommend using the new channel_id parameter with a single str value instead (e.g. 'C12345').
      ✔ adds a single file data to uploads with file supplied
      ✔ adds multiple files data to an upload
[WARN]  web-api:WebClient:0 Although the 'channels' parameter is still supported for smoother migration from legacy files.upload, we recommend using the new channel_id parameter with a single str value instead (e.g. 'C12345').
[WARN]  web-api:WebClient:0 Although the 'channels' parameter is still supported for smoother migration from legacy files.upload, we recommend using the new channel_id parameter with a single str value instead (e.g. 'C12345').
[WARN]  web-api:WebClient:0 Although the 'channels' parameter is still supported for smoother migration from legacy files.upload, we recommend using the new channel_id parameter with a single str value instead (e.g. 'C12345').
[WARN]  web-api:WebClient:0 Although the 'channels' parameter is still supported for smoother migration from legacy files.upload, we recommend using the new channel_id parameter with a single str value instead (e.g. 'C12345').
[WARN]  web-api:WebClient:0 Although the 'channels' parameter is still supported for smoother migration from legacy files.upload, we recommend using the new channel_id parameter with a single str value instead (e.g. 'C12345').
      ✔ adds single and multiple files data to uploads
      ✔ handles multiple files with a single channel_id, initial_comment and/or thread_ts

After changes:

    getAllFileUploads
      ✔ adds a single file data to uploads with content supplied
      ✔ adds a single file data to uploads with file supplied
      ✔ adds multiple files data to an upload
      ✔ adds single and multiple files data to uploads
      ✔ handles multiple files with a single channel_id, initial_comment and/or thread_ts

Requirements

@zimeg zimeg added tests M-T: Testing work only pkg:web-api applies to `@slack/web-api` labels Mar 20, 2025
@zimeg zimeg added this to the [email protected] milestone Mar 20, 2025
@zimeg zimeg requested a review from WilliamBergamin March 20, 2025 16:07
@zimeg zimeg self-assigned this Mar 20, 2025
@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 20, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.99%. Comparing base (9210a1c) to head (044ac34).
Report is 1 commits behind head on main.

✅ All tests successful. No failed tests found.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #2197   +/-   ##
=======================================
  Coverage   91.99%   91.99%           
=======================================
  Files          38       38           
  Lines       10386    10386           
  Branches      657      657           
=======================================
  Hits         9555     9555           
  Misses        819      819           
  Partials       12       12
Flag Coverage Δ
cli-hooks 95.23% <ø> (ø)
cli-test 94.76% <ø> (ø)
oauth 77.39% <ø> (ø)
socket-mode 61.82% <ø> (ø)
web-api 96.93% <100.00%> (ø)
webhook 96.65% <ø> (ø)

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

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@zimeg
Copy link
Copy Markdown
Member Author

zimeg commented Mar 20, 2025

🤔 Super glad this was suggested to be in another PR by @WilliamBergamin! I'm now wondering if one test should keep "channels" until the next breaking release to avoid possible regressions in related changes?

For quick reference, here's the warning output:

Although the 'channels' parameter is still supported for smoother migration from legacy files.upload, we recommend using the new channel_id parameter with a single str value instead (e.g. 'C12345').

WilliamBergamin
WilliamBergamin approved these changes Mar 20, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@WilliamBergamin WilliamBergamin left a comment

Choose a reason for hiding this comment

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

This seems good 💯 removing the warning from the tests output is nice!

Maybe we could add a unit tests that ensure the warning is printed if channels is used instead of channel_id, but not a blocker

@zimeg
Copy link
Copy Markdown
Member Author

zimeg commented Mar 24, 2025

@WilliamBergamin Thanks so much for reviewing!

I was looking into adding a regression test here, but found the exact case covered already- 🎉

node-slack-sdk/packages/web-api/src/file-upload.spec.ts

Lines 84 to 92 in 9210a1c

it('warns when channels is supplied', async () => {
const channelsSupplied = {
file: Buffer.from('test'),
filename: 'test',
channels: 'C1234',
};
await getFileUploadJob(channelsSupplied, logger);
assert.isTrue((logger.warn as sinon.SinonSpy).calledWith(buildChannelsWarning()));
}); 

  file-upload
    getFileUploadJob
      ✔ returns a fileUploadEntry
      ✔ warns if legacy filetype
      ✔ warns when missing or invalid filename
      ✔ warns when possibly missing a file extension in filename supplied
      ✔ warns when channels is supplied
      ✔ errors when channels is supplied with csv value, aka multiple channels

I'll update this branch before merging, but am feeling good about these testing changes!

@zimeg zimeg merged commit 0101781 into main Mar 24, 2025
57 checks passed
@zimeg zimeg deleted the zimeg-test-web-api-files-upload-v2-channel-id branch March 24, 2025 19:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@WilliamBergamin WilliamBergamin WilliamBergamin approved these changes

Assignees

@zimeg zimeg

Labels

pkg:web-api applies to `@slack/web-api` tests M-T: Testing work only

Projects

None yet

Milestone

[email protected]

Development

Successfully merging this pull request may close these issues.

2 participants

@zimeg @WilliamBergamin