Skip to content

Conversation

@WilliamBergamin
Copy link
Contributor

Summary

These changes aim to remove the mock_fetch dependency that was cause some issues with Deno 2 in favor of using the standard library stubbing

testing

N/A

Requirements

  • I've read and understood the Contributing Guidelines and have done my best effort to follow them.
  • I've read and agree to the Code of Conduct.
  • I've ran deno task test after making the changes.

@WilliamBergamin WilliamBergamin requested review from mwbrooks and zimeg May 13, 2025 20:48
@WilliamBergamin WilliamBergamin self-assigned this May 13, 2025
@WilliamBergamin WilliamBergamin requested a review from a team as a code owner May 13, 2025 20:48
@WilliamBergamin WilliamBergamin added the enhancement New feature or request label May 13, 2025
@codecov
Copy link

codecov bot commented May 13, 2025

Codecov Report

Attention: Patch coverage is 34.17722% with 52 lines in your changes missing coverage. Please review.

Project coverage is 96.83%. Comparing base (8bf1dd2) to head (d1991d2).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
scripts/src/imports/update.ts 17.46% 52 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##              main     #122      +/-   ##
===========================================
- Coverage   100.00%   96.83%   -3.17%     
===========================================
  Files           36       39       +3     
  Lines         1133     1643     +510     
  Branches        17       25       +8     
===========================================
+ Hits          1133     1591     +458     
- Misses           0       52      +52     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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.

Hey @WilliamBergamin 👋🏻 I'll take a look at this PR in a bit, but I was curious how updating the test files adds support for Deno 2.x since the distributed module would not include the test files. Or, is this a step forward in adding Deno 2.x support?

Comment on lines +16 to +19
deno-version:
- v1.x
- v1.46.2
- v2.x
Copy link
Member

Choose a reason for hiding this comment

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

praise: Thanks for expanding this out!

# we test on both most recent stable version of deno (v1.x) as well as
# the version of deno used by Run on Slack (as noted in https://api.slack.com/slackcli/metadata.json)
deno-version: [v1.x, v1.45.4]
deno-version: [v1.x, v1.45.4, 2.x]
Copy link
Member

Choose a reason for hiding this comment

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

nit: For consistency with deno.yml, it might be nice to expand this out to a bullet list

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point 💯 but I did notice this is and issue for sample CI tests 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Oh interesting, maybe some of the sample CI test pull the latest and build it? Either way, these are positive changes!

@WilliamBergamin WilliamBergamin requested a review from mwbrooks May 29, 2025 14:53
@WilliamBergamin
Copy link
Contributor Author

@mwbrooks @zimeg all green for tests 👍 this PR is in a good state for review 🚀

Copy link
Member

@zimeg zimeg left a comment

Choose a reason for hiding this comment

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

@WilliamBergamin LGTM!

These are all super nice improvements to tests and testing scripts to keep us confident in this update 🙏 ✨

Copy link
Member

Choose a reason for hiding this comment

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

🪓 ✨

persist-credentials: false

- name: Set imports.deno-slack-api/ to ../deno-slack-api/src/ in import_map.json
- name: Set imports.deno-slack-api/ to ../deno-slack-api/src/ in imports
Copy link
Member

Choose a reason for hiding this comment

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

🔍 The logging in this updated file is so helpful!

https://github.com/slackapi/deno-slack-api/actions/runs/15397083256/job/43320475967#step:5:205

`import file` out content:
...
    "deno-slack-api/": "../deno-slack-api/src/",

deno-version: [v1.x, v1.45.4]
deno-version:
- v1.x
- v1.45.4
Copy link
Member

Choose a reason for hiding this comment

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

This might've been updated recently, but I'm finding 1.46.2 is the current ROSI runtime?

https://api.slack.com/slackcli/metadata.json

@WilliamBergamin WilliamBergamin merged commit 9cc067f into slackapi:main Jun 4, 2025
24 of 26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants