Skip to content
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

Send Slack notification for UI/Unit test failures #9853

Merged
merged 2 commits into from
Jun 2, 2023

Conversation

jostnes
Copy link
Contributor

@jostnes jostnes commented Jun 1, 2023

Description

This PR updates the version of a8c-ci-toolkit plugin to the latest v2.17.0 to get the recent change where a Slack notification will be sent when there's a test failure. This change will be for both Unit and UI test failures.

I updated the 3 .yml files under .buildkite to point to the latest version. I'm not sure if that's the right way but please me know if this shouldn't be the case.

Testing instructions

This was previously tested using this draft PR, when there's a failing test, a notification is sent to the channel set in annotate_test_failures. When there are no test failures, the notification is not sent.

@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Jun 1, 2023

You can test the changes from this Pull Request by:
  • Clicking here or scanning the QR code below to access App Center
  • Then installing the build number pr9853-06c13dd on your iPhone

If you need access to App Center, please ask a maintainer to add you.

@jostnes jostnes added the category: tooling Anything that involves building & maintaining the project, including scripts, `Fastfile`, etc. label Jun 1, 2023
@jostnes jostnes added this to the 14.0 milestone Jun 1, 2023
@jostnes jostnes marked this pull request as ready for review June 1, 2023 08:38
@jostnes jostnes requested review from a team June 1, 2023 08:38
Copy link
Contributor

@AliSoftware AliSoftware left a comment

Choose a reason for hiding this comment

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

I updated the 3 .yml files under .buildkite to point to the latest version. I'm not sure if that's the right way but please me know if this shouldn't be the case.

Yep that's the best way to go 👍 🙇

  • Purely technically speaking, for this to work you only needed to update the .yml file that would call commands that used the new annotate_test_failures, and since pipeline.yml is the only one calling run-ui-tests.sh and run-unit-tests.sh, this was the only one that required to use the latest plugin version.
  • Practically speaking, it's way nicer and better practice to try to use the same version of the plugin on all our pipelines, for consistency (and to avoid confusing issues where CI commands would work in a pipeline but not another and it could take us quite a long time to realize that it was because of plugin versions being different). So thank you for doing that (as I see that the versions were not consistent before your PR already)!
  • As a side note just FYI, we hope that at some point in the future we could define that value in a single place and have all .yml files refer to the same source of truth for the plugin version… which would solve the risk of inconsistent versions between the .yml files. But we're not there yet 😛 so in the meantime, manualy updating the version consistently in all .yml files is the way to go 🙂

Comment on lines 40 to 41
if [ "$BUILDKITE_BRANCH" = "trunk" ]; then
annotate_test_failures "fastlane/test_output/WooCommerce.xml" --slack "build-and-ship"
Copy link
Contributor

Choose a reason for hiding this comment

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

Open question: I wonder if you wouldn't want to get Slack notifications for any release/* branch too? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a good question. Thinking about faster feedback, it makes sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

while the original scope was only to turn this on for trunk, yeah i agree it's probably worth extending this to include release/* branches too, especially since we saw at least one release build recently that started failing because of tests 😬

i updated the condition to include release/* in 06c13dd and tested that using this draft PR (used test/ instead of release/ to test the condition).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: tooling Anything that involves building & maintaining the project, including scripts, `Fastfile`, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants