Send Slack notification for Test Failures

[jostnes]

Description

This PR updates annotate_test_failures with the ability to send Slack notification when there are test failures. It is an optional feature and is used by passing a second argument when calling it from project repos (I just realized via this PR when updating to latest trunk that Tumblr also uses this and they might not want the Slack notification to be sent which was the reason I added the second argument).

The changes made for this to happen:

1. Update update_annotation to return a JSON object when a failure happen
2. Create send_slack_notification to read the JSON object, parse and form the message payload, and send a POST call to the Slack Webhook. For now, it's set specifically to #build-and-ship channel

In this change, the message displays the repo (to know which app) and step name (to know if it's Unit, UI iPhone or UI iPad, etc), the number of failing test cases, and the names + test classes of failing tests.

Some notes:

1. Failure notification is only sent for merges to trunk (failures on other branches will not as we are trying to get an idea how noisy this will be for trunk first)
2. When there are no failures, a notification is not sent (no notification for flaky tests)
3. If the app crashes before the JUnit XML file is created, a notification is not sent (similar to how annotation on Buildkite is not displayed when this happens currently)

Testing

Testing PR: woocommerce/woocommerce-ios#9798

1. When there are multiple failures (build):

Slack: p1685000034603039-slack-C03Q9FTCQA0

2. When there is 1 failure (build):

Slack: p1685001564964369-slack-C03Q9FTCQA0

3. When all test pass (with flaky tests) or one without flaky test here:
   

4. When failure not on trunk branch:
   

5. When should_send_slack_notification is "false":
   

---

• I have considered if this change warrants release notes and have added them to the appropriate section in the CHANGELOG.md if necessary.

[AliSoftware]

@jostnes FYI, given #58 was recently merged and also updated annotate_test_failures script, you might want to update your branch to make sure your changes fit nicely on top of those (I don't forsee any conflict, but just to make things clean and avoid any surprise) 🙃

[AliSoftware]

My main concern is that your current implementation return from the update_annotation method before reaching system('buildkite-agent', 'annotate', …), so that will break the existing behavior. Another concern is the code crashing if SLACK_WEBHOOK env var is not defined (which will likely be the case for many pipelines other than Tumblr-Android and WCiOS).
Hence why the "Request changes" review.

The rest of the comments are mostly suggestions to make the code much nicer (less convoluted and hopefully easier to read) and more flexible. ✨ 😉

[jostnes]

thanks for the review @AliSoftware ! i made the suggested changes and tested with this CI build and locally, it's ready for another review.

though i did notice one thing - the main branches name are different on Tumblr repos. i made this update f89ed06 to also check if the build is on develop or master. wdyt?

[jostnes]

Removed the branch condition and tested using two WCiOS testing builds:

1. Slack notification sent
2. Slack notification not sent

[AliSoftware]

Left some final nitpick touches/suggestions, otherwise this looks really nice and ready now 🎉

I'll approve it already to unblock, and trust you to properly apply the final suggestions and test them one last time before merging if you like them, without needing another approval round from me by now 🙂

[AliSoftware]

Instead of using .to_s to convert the array into its debug representation (that will contain the brackets), I'd suggest you to use .join(',') to join the items of the array into a nice-looking list.

You could even decide to join the list of failing tests using .join("\n") instead if you prefer them to be rendered each on their own line, In which case I'd advocate to also add - in front of each entry to make it look like a bullet-point list in Slack 🙂 And while we're at prettifying all this, you can also use backticks around the test and class name too 🙃

failing_tests = list.map { |item| " - `#{item.name}` in `#{item.classname}`" }

   "text": "*Failing #{test_text}:*\n#{failing_tests.join("\n")}"

[jostnes]

thanks for the re-review!

agreed and made all the changes in 32ef08f except for displaying them as a bullet point list. found that Slack message, when sent by the block API, only covers half the screen (looks like a known issue) and because some test names can be pretty long, for those, the message doesn't look that great (screenshot below from a local test, which is why the default label "Tests" is displayed):

[AliSoftware]

Nitpick: no need to use string interpolation to interpolate… a variable that is already the whole string you need 😃

Suggested change

	uri = URI("#{slack_webhook}")
	uri = URI(slack_webhook)

[AliSoftware]

The whole first part of that URL is already also exposed by Buildkite as BUILDKITE_BUILD_URL env var, so you might as well use this to make this more readable 😉

Suggested change

	"url": "https://buildkite.com/#{ENV['BUILDKITE_ORGANIZATION_SLUG']}/#{ENV['BUILDKITE_PIPELINE_SLUG']}/builds/#{ENV['BUILDKITE_BUILD_NUMBER']}##{ENV['BUILDKITE_JOB_ID']}",
	"url": "#{ENV['BUILDKITE_BUILD_URL']}##{ENV['BUILDKITE_JOB_ID']}",

[AliSoftware]

Suggested change

	Annotates the Buildkite build with a summary of failed and flaky tests based on a JUnit report file.
	Annotates the Buildkite build with a summary of failed and flaky tests based on a JUnit report file (defaults to using `build/results/report.junit`).

[AliSoftware]

Suggested change

	# annotate_test_failures [options] [junit-report-file-path]
	# annotate_test_failures [junit-report-file-path] [--slack CHANNEL] [--slack-webhook URL]

[jostnes]

Merging this, the latest changes were tested using this WCiOS build (notification sent, notification not sent) and works as expected.