Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Merged
zimeg merged 2 commits into from
Mar 24, 2025
Merged

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

zimeg merged 2 commits into from
Mar 24, 2025

Conversation

@zimeg
Copy link
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 web-api@7.9.1 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

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
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
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
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

web-api@7.9.1

Development

Successfully merging this pull request may close these issues.

3 participants

@zimeg @WilliamBergamin