Skip to content

Comments

fix: anticipate optional chained parameters as undefined when parsing events details#2700

Merged
hello-ashleyintech merged 5 commits intomainfrom
ah-troubleshoot-test
Oct 28, 2025
Merged

fix: anticipate optional chained parameters as undefined when parsing events details#2700
hello-ashleyintech merged 5 commits intomainfrom
ah-troubleshoot-test

Conversation

@hello-ashleyintech
Copy link
Contributor

@hello-ashleyintech hello-ashleyintech commented Oct 27, 2025

Summary

This PR aims to resolve recurring failing tests on main.

Requirements (place an x in each [ ])

@hello-ashleyintech hello-ashleyintech changed the title Update to anticipate optional Update tests to anticipate optional / undefined params Oct 27, 2025
@hello-ashleyintech hello-ashleyintech changed the title Update tests to anticipate optional / undefined params fix: update tests to anticipate optional / undefined params Oct 27, 2025
@codecov
Copy link

codecov bot commented Oct 27, 2025 •
edited
Loading

Codecov Report

❌ Patch coverage is 66.66667% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 93.44%. Comparing base (3a8953c) to head (3c0c3ce).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/helpers.ts 66.66% 1 Missing ⚠️
Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #2700   +/-   ##
=======================================
  Coverage   93.44%   93.44%           
=======================================
  Files          37       37           
  Lines        7668     7668           
  Branches      669      669           
=======================================
  Hits         7165     7165           
  Misses        498      498           
  Partials        5        5

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

@hello-ashleyintech hello-ashleyintech marked this pull request as ready for review October 27, 2025 14:29
@zimeg zimeg added this to the 4.6.0 milestone Oct 28, 2025
@zimeg
Copy link
Member

zimeg commented Oct 28, 2025

🔍 So interesting! I'm noticing the scheduled tests are failing with the following when I run these on my machine:

src/helpers.ts:48:28 - error TS18048: 'event.channel' is possibly 'undefined'.

48         } else if ('id' in event.channel) {
                              ~~~~~~~~~~~~~


Found 1 error in src/helpers.ts:48

🔗 https://github.com/slackapi/bolt-js/actions/runs/18825825888/job/53708278997

👁️‍🗨️ But it's not clear to me which dependencies might've changed to cause this? So curious but will review this in acceptance that dependencies change otherwise.

@zimeg zimeg changed the title fix: update tests to anticipate optional / undefined params fix: anticipate optional chained parameters as undefined when parsing events details Oct 28, 2025
zimeg
zimeg approved these changes Oct 28, 2025
View reviewed changes
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.

@hello-ashleyintech LGTM! And thanks for noticing a break in these test!

I'm leaving a few comments of suggestion below but this ought fix tests I believe 🚢 💨

zimeg
zimeg approved these changes Oct 28, 2025
View reviewed changes
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.

📫 One more comment noticed!

src/helpers.ts
Comment on lines 46 to 48
if (typeof event?.channel === 'string') {
foundConversationId = event?.channel;
} else if (typeof event?.channel === 'object' && 'id' in event.channel) {
Copy link
Member

@zimeg zimeg Oct 28, 2025

Choose a reason for hiding this comment

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

Suggested change
if (typeof event?.channel === 'string') {
foundConversationId = event?.channel;
} else if (typeof event?.channel === 'object' && 'id' in event.channel) {
if (typeof event.channel === 'string') {
foundConversationId = event?.channel;
} else if (typeof event.channel === 'object' && 'id' in event.channel) {

🤖 question: We also check that event isn't undefined earlier - are these chainings alright to remove?

bolt-js/src/helpers.ts

Line 39 in 9e1e350

if (body.event !== undefined) {

@zimeg zimeg mentioned this pull request Oct 28, 2025
Merged
2 tasks
hello-ashleyintech and others added 2 commits October 28, 2025 09:44
@hello-ashleyintech @zimeg
Update src/helpers.ts
dc39ba7 
Co-authored-by: Eden Zimbelman <[email protected]>
@hello-ashleyintech @zimeg
Update src/helpers.ts
3c0c3ce 
Co-authored-by: Eden Zimbelman <[email protected]>
@hello-ashleyintech hello-ashleyintech merged commit 0cf26ad into main Oct 28, 2025
17 checks passed
@hello-ashleyintech hello-ashleyintech deleted the ah-troubleshoot-test branch October 28, 2025 13:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zimeg zimeg zimeg approved these changes

@misscoded misscoded Awaiting requested review from misscoded

Assignees

No one assigned

Labels

semver:patch tests M-T: Testing work only TypeScript-specific

Projects

None yet

Milestone

4.6.0

Development

Successfully merging this pull request may close these issues.

2 participants

@hello-ashleyintech @zimeg