Skip to content

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

Merged
charlielye merged 15 commits intomasterfrom
cl/slack-flakes
Feb 26, 2025
Merged

feat: Slack message to ci channel tagging owners on flakes.#12284
charlielye merged 15 commits intomasterfrom
cl/slack-flakes

Conversation

@charlielye
Copy link
Copy Markdown
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
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

ah right


function test_cmds {
echo "$hash ./spartan/bootstrap.sh test-local"
# echo "$hash ./spartan/bootstrap.sh test-local"
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

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
{
"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
Copy Markdown
Contributor

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