-
Couldn't load subscription status.
- Fork 680
cli-test(fix): wait for cli run start trace instead of activity output #2355
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.
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2355 +/- ##
=======================================
Coverage 92.77% 92.77%
=======================================
Files 39 39
Lines 10693 10693
Branches 692 692
=======================================
Hits 9920 9920
Misses 761 761
Partials 12 12
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👾 Rambling thoughts on this change but I am open to follow up or updates otherwise!
| ); | ||
| sandbox.assert.calledWith( | ||
| waitForOutputSpy, | ||
| SlackTracerId.SLACK_TRACE_PLATFORM_RUN_START, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🗣️ note: This is preferred over SLACK_TRACE_PLATFORM_RUN_READY since the "ready" variable appears once activity logs stream for CLI managed connections. It does not appear for bolt apps at this time.
👁️🗨️ thought: I'm unsure if we should add that trace to the CLI outputs in a follow up PR? For now I think using a trace is still an improvement but I'm somewhat worried about tests skipping to the next step before run is settled for some apps... 🦕
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm unsure if we should add that trace to the CLI outputs in a follow up PR?
Just checking if I understand correctly - are you thinking that the CLI would print the SLACK_TRACE_PLATFORM_RUN_READY trace for sdk-managed socket connections?
|
LGTM! Like the more reliable detection of running Slack apps 👏 👏 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Very healthy fix for Bolt tests! Sorry that this review took 3 weeks! :shame:
| ); | ||
| sandbox.assert.calledWith( | ||
| waitForOutputSpy, | ||
| SlackTracerId.SLACK_TRACE_PLATFORM_RUN_START, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm unsure if we should add that trace to the CLI outputs in a follow up PR?
Just checking if I understand correctly - are you thinking that the CLI would print the SLACK_TRACE_PLATFORM_RUN_READY trace for sdk-managed socket connections?
|
@ewanek1 @mwbrooks Thanks both for the kind reviews. And never worries on a rushed review for this! 👾
Exactly this! I'm not finding such printed at this time, but I'm also not sure if that'd be possible to know for SDK managed connections... Somewhat following that, I'm hesitant to release this change since it might disrupt our E2E tests if workflows are started after "PLATFORM_RUN_START" and before "PLATFORM_RUN_READY" but we can perhaps wait for the second trace in an explicit step? |
Summary
This PR uses
SLACK_TRACE_PLATFORM_RUN_STARTvariable as a signal of an active running app instead of the exact output:Fixes an issue where
boltapps were not detected as running.Requirements