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

feat: Slack message to ci channel tagging owners on flakes. #12284

Merged
merged 15 commits into from
Feb 26, 2025

Conversation

charlielye
Copy link
Contributor

@charlielye charlielye commented Feb 26, 2025

  • .test_skip_patterns is now .test_patterns.yml and assigns owners to tests.
  • If in CI and a matching test fails, it doesnt fail the build but slacks the log to the owner.
  • Some additional denoise header metadata.
  • We still "fail fast" when doing a test run locally.

@charlielye charlielye requested a review from ludamad February 26, 2025 19:15
@@ -133,7 +133,7 @@ function test_cmds {
# This is a test as it runs over our test programs (format is usually considered a build step).
echo "$test_hash noir/bootstrap.sh format --check"
# We need to include these as they will go out of date otherwise and externals use these examples.
local example_test_hash=$(hash_str $test_hash-$(../barretenberg/cpp/bootstrap.sh hash))
local example_test_hash=$(hash_str $test_hash-$(../../barretenberg/cpp/bootstrap.sh hash))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why did this need to change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was actually silently failing i think. we cd into noir-repo.

Copy link
Collaborator

Choose a reason for hiding this comment

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

ah right

@@ -49,7 +49,7 @@ function gke {
}

function test_cmds {
echo "$hash ./spartan/bootstrap.sh test-local"
# echo "$hash ./spartan/bootstrap.sh test-local"
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is skipped, still needs to be commented out?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You said you were read to let it die, but wasn't sure. We want to keep it as a "always skip no alert"?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I was ready to have it skipped to get in, still hold out for fixing it

# Send slack message to owners.
slack_uids=""
for uid in $owners; do
slack_uids+="<@$uid> "
Copy link
Collaborator

@ludamad ludamad Feb 26, 2025

Choose a reason for hiding this comment

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

Suggested change
slack_uids+="<@$uid> "
slack_uids+=" <@$uid>"

for Palla's OCD to not do "@charlie : foo"

Copy link
Collaborator

Choose a reason for hiding this comment

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

Using ${slack_uids% } in L97 would also do the trick by removing trailing whitespace:

$ FOO='bar baz baq '
$ echo "${FOO% }: Test flaked"
bar baz baq: Test flaked

@charlielye charlielye merged commit e83fe03 into master Feb 26, 2025
7 checks passed
@charlielye charlielye deleted the cl/slack-flakes branch February 26, 2025 22:52
@@ -94,7 +94,7 @@ function flake {
read -r -d '' data <<EOF
{
"channel": "#aztec3-ci",
"text": "${slack_uids}: Test flaked \`$test_cmd\` http://ci.aztec-labs.com/$log_key"
"text": "${slack_uids% }: Test flaked \`$test_cmd\` http://ci.aztec-labs.com/$log_key"
Copy link
Collaborator

Choose a reason for hiding this comment

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

❤️

TomAFrench added a commit that referenced this pull request Feb 26, 2025
* master: (31 commits)
  feat: Slack message to ci channel tagging owners on flakes. (#12284)
  fix: slack notify was broken by quoted commit titles
  revert: "chore: Fix and reenable fees-settings test (#12302)"
  fix: run arm64 on master (#12307)
  yolo fix
  chore: Fix and reenable fees-settings test (#12302)
  feat!: rename compute_nullifier_without_context (#12308)
  chore: Lazy loading artifacts everywhere (#12285)
  chore: Reenable dapp subscription test (#12304)
  chore: Run prover test with fake proofs when requested (#12305)
  chore: Do not set CI_FULL outside CI (#12300)
  chore: new mnemonic deployments on sepolia (#12076)
  chore!: enable multiple L1 nodes to be used (#11945)
  chore: remove no longer supported extension from vscode/extension.json (#12303)
  fix(e2e): p2p_reqresp (#12297)
  feat: Sync from noir (#12298)
  chore: enabling `e2e_contract_updates` in CI + nuking irrelevant test (#12293)
  feat: prepend based merge (#12093)
  feat: fetch addresses from registry (#12000)
  feat: live logs (#12271)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants