From 897f001280947cd19d081cb8747ee8098d955a2e Mon Sep 17 00:00:00 2001 From: Mateusz Szostok Date: Thu, 8 Feb 2024 07:48:01 -0800 Subject: [PATCH] Update Cloud Slack to always respond in a thread (#1372) * Refactor e2e tests to handle thread messages assertion * Add OnChannel assertion --- .../actions/setup-go-mod-private/action.yaml | 5 + .github/workflows/branch-build.yml | 11 +- .github/workflows/prod-e2e-test.yml | 3 - pkg/bot/slack_cloud.go | 49 +++++--- pkg/bot/slack_shared.go | 10 ++ .../cloud_slack_dev_e2e_test.go | 41 +++--- test/commplatform/discord_tester.go | 5 + test/commplatform/generic.go | 5 + test/commplatform/slack_tester.go | 118 ++++++++++++------ test/commplatform/teams_tester.go | 5 + test/e2e/bots_test.go | 108 +++++++--------- 11 files changed, 218 insertions(+), 142 deletions(-) diff --git a/.github/actions/setup-go-mod-private/action.yaml b/.github/actions/setup-go-mod-private/action.yaml index 794769e8e..8e2c31324 100644 --- a/.github/actions/setup-go-mod-private/action.yaml +++ b/.github/actions/setup-go-mod-private/action.yaml @@ -12,6 +12,11 @@ inputs: runs: using: "composite" steps: + - name: Setup Go + uses: actions/setup-go@v4 + with: + go-version-file: './test/go.mod' + cache: true - name: Download Go modules with private repository shell: bash run: | diff --git a/.github/workflows/branch-build.yml b/.github/workflows/branch-build.yml index 168035a0f..05e9b10bc 100644 --- a/.github/workflows/branch-build.yml +++ b/.github/workflows/branch-build.yml @@ -71,7 +71,6 @@ jobs: GORELEASER_CURRENT_TAG: ${{ matrix.image-version }} IMAGE_TAG: ${{ matrix.image-version }} - integration-tests: if: github.event_name != 'repository_dispatch' # skip if triggered by repository_dispatch name: Integration tests @@ -97,12 +96,6 @@ jobs: with: persist-credentials: false - - name: Setup Go - uses: actions/setup-go@v4 - with: - go-version-file: 'go.mod' - cache: true - - name: Setup Go modules uses: ./.github/actions/setup-go-mod-private with: @@ -298,14 +291,14 @@ jobs: uses: ./.github/actions/cloud-slack-e2e with: access_token: ${{ secrets.E2E_TEST_GH_DEV_ACCOUNT_PAT }} - + slack_workspace_name: ${{ secrets.E2E_DEV_SLACK_WORKSPACE_NAME }} slack_email: ${{ secrets.E2E_DEV_SLACK_EMAIL }} slack_password: ${{ secrets.E2E_DEV_SLACK_USER_PASSWORD }} slack_bot_display_name: "BotkubeDev" slack_tester_bot_token: ${{ secrets.E2E_DEV_SLACK_TESTER_BOT_TOKEN }} slack_tester_bot_name: "botkubedev" - + botkube_cloud_api_base_url: "https://api-dev.botkube.io" botkube_cloud_email: ${{ secrets.E2E_DEV_BOTKUBE_CLOUD_EMAIL }} botkube_cloud_password: ${{ secrets.E2E_DEV_BOTKUBE_CLOUD_PASSWORD }} diff --git a/.github/workflows/prod-e2e-test.yml b/.github/workflows/prod-e2e-test.yml index 770c40c18..8e1d7b31d 100644 --- a/.github/workflows/prod-e2e-test.yml +++ b/.github/workflows/prod-e2e-test.yml @@ -8,9 +8,6 @@ on: env: HELM_VERSION: v3.9.0 K3D_VERSION: v5.4.6 - IMAGE_REGISTRY: "ghcr.io" - IMAGE_REPOSITORY: "kubeshop/botkube" - IMAGE_TAG: v9.99.9-dev # TODO: Use commit hash tag to make the predictable builds for each commit on branch GIT_USER: botkube-dev jobs: diff --git a/pkg/bot/slack_cloud.go b/pkg/bot/slack_cloud.go index 68e6de050..5945dfba8 100644 --- a/pkg/bot/slack_cloud.go +++ b/pkg/bot/slack_cloud.go @@ -550,7 +550,7 @@ func (b *CloudSlack) send(ctx context.Context, event slackMessage, resp interact var file *slack.File var err error if len(markdown) >= slackMaxMessageSize { - file, err = uploadFileToSlack(ctx, event.Channel, resp, b.client, event.ThreadTimeStamp) + file, err = b.uploadFileToSlack(ctx, event, resp) if err != nil { return err } @@ -578,10 +578,6 @@ func (b *CloudSlack) send(ctx context.Context, event slackMessage, resp interact b.renderer.RenderInteractiveMessage(resp), } - if ts := b.getThreadOptionIfNeeded(event, file); ts != nil { - options = append(options, ts) - } - if resp.ReplaceOriginal && event.ResponseURL != "" { options = append(options, slack.MsgOptionReplaceOriginal(event.ResponseURL)) } @@ -591,6 +587,10 @@ func (b *CloudSlack) send(ctx context.Context, event slackMessage, resp interact return fmt.Errorf("while posting Slack message visible only to user: %w", err) } } else { + if ts := b.getThreadOptionIfNeeded(event, file); ts != nil { + options = append(options, ts) + } + if _, _, err := b.client.PostMessageContext(ctx, event.Channel, options...); err != nil { return fmt.Errorf("while posting Slack message: %w", err) } @@ -600,6 +600,24 @@ func (b *CloudSlack) send(ctx context.Context, event slackMessage, resp interact return nil } +func (b *CloudSlack) uploadFileToSlack(ctx context.Context, event slackMessage, resp interactive.CoreMessage) (*slack.File, error) { + params := slack.FileUploadParameters{ + Filename: "Response.txt", + Title: "Response.txt", + InitialComment: resp.Description, + Content: interactive.MessageToPlaintext(resp, interactive.NewlineFormatter), + Channels: []string{event.Channel}, + ThreadTimestamp: event.GetTimestamp(), + } + + file, err := b.client.UploadFileContext(ctx, params) + if err != nil { + return nil, fmt.Errorf("while uploading file: %w", err) + } + + return file, nil +} + func (b *CloudSlack) findAndTrimBotMention(msg string) (string, bool) { if !b.botMentionRegex.MatchString(msg) { return "", false @@ -619,20 +637,17 @@ func (b *CloudSlack) BotName() string { } func (b *CloudSlack) getThreadOptionIfNeeded(event slackMessage, file *slack.File) slack.MsgOption { - //if the message is from thread then add an option to return the response to the thread - if event.ThreadTimeStamp != "" { - return slack.MsgOptionTS(event.ThreadTimeStamp) - } - - if file == nil { - return nil + if file != nil { + // If the message was already as a file attachment, reply it a given thread + for _, share := range file.Shares.Public { + if len(share) >= 1 && share[0].Ts != "" { + return slack.MsgOptionTS(share[0].Ts) + } + } } - // If the message was already as a file attachment, reply it a given thread - for _, share := range file.Shares.Public { - if len(share) >= 1 && share[0].Ts != "" { - return slack.MsgOptionTS(share[0].Ts) - } + if ts := event.GetTimestamp(); ts != "" { + return slack.MsgOptionTS(ts) } return nil diff --git a/pkg/bot/slack_shared.go b/pkg/bot/slack_shared.go index 07a67c373..7cf72820b 100644 --- a/pkg/bot/slack_shared.go +++ b/pkg/bot/slack_shared.go @@ -69,3 +69,13 @@ type slackMessage struct { EventTimeStamp string RootMessageTimeStamp string } + +// GetTimestamp returns the timestamp for the response message. +func (s *slackMessage) GetTimestamp() string { + // If the event is coming from the thread, then we simply respond in that thread + if s.ThreadTimeStamp != "" { + return s.ThreadTimeStamp + } + // otherwise, we use the event timestamp to respond in the thread to the message that triggered our response + return s.EventTimeStamp +} diff --git a/test/cloud-slack-dev-e2e/cloud_slack_dev_e2e_test.go b/test/cloud-slack-dev-e2e/cloud_slack_dev_e2e_test.go index c21237bcc..b4e061260 100644 --- a/test/cloud-slack-dev-e2e/cloud_slack_dev_e2e_test.go +++ b/test/cloud-slack-dev-e2e/cloud_slack_dev_e2e_test.go @@ -374,34 +374,46 @@ func TestCloudSlackE2E(t *testing.T) { t.Log("Testing ping with --cluster-name") command := fmt.Sprintf("ping --cluster-name %s", deployment.Name) expectedMessage := fmt.Sprintf("`%s` on `%s`\n```\npong", command, deployment.Name) - tester.PostMessageToBot(t, channel.ID(), command) + tester.PostMessageToBot(t, channel.Identifier(), command) err = tester.WaitForLastMessageContains(tester.BotUserID(), channel.ID(), expectedMessage) require.NoError(t, err) t.Log("Testing ping for not connected deployment #2") command = "ping" expectedBlockMessage := notConnectedMessage(deployment2.Name, deployment2.ID) - tester.PostMessageToBot(t, channel.ID(), fmt.Sprintf("%s --cluster-name %s", command, deployment2.Name)) + tester.PostMessageToBot(t, channel.Identifier(), fmt.Sprintf("%s --cluster-name %s", command, deployment2.Name)) renderedMsg := interactive.RenderMessage(tester.MDFormatter(), expectedBlockMessage) renderedMsg = strings.Replace(renderedMsg, "\n", " ", -1) renderedMsg = strings.TrimSuffix(renderedMsg, " ") err = tester.WaitForLastInteractiveMessagePostedEqualWithCustomRender(tester.BotUserID(), channel.ID(), renderedMsg) + if err != nil { // the new cloud backend not release yet + t.Logf("Fallback to the old behavior with message sent at the channel level...") + err = tester.OnChannel().WaitForLastInteractiveMessagePostedEqualWithCustomRender(tester.BotUserID(), channel.ID(), renderedMsg) + } require.NoError(t, err) t.Log("Testing ping for not existing deployment") command = "ping" deployName := "non-existing-deployment" expectedMessage = fmt.Sprintf("*Instance not found* The cluster %q does not exist.", deployName) - tester.PostMessageToBot(t, channel.ID(), fmt.Sprintf("%s --cluster-name %s", command, deployName)) + tester.PostMessageToBot(t, channel.Identifier(), fmt.Sprintf("%s --cluster-name %s", command, deployName)) err = tester.WaitForLastMessageContains(tester.BotUserID(), channel.ID(), expectedMessage) + if err != nil { // the new cloud backend not release yet + t.Logf("Fallback to the old behavior with message sent at the channel level...") + err = tester.OnChannel().WaitForLastMessageContains(tester.BotUserID(), channel.ID(), expectedMessage) + } require.NoError(t, err) t.Log("Setting cluster as default") - tester.PostMessageToBot(t, channel.ID(), fmt.Sprintf("cloud set default-instance %s", deployment.ID)) + tester.PostMessageToBot(t, channel.Identifier(), fmt.Sprintf("cloud set default-instance %s", deployment.ID)) t.Log("Waiting for confirmation message...") expectedClusterDefaultMsg := fmt.Sprintf(":white_check_mark: Instance %s was successfully selected as the default cluster for this channel.", deployment.Name) err = tester.WaitForLastMessageEqual(tester.BotUserID(), channel.ID(), expectedClusterDefaultMsg) + if err != nil { // the new cloud backend not release yet + t.Logf("Fallback to the old behavior with message sent at the channel level...") + err = tester.OnChannel().WaitForLastMessageEqual(tester.BotUserID(), channel.ID(), expectedClusterDefaultMsg) + } require.NoError(t, err) t.Log("Testing getting all deployments") @@ -411,7 +423,7 @@ func TestCloudSlackE2E(t *testing.T) { strings.Contains(msg, "coredns") && strings.Contains(msg, "botkube"), 0, "" } - tester.PostMessageToBot(t, channel.ID(), command) + tester.PostMessageToBot(t, channel.Identifier(), command) err = tester.WaitForMessagePosted(tester.BotUserID(), channel.ID(), 1, assertionFn) require.NoError(t, err) }) @@ -458,22 +470,23 @@ func TestCloudSlackE2E(t *testing.T) { } return result, 0, "" } - err = tester.WaitForMessagePosted(tester.BotUserID(), channel.ID(), 1, assertionFn) + err = tester.OnChannel().WaitForMessagePosted(tester.BotUserID(), channel.ID(), 1, assertionFn) require.NoError(t, err) }) t.Run("Botkube Deployment -> Cloud sync", func(t *testing.T) { t.Log("Disabling notification...") - tester.PostMessageToBot(t, channel.ID(), "disable notifications") + tester.PostMessageToBot(t, channel.Identifier(), "disable notifications") t.Log("Waiting for config reload message...") expectedReloadMsg := fmt.Sprintf(":arrows_counterclockwise: Configuration reload requested for cluster '%s'. Hold on a sec...", deployment.Name) - err = tester.WaitForMessagePostedRecentlyEqual(tester.BotUserID(), channel.ID(), expectedReloadMsg) + + err = tester.OnChannel().WaitForMessagePostedRecentlyEqual(tester.BotUserID(), channel.ID(), expectedReloadMsg) require.NoError(t, err) t.Log("Waiting for watch begin message...") expectedWatchBeginMsg := fmt.Sprintf("My watch begins for cluster '%s'! :crossed_swords:", deployment.Name) recentMessages := 2 // take into the account the optional "upgrade checker message" - err = tester.WaitForMessagePosted(tester.BotUserID(), channel.ID(), recentMessages, func(msg string) (bool, int, string) { + err = tester.OnChannel().WaitForMessagePosted(tester.BotUserID(), channel.ID(), recentMessages, func(msg string) (bool, int, string) { if !strings.EqualFold(expectedWatchBeginMsg, msg) { count := diff.CountMatchBlock(expectedWatchBeginMsg, msg) msgDiff := diff.Diff(expectedWatchBeginMsg, msg) @@ -490,7 +503,7 @@ func TestCloudSlackE2E(t *testing.T) { command := "status notifications" expectedBody := formatx.CodeBlock(fmt.Sprintf("Notifications from cluster '%s' are disabled here.", deployment.Name)) expectedMessage := fmt.Sprintf("%s\n%s", cmdHeader(command), expectedBody) - tester.PostMessageToBot(t, channel.ID(), "status notifications") + tester.PostMessageToBot(t, channel.Identifier(), "status notifications") err = tester.WaitForLastMessageEqual(tester.BotUserID(), channel.ID(), expectedMessage) require.NoError(t, err) }) @@ -502,13 +515,13 @@ func TestCloudSlackE2E(t *testing.T) { t.Log("Waiting for config reload message...") expectedReloadMsg := fmt.Sprintf(":arrows_counterclockwise: Configuration reload requested for cluster '%s'. Hold on a sec...", deployment.Name) - err = tester.WaitForMessagePostedRecentlyEqual(tester.BotUserID(), channel.ID(), expectedReloadMsg) + err = tester.OnChannel().WaitForMessagePostedRecentlyEqual(tester.BotUserID(), channel.ID(), expectedReloadMsg) require.NoError(t, err) t.Log("Waiting for watch begin message...") expectedWatchBeginMsg := fmt.Sprintf("My watch begins for cluster '%s'! :crossed_swords:", deployment.Name) recentMessages := 2 // take into the account the optional "upgrade checker message" - err = tester.WaitForMessagePosted(tester.BotUserID(), channel.ID(), recentMessages, func(msg string) (bool, int, string) { + err = tester.OnChannel().WaitForMessagePosted(tester.BotUserID(), channel.ID(), recentMessages, func(msg string) (bool, int, string) { if !strings.EqualFold(expectedWatchBeginMsg, msg) { count := diff.CountMatchBlock(expectedWatchBeginMsg, msg) msgDiff := diff.Diff(expectedWatchBeginMsg, msg) @@ -517,13 +530,13 @@ func TestCloudSlackE2E(t *testing.T) { return true, 0, "" }) require.NoError(t, err) - tester.PostMessageToBot(t, channel.ID(), "list sources") + tester.PostMessageToBot(t, channel.Identifier(), "list sources") t.Log("Waiting for empty source list...") expectedSourceListMsg := fmt.Sprintf("%s\n```\nSOURCE ENABLED RESTARTS STATUS LAST_RESTART\n```", cmdHeader("list sources")) err = tester.WaitForLastMessageEqual(tester.BotUserID(), channel.ID(), expectedSourceListMsg) require.NoError(t, err) - tester.PostMessageToBot(t, channel.ID(), "list actions") + tester.PostMessageToBot(t, channel.Identifier(), "list actions") t.Log("Waiting for actions list...") expectedActionsListMsg := fmt.Sprintf("%s\n```\nACTION ENABLED DISPLAY NAME\naction_xxx22 true Action Name\n```", cmdHeader("list actions")) err = tester.WaitForLastMessageEqual(tester.BotUserID(), channel.ID(), expectedActionsListMsg) diff --git a/test/commplatform/discord_tester.go b/test/commplatform/discord_tester.go index 1ea3bdf6f..b22e124cc 100644 --- a/test/commplatform/discord_tester.go +++ b/test/commplatform/discord_tester.go @@ -446,3 +446,8 @@ func (d *DiscordTester) findUserID(t *testing.T, name string) string { func (d *DiscordTester) ReplaceBotNamePlaceholder(msg *interactive.CoreMessage, clusterName string) { msg.ReplaceBotNamePlaceholder(d.BotName()) } + +// OnChannel assertion is the default mode for Discord, no action needed. +func (d *DiscordTester) OnChannel() BotDriver { + return d +} diff --git a/test/commplatform/generic.go b/test/commplatform/generic.go index ba5b5a11e..0f2f96721 100644 --- a/test/commplatform/generic.go +++ b/test/commplatform/generic.go @@ -51,6 +51,11 @@ type BotDriver interface { Timeout() time.Duration ReplaceBotNamePlaceholder(msg *interactive.CoreMessage, clusterName string) AssertEquals(expectedMessage string) MessageAssertion + // OnChannel sets the expectation that the message should be posted in the channel. This is necessary when Bots + // by default expect a given message to be posted in the thread of the recently sent message. + // For example, in the context of source notification, we need to alter that default behavior + // and expect the message on the channel instead. + OnChannel() BotDriver } type MessageAssertion func(content string) (bool, int, string) diff --git a/test/commplatform/slack_tester.go b/test/commplatform/slack_tester.go index 25f39d968..61cc9d346 100644 --- a/test/commplatform/slack_tester.go +++ b/test/commplatform/slack_tester.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "maps" "regexp" "strings" "testing" @@ -24,6 +25,8 @@ import ( "github.com/kubeshop/botkube/pkg/ptr" ) +var _ BotDriver = (*SlackTester)(nil) + const ( slackInteractiveElementsMsgSuffix = ", with interactive elements" ) @@ -61,22 +64,25 @@ func (s *SlackChannel) Identifier() string { } type SlackTester struct { - cli *slack.Client - cfg SlackConfig - botUserID string - testerUserID string - channel Channel - secondChannel Channel - thirdChannel Channel - mdFormatter interactive.MDFormatter - configProviderApiKey string + cli *slack.Client + cfg SlackConfig + botUserID string + testerUserID string + firstChannel Channel + secondChannel Channel + thirdChannel Channel + mdFormatter interactive.MDFormatter + configProviderApiKey string + recentlyPostedMsgTS map[string]string + channelsByName map[string]Channel + restoreRecentlyPostedMsgTS func() } func (s *SlackTester) ReplaceBotNamePlaceholder(msg *interactive.CoreMessage, clusterName string) { msg.ReplaceBotNamePlaceholder(s.BotName(), api.BotNameWithClusterName(clusterName)) } -func NewSlackTester(slackCfg SlackConfig, apiKey *string) (BotDriver, error) { +func NewSlackTester(slackCfg SlackConfig, apiKey *string) (*SlackTester, error) { var token string if slackCfg.TesterAppToken == "" && slackCfg.TesterBotToken == "" && slackCfg.CloudTesterAppToken == "" { return nil, errors.New("slack tester tokens are not set") @@ -100,7 +106,15 @@ func NewSlackTester(slackCfg SlackConfig, apiKey *string) (BotDriver, error) { mdFormatter := interactive.NewMDFormatter(interactive.NewlineFormatter, func(msg string) string { return fmt.Sprintf("*%s*", msg) }) - return &SlackTester{cli: slackCli, cfg: slackCfg, mdFormatter: mdFormatter, configProviderApiKey: ptr.ToValue(apiKey)}, nil + return &SlackTester{ + cli: slackCli, + cfg: slackCfg, + mdFormatter: mdFormatter, + configProviderApiKey: ptr.ToValue(apiKey), + recentlyPostedMsgTS: make(map[string]string), + channelsByName: make(map[string]Channel), + restoreRecentlyPostedMsgTS: func() {}, + }, nil } func (s *SlackTester) InitUsers(t *testing.T) { @@ -115,7 +129,7 @@ func (s *SlackTester) InitUsers(t *testing.T) { func (s *SlackTester) InitChannels(t *testing.T) []func() { channel, cleanupChannelFn := s.CreateChannel(t, "first") - s.channel = channel + s.firstChannel = channel secondChannel, cleanupSecondChannelFn := s.CreateChannel(t, "second") s.secondChannel = secondChannel @@ -147,7 +161,7 @@ func (s *SlackTester) TesterUserID() string { } func (s *SlackTester) FirstChannel() Channel { - return s.channel + return s.firstChannel } func (s *SlackTester) SecondChannel() Channel { @@ -175,9 +189,11 @@ func (s *SlackTester) PostInitialMessage(t *testing.T, channelName string) { require.NoError(t, err) } -func (s *SlackTester) PostMessageToBot(t *testing.T, channel, command string) { +func (s *SlackTester) PostMessageToBot(t *testing.T, channelName, command string) { message := fmt.Sprintf("<@%s> %s", s.cfg.BotUsername(), command) - _, _, err := s.cli.PostMessage(channel, slack.MsgOptionText(message, false)) + _, ts, err := s.cli.PostMessage(channelName, slack.MsgOptionText(message, false)) + id := s.channelsByName[channelName].ID() + s.recentlyPostedMsgTS[id] = ts require.NoError(t, err) } @@ -217,6 +233,8 @@ func (s *SlackTester) AssertEquals(expectedMsg string) MessageAssertion { } func (s *SlackTester) WaitForMessagePosted(userID, channelID string, limitMessages int, assertFn MessageAssertion) error { + defer s.restoreMsgTsIfNeeded() + var fetchedMessages []slack.Message var lastErr error var diffMessage string @@ -226,16 +244,13 @@ func (s *SlackTester) WaitForMessagePosted(userID, channelID string, limitMessag } err := wait.PollUntilContextTimeout(context.Background(), pollInterval, s.cfg.MessageWaitTimeout, false, func(ctx context.Context) (done bool, err error) { - historyRes, err := s.cli.GetConversationHistory(&slack.GetConversationHistoryParameters{ - ChannelID: channelID, Limit: limitMessages, - }) + fetchedMessages, err = s.getMessages(channelID, limitMessages) if err != nil { lastErr = err return false, nil } - fetchedMessages = historyRes.Messages - for _, msg := range historyRes.Messages { + for _, msg := range fetchedMessages { if msg.User != userID { continue } @@ -255,7 +270,7 @@ func (s *SlackTester) WaitForMessagePosted(userID, channelID string, limitMessag return false, nil }) if lastErr == nil { - lastErr = fmt.Errorf("message assertion function returned false%s", diffMessage) + lastErr = fmt.Errorf("message assertion function returned false; fetched messages: %s\ndiff:\n%s", structDumper.Sdump(fetchedMessages), diffMessage) } if err != nil { if wait.Interrupted(err) { @@ -267,20 +282,34 @@ func (s *SlackTester) WaitForMessagePosted(userID, channelID string, limitMessag return nil } +func (s *SlackTester) getMessages(channelID string, limitMessages int) ([]slack.Message, error) { + if ts := s.recentlyPostedMsgTS[channelID]; ts != "" { + replies, _, _, err := s.cli.GetConversationReplies(&slack.GetConversationRepliesParameters{ + ChannelID: channelID, + Timestamp: ts, + Limit: limitMessages, + }) + return replies, err + } + + history, err := s.cli.GetConversationHistory(&slack.GetConversationHistoryParameters{ + ChannelID: channelID, Limit: limitMessages, + }) + return history.Messages, err +} + func (s *SlackTester) WaitForInteractiveMessagePosted(userID, channelID string, limitMessages int, assertFn MessageAssertion) error { + defer s.restoreMsgTsIfNeeded() var fetchedMessages []slack.Message var lastErr error err := wait.PollUntilContextTimeout(context.Background(), pollInterval, s.cfg.MessageWaitTimeout, false, func(_ context.Context) (done bool, err error) { - historyRes, err := s.cli.GetConversationHistory(&slack.GetConversationHistoryParameters{ - ChannelID: channelID, Limit: limitMessages, - }) + fetchedMessages, err = s.getMessages(channelID, limitMessages) if err != nil { lastErr = err return false, nil } - fetchedMessages = historyRes.Messages - for _, msg := range historyRes.Messages { + for _, msg := range fetchedMessages { if msg.User != userID { continue } @@ -294,14 +323,13 @@ func (s *SlackTester) WaitForInteractiveMessagePosted(userID, channelID string, if !ok { continue } - return true, nil } return false, nil }) if lastErr == nil { - lastErr = errors.New("message assertion function returned false") + lastErr = fmt.Errorf("message assertion function returned false; fetched messages: %s", structDumper.Sdump(fetchedMessages)) } if err != nil { if wait.Interrupted(err) { @@ -314,19 +342,17 @@ func (s *SlackTester) WaitForInteractiveMessagePosted(userID, channelID string, } func (s *SlackTester) WaitForMessagePostedWithFileUpload(userID, channelID string, assertFn FileUploadAssertion) error { + defer s.restoreMsgTsIfNeeded() var fetchedMessages []slack.Message var lastErr error err := wait.PollUntilContextTimeout(context.Background(), pollInterval, s.cfg.MessageWaitTimeout, false, func(ctx context.Context) (done bool, err error) { - historyRes, err := s.cli.GetConversationHistory(&slack.GetConversationHistoryParameters{ - ChannelID: channelID, Limit: 1, - }) + fetchedMessages, err := s.getMessages(channelID, 2) // Fetching 2 messages, where the first one is the file itself and the second is the filter input. if err != nil { lastErr = err return false, nil } - fetchedMessages = historyRes.Messages - for _, msg := range historyRes.Messages { + for _, msg := range fetchedMessages { if msg.User != userID { continue } @@ -347,7 +373,7 @@ func (s *SlackTester) WaitForMessagePostedWithFileUpload(userID, channelID strin return false, nil }) if lastErr == nil { - lastErr = errors.New("message assertion function returned false") + lastErr = fmt.Errorf("message assertion function returned false; fetched messages: %s", structDumper.Sdump(fetchedMessages)) } if err != nil { if wait.Interrupted(err) { @@ -360,6 +386,7 @@ func (s *SlackTester) WaitForMessagePostedWithFileUpload(userID, channelID strin } func (s *SlackTester) WaitForMessagePostedWithAttachment(userID, channelID string, limitMessages int, assertFn ExpAttachmentInput) error { + defer s.restoreMsgTsIfNeeded() renderer := bot.NewSlackRenderer() var expTime time.Time @@ -513,8 +540,10 @@ func (s *SlackTester) CreateChannel(t *testing.T, prefix string) (Channel, func( err = s.cli.ArchiveConversation(channel.ID) assert.NoError(t, err) } + out := &SlackChannel{Channel: channel} - return &SlackChannel{channel}, cleanupFn + s.channelsByName[out.Name()] = out + return out, cleanupFn } func (s *SlackConfig) BotUsername() string { @@ -550,6 +579,25 @@ func (s *SlackTester) trimAttachmentTimestamp(in string) (string, string) { return msgParts[0], ts } +// OnChannel removes expectation that messages should be posted in the thread of recently sent message. +// After first assertion it restores expectation. +func (s *SlackTester) OnChannel() BotDriver { + old := make(map[string]string) + maps.Copy(old, s.recentlyPostedMsgTS) + s.recentlyPostedMsgTS = map[string]string{} + s.restoreRecentlyPostedMsgTS = func() { + s.recentlyPostedMsgTS = old + } + return s +} + +func (s *SlackTester) restoreMsgTsIfNeeded() { + if s.restoreRecentlyPostedMsgTS != nil { + s.restoreRecentlyPostedMsgTS() + } + s.restoreRecentlyPostedMsgTS = nil +} + var emojiSlackMapping = map[string]string{ "🟢": ":large_green_circle:", "💡": ":bulb:", diff --git a/test/commplatform/teams_tester.go b/test/commplatform/teams_tester.go index ab165ac6b..59fa9f822 100644 --- a/test/commplatform/teams_tester.go +++ b/test/commplatform/teams_tester.go @@ -395,6 +395,11 @@ func (s *TeamsTester) AssertEquals(expectedMsg string) MessageAssertion { } } +// OnChannel assertion is the default mode for Teams, no action needed. +func (s *TeamsTester) OnChannel() BotDriver { + return s +} + // NormalizeTeamsWhitespacesInMessages normalizes messages, as the Teams renderer uses different line breaks in order to make the message // more readable. It's hard to come up with a single message that matches all our communication platforms so // this makes sure that we're normalizing the message to a single line break. diff --git a/test/e2e/bots_test.go b/test/e2e/bots_test.go index 20b2c1895..1ac88ba1d 100644 --- a/test/e2e/bots_test.go +++ b/test/e2e/bots_test.go @@ -566,6 +566,10 @@ func runBotTest(t *testing.T, // Same expected message as before err = botDriver.WaitForLastMessageContains(userId, botDriver.FirstChannel().ID(), expMessage) + if err != nil && botDriver.Type() == commplatform.SlackBot { // the new cloud backend not release yet + t.Logf("Fallback to the old behavior with message sent at the channel level...") + err = botDriver.OnChannel().WaitForLastMessageContains(userId, botDriver.FirstChannel().ID(), expMessage) + } assert.NoError(t, err) }) }) @@ -657,7 +661,7 @@ func runBotTest(t *testing.T, assertionFn := func(msg string) (bool, int, string) { return hasValidHeader(command, msg), 0, "" } - err = botDriver.WaitForMessagePosted(botDriver.BotUserID(), botDriver.FirstChannel().ID(), 1, assertionFn) + err = botDriver.OnChannel().WaitForMessagePosted(botDriver.BotUserID(), botDriver.FirstChannel().ID(), 1, assertionFn) }) t.Run("Get forbidden resource", func(t *testing.T) { @@ -796,6 +800,13 @@ func runBotTest(t *testing.T, }) var firstCMUpdate commplatform.ExpAttachmentInput + limitMessages := func() int { + if botDriver.Type().IsCloud() { + return limitLastMessageAfterCloudReload + } + return 2 + } + t.Run("Multi-channel notifications", func(t *testing.T) { t.Log("Getting notifier status from second channel...") command := "status notifications" @@ -819,11 +830,7 @@ func runBotTest(t *testing.T, botDriver.PostMessageToBot(t, botDriver.SecondChannel().Identifier(), command) - limitMessages := 1 - if botDriver.Type().IsCloud() { - limitMessages = limitLastMessageAfterCloudReload - } - err = botDriver.WaitForMessagePosted(botDriver.BotUserID(), botDriver.SecondChannel().ID(), limitMessages, botDriver.AssertEquals(expectedMessage)) + err = botDriver.WaitForMessagePosted(botDriver.BotUserID(), botDriver.SecondChannel().ID(), limitMessages(), botDriver.AssertEquals(expectedMessage)) require.NoError(t, err) cfgMapCli := k8sCli.CoreV1().ConfigMaps(appCfg.Deployment.Namespace) @@ -865,20 +872,13 @@ func runBotTest(t *testing.T, }, }, } - limitMessages = 2 - if botDriver.Type().IsCloud() { - limitMessages = limitLastMessageAfterCloudReload - } - err = botDriver.WaitForMessagePostedWithAttachment(botDriver.BotUserID(), botDriver.FirstChannel().ID(), limitMessages, expAttachmentIn) + err = botDriver.OnChannel().WaitForMessagePostedWithAttachment(botDriver.BotUserID(), botDriver.FirstChannel().ID(), limitMessages(), expAttachmentIn) require.NoError(t, err) t.Log("Ensuring bot didn't post anything new in second channel...") time.Sleep(appCfg.Slack.MessageWaitTimeout) - limitMessages = 2 - if botDriver.Type().IsCloud() { - limitMessages = limitLastMessageAfterCloudReload - } - err = botDriver.WaitForMessagePosted(botDriver.BotUserID(), botDriver.SecondChannel().ID(), limitMessages, botDriver.AssertEquals(expectedMessage)) + + err = botDriver.WaitForMessagePosted(botDriver.BotUserID(), botDriver.SecondChannel().ID(), limitMessages(), botDriver.AssertEquals(expectedMessage)) require.NoError(t, err) t.Log("Updating ConfigMap for not watched field...") @@ -890,17 +890,11 @@ func runBotTest(t *testing.T, t.Log("Ensuring bot didn't post anything new...") time.Sleep(appCfg.Slack.MessageWaitTimeout) - limitMessages = 2 - if botDriver.Type().IsCloud() { - limitMessages = limitLastMessageAfterCloudReload - } - err = botDriver.WaitForMessagePostedWithAttachment(botDriver.BotUserID(), botDriver.FirstChannel().ID(), limitMessages, expAttachmentIn) + + err = botDriver.OnChannel().WaitForMessagePostedWithAttachment(botDriver.BotUserID(), botDriver.FirstChannel().ID(), limitMessages(), expAttachmentIn) require.NoError(t, err) - limitMessages = 2 - if botDriver.Type().IsCloud() { - limitMessages = limitLastMessageAfterCloudReload - } - err = botDriver.WaitForMessagePosted(botDriver.BotUserID(), botDriver.SecondChannel().ID(), limitMessages, botDriver.AssertEquals(expectedMessage)) + + err = botDriver.WaitForMessagePosted(botDriver.BotUserID(), botDriver.SecondChannel().ID(), limitMessages(), botDriver.AssertEquals(expectedMessage)) require.NoError(t, err) t.Log("Updating ConfigMap for observed field...") @@ -915,7 +909,7 @@ func runBotTest(t *testing.T, // Third (RBAC) channel is isolated from this channelIDs := []string{channels[deployEnvChannelIDName].ID(), channels[deployEnvSecondaryChannelIDName].ID()} for _, channelID := range channelIDs { - err = botDriver.WaitForMessagePostedWithAttachment(botDriver.BotUserID(), channelID, 2, commplatform.ExpAttachmentInput{ + err = botDriver.OnChannel().WaitForMessagePostedWithAttachment(botDriver.BotUserID(), channelID, 2, commplatform.ExpAttachmentInput{ AllowedTimestampDelta: time.Minute, Message: api.Message{ Type: api.NonInteractiveSingleSection, @@ -944,12 +938,11 @@ func runBotTest(t *testing.T, expectedMessage = fmt.Sprintf("%s\n%s", cmdHeader(command), expectedBody) botDriver.PostMessageToBot(t, botDriver.FirstChannel().Identifier(), command) - limitMessages = 1 + if botDriver.Type().IsCloud() { waitForRestart(t, botDriver, botDriver.BotUserID(), botDriver.FirstChannel().ID(), appCfg.ClusterName) - limitMessages = limitLastMessageAfterCloudReload } - err = botDriver.WaitForMessagePosted(botDriver.BotUserID(), botDriver.FirstChannel().ID(), limitMessages, botDriver.AssertEquals(expectedMessage)) + err = botDriver.WaitForMessagePosted(botDriver.BotUserID(), botDriver.FirstChannel().ID(), limitMessages(), botDriver.AssertEquals(expectedMessage)) assert.NoError(t, err) t.Log("Getting notifier status from second channel...") @@ -981,12 +974,8 @@ func runBotTest(t *testing.T, t.Log("Ensuring bot didn't post anything new on first channel...") time.Sleep(appCfg.Slack.MessageWaitTimeout) // Same expected message as before - limitMessages = 1 - if botDriver.Type().IsCloud() { - limitMessages = limitLastMessageAfterCloudReload - } - err = botDriver.WaitForMessagePosted(botDriver.BotUserID(), botDriver.FirstChannel().ID(), limitMessages, botDriver.AssertEquals(expectedMessage)) - require.NoError(t, err) + err = botDriver.WaitForLastMessageEqual(botDriver.BotUserID(), botDriver.FirstChannel().ID(), expectedMessage) + assert.NoError(t, err) secondCMUpdate := commplatform.ExpAttachmentInput{ AllowedTimestampDelta: time.Minute, @@ -1009,7 +998,8 @@ func runBotTest(t *testing.T, }, } t.Log("Expecting bot message in second channel...") - err = botDriver.WaitForMessagePostedWithAttachment(botDriver.BotUserID(), botDriver.SecondChannel().ID(), 2, secondCMUpdate) + err = botDriver.OnChannel().WaitForMessagePostedWithAttachment(botDriver.BotUserID(), botDriver.SecondChannel().ID(), 2, secondCMUpdate) + assert.NoError(t, err) t.Log("Starting notifier in first channel") command = "enable notifications" @@ -1017,12 +1007,8 @@ func runBotTest(t *testing.T, expectedMessage = fmt.Sprintf("%s\n%s", cmdHeader(command), expectedBody) botDriver.PostMessageToBot(t, botDriver.FirstChannel().Identifier(), command) - limitMessages = 1 - if botDriver.Type().IsCloud() { - limitMessages = limitLastMessageAfterCloudReload - } - err = botDriver.WaitForMessagePosted(botDriver.BotUserID(), botDriver.FirstChannel().ID(), limitMessages, botDriver.AssertEquals(expectedMessage)) - require.NoError(t, err) + err = botDriver.WaitForMessagePosted(botDriver.BotUserID(), botDriver.FirstChannel().ID(), limitMessages(), botDriver.AssertEquals(expectedMessage)) + assert.NoError(t, err) if botDriver.Type().IsCloud() { waitForRestart(t, botDriver, botDriver.BotUserID(), botDriver.FirstChannel().ID(), appCfg.ClusterName) @@ -1045,11 +1031,7 @@ func runBotTest(t *testing.T, t.Log("Ensuring bot didn't post anything new...") time.Sleep(appCfg.Slack.MessageWaitTimeout) - limitMessages = 1 - if botDriver.Type().IsCloud() { - limitMessages = limitLastMessageAfterCloudReload - } - err = botDriver.WaitForMessagePosted(botDriver.BotUserID(), botDriver.FirstChannel().ID(), limitMessages, botDriver.AssertEquals(expectedMessage)) + err = botDriver.WaitForMessagePosted(botDriver.BotUserID(), botDriver.FirstChannel().ID(), limitMessages(), botDriver.AssertEquals(expectedMessage)) require.NoError(t, err) t.Log("Deleting ConfigMap") @@ -1079,17 +1061,18 @@ func runBotTest(t *testing.T, }, } t.Log("Expecting bot message on first channel...") - err = botDriver.WaitForMessagePostedWithAttachment(botDriver.BotUserID(), botDriver.FirstChannel().ID(), 2, firstCMUpdate) + err = botDriver.OnChannel().WaitForMessagePostedWithAttachment(botDriver.BotUserID(), botDriver.FirstChannel().ID(), 2, firstCMUpdate) require.NoError(t, err) t.Log("Ensuring bot didn't post anything new in second channel...") time.Sleep(appCfg.Slack.MessageWaitTimeout) - limitMessages = 2 + + limitMessagesNo := 2 if botDriver.Type().IsCloud() { // There are 2 config reload requested after second cm update - limitMessages = 2 * limitLastMessageAfterCloudReload + limitMessagesNo = 2 * limitLastMessageAfterCloudReload } - err = botDriver.WaitForMessagePostedWithAttachment(botDriver.BotUserID(), botDriver.SecondChannel().ID(), limitMessages, secondCMUpdate) + err = botDriver.OnChannel().WaitForMessagePostedWithAttachment(botDriver.BotUserID(), botDriver.SecondChannel().ID(), limitMessagesNo, secondCMUpdate) require.NoError(t, err) }) @@ -1115,11 +1098,7 @@ func runBotTest(t *testing.T, t.Cleanup(func() { cleanupCreatedPod(t, podDefaultNSCli, podIgnored.Name) }) time.Sleep(appCfg.Slack.MessageWaitTimeout) - limitMessages := 1 - if botDriver.Type().IsCloud() { - limitMessages = 5 - } - err = botDriver.WaitForMessagePostedWithAttachment(botDriver.BotUserID(), botDriver.FirstChannel().ID(), limitMessages, firstCMUpdate) + err = botDriver.OnChannel().WaitForMessagePostedWithAttachment(botDriver.BotUserID(), botDriver.FirstChannel().ID(), limitMessages(), firstCMUpdate) require.NoError(t, err) t.Log("Creating Pod...") @@ -1146,7 +1125,7 @@ func runBotTest(t *testing.T, // - message with recommendations from 'k8s-events' // - massage with pod create event from 'k8s-pod-create-events' // - message with kc execution via 'get-created-resource' automation - err = botDriver.WaitForMessagePostedWithAttachment(botDriver.BotUserID(), botDriver.FirstChannel().ID(), 3, commplatform.ExpAttachmentInput{ + err = botDriver.OnChannel().WaitForMessagePostedWithAttachment(botDriver.BotUserID(), botDriver.FirstChannel().ID(), 3, commplatform.ExpAttachmentInput{ AllowedTimestampDelta: time.Minute, Message: api.Message{ Type: api.NonInteractiveSingleSection, @@ -1204,7 +1183,7 @@ func runBotTest(t *testing.T, strings.Count(msg, pod.Name) == podNameCount, 0, "" } - err = botDriver.WaitForMessagePosted(botDriver.BotUserID(), botDriver.FirstChannel().ID(), 2, automationAssertionFn) + err = botDriver.OnChannel().WaitForMessagePosted(botDriver.BotUserID(), botDriver.FirstChannel().ID(), 2, automationAssertionFn) require.NoError(t, err) t.Log("Creating Service...") @@ -1234,7 +1213,7 @@ func runBotTest(t *testing.T, t.Log("Ensuring bot didn't post anything new on first channel...") time.Sleep(appCfg.Slack.MessageWaitTimeout) // same expected message as before - err = botDriver.WaitForMessagePosted(botDriver.BotUserID(), botDriver.FirstChannel().ID(), 2, automationAssertionFn) + err = botDriver.OnChannel().WaitForMessagePosted(botDriver.BotUserID(), botDriver.FirstChannel().ID(), 2, automationAssertionFn) require.NoError(t, err) t.Log("Ensuring bot automation was executed and label created Service...") @@ -1392,7 +1371,7 @@ func runBotTest(t *testing.T, require.NoError(t, err) t.Log("Expecting bot event message...") - err = botDriver.WaitForMessagePostedWithAttachment(botDriver.BotUserID(), botDriver.FirstChannel().ID(), 2, commplatform.ExpAttachmentInput{ + err = botDriver.OnChannel().WaitForMessagePostedWithAttachment(botDriver.BotUserID(), botDriver.FirstChannel().ID(), 2, commplatform.ExpAttachmentInput{ AllowedTimestampDelta: time.Minute, Message: api.Message{ Type: api.NonInteractiveSingleSection, @@ -1522,7 +1501,7 @@ func runBotTest(t *testing.T, assertionFn := func(msg string) (bool, int, string) { return strings.Contains(msg, expectedMessage), 0, "" } - err = botDriver.WaitForMessagePosted(botDriver.BotUserID(), botDriver.FirstChannel().ID(), 3, assertionFn) + err = botDriver.OnChannel().WaitForMessagePosted(botDriver.BotUserID(), botDriver.FirstChannel().ID(), 3, assertionFn) require.NoError(t, err) err = cfgMapCli.Delete(context.Background(), testConfigMapName, metav1.DeleteOptions{}) @@ -1686,6 +1665,7 @@ func crashConfigMapSourcePlugin(t *testing.T, cfgMapCli corev1.ConfigMapInterfac func waitForRestart(t *testing.T, tester commplatform.BotDriver, userID, channel, clusterName string) { t.Log("Waiting for restart...") + originalTimeout := tester.Timeout() tester.SetTimeout(90 * time.Second) if tester.Type() == commplatform.TeamsBot { @@ -1702,7 +1682,7 @@ func waitForRestart(t *testing.T, tester commplatform.BotDriver, userID, channel } } - err := tester.WaitForMessagePosted(userID, channel, 2, assertFn) + err := tester.OnChannel().WaitForMessagePosted(userID, channel, 2, assertFn) if err != nil && tester.Type() == commplatform.TeamsBot { // TODO(https://github.com/kubeshop/botkube-cloud/issues/854): for some reason, Teams restarts are not deterministic and sometimes it doesn't happen // We should add fetching Agent logs to see why it happens. @@ -1746,7 +1726,7 @@ func waitForLastPlaintextMessageEqual(driver commplatform.BotDriver, channelID, }, }) default: - return driver.WaitForLastMessageEqual(driver.BotUserID(), channelID, expectedMsg) + return driver.OnChannel().WaitForLastMessageEqual(driver.BotUserID(), channelID, expectedMsg) } }