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

[CI/CD] Fix coverage triggering bug. #12742

Merged
merged 1 commit into from
Mar 29, 2024
Merged

[CI/CD] Fix coverage triggering bug. #12742

merged 1 commit into from
Mar 29, 2024

Conversation

JoshLind
Copy link
Contributor

@JoshLind JoshLind commented Mar 29, 2024

Description

This PR fixes a bug in the coverage.yaml GHA. The workflow should only trigger if the label CICD:run-coverage is added to the PR, or on a schedule. But, the GHA is actually being triggered by PRs that add any label. For example, see the runs here.

This seems to do with the way in which GHA is evaluating expressions in the if conditions of the workflows.

Testing Plan

Manual verification. For example, this PR skips the coverage workflow despite having a label.

Copy link

trunk-io bot commented Mar 29, 2024

⏱️ 6h 19m total CI duration on this PR
Job Cumulative Duration Recent Runs
rust-unit-tests 1h 8m 🟩 (+3 more)
rust-images / rust-all 53m 🟩 (+2 more)
rust-lints 36m 🟩 (+3 more)
run-tests-main-branch 35m 🟩🟩🟩🟩 (+3 more)
rust-smoke-tests 34m 🟩
execution-performance / single-node-performance 21m 🟩
windows-build 19m 🟩
check-dynamic-deps 18m 🟩🟩🟩🟩🟩 (+4 more)
forge-e2e-test / forge 16m 🟩
general-lints 16m 🟩🟩🟩🟩 (+3 more)
rust-smoke-coverage 13m
rust-unit-coverage 12m
forge-compat-test / forge 12m 🟩
cli-e2e-tests / run-cli-tests 7m 🟩
check 5m 🟩
semgrep/ci 4m 🟩🟩🟩🟩🟩 (+4 more)
file_change_determinator 2m 🟩🟩🟩🟩🟩 (+4 more)
file_change_determinator 2m 🟩🟩🟩🟩🟩 (+4 more)
file_change_determinator 2m 🟩🟩🟩🟩 (+4 more)
file_change_determinator 2m 🟩🟩🟩🟩 (+3 more)
node-api-compatibility-tests / node-api-compatibility-tests 49s 🟩
permission-check 42s 🟩🟩🟩🟩🟩 (+4 more)
permission-check 34s 🟩🟩🟩🟩🟩 (+4 more)
rust-move-tests 29s 🟩🟩🟩🟩🟩 (+4 more)
permission-check 24s 🟩🟩🟩🟩🟩 (+4 more)
permission-check 23s 🟩🟩🟩🟩🟩 (+4 more)
determine-docker-build-metadata 21s 🟩🟩🟩🟩 (+3 more)
permission-check 19s 🟩🟩🟩🟩🟩 (+3 more)
execution-performance / file_change_determinator 11s 🟩
upload-to-codecov 9s 🟥

🚨 1 job on the last run was significantly faster/slower than expected

Job Duration vs 7d avg Delta
rust-lints 5m 7m -29%

settingsfeedbackdocs ⋅ learn more about trunk.io

@JoshLind JoshLind added the CICD:build-images when this label is present github actions will start build+push rust images from the PR. label Mar 29, 2024
@JoshLind JoshLind added CICD:build-images when this label is present github actions will start build+push rust images from the PR. and removed CICD:build-images when this label is present github actions will start build+push rust images from the PR. labels Mar 29, 2024
@JoshLind JoshLind removed the CICD:build-images when this label is present github actions will start build+push rust images from the PR. label Mar 29, 2024
@JoshLind JoshLind changed the title [Coverage] Fix scheduling bug. [CI/CD] Fix coverage triggering bug. Mar 29, 2024
@JoshLind JoshLind marked this pull request as ready for review March 29, 2024 20:14
@JoshLind JoshLind requested review from a team as code owners March 29, 2024 20:14
@JoshLind JoshLind added CICD:run-coverage run tests with test coverage instrumentation CICD:build-images when this label is present github actions will start build+push rust images from the PR. and removed CICD:run-coverage run tests with test coverage instrumentation labels Mar 29, 2024
@JoshLind JoshLind enabled auto-merge (rebase) March 29, 2024 20:26

This comment has been minimized.

This comment has been minimized.

Copy link
Contributor

✅ Forge suite compat success on aptos-node-v1.9.5 ==> 93dc6713550d615cf351c71da6e8221a2abf9c44

Compatibility test results for aptos-node-v1.9.5 ==> 93dc6713550d615cf351c71da6e8221a2abf9c44 (PR)
1. Check liveness of validators at old version: aptos-node-v1.9.5
compatibility::simple-validator-upgrade::liveness-check : committed: 6278 txn/s, latency: 5169 ms, (p50: 4800 ms, p90: 8700 ms, p99: 15400 ms), latency samples: 226020
2. Upgrading first Validator to new version: 93dc6713550d615cf351c71da6e8221a2abf9c44
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 1016 txn/s, latency: 27295 ms, (p50: 32300 ms, p90: 37700 ms, p99: 41300 ms), latency samples: 56920
3. Upgrading rest of first batch to new version: 93dc6713550d615cf351c71da6e8221a2abf9c44
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 281 txn/s, submitted: 623 txn/s, expired: 341 txn/s, latency: 31386 ms, (p50: 35500 ms, p90: 52100 ms, p99: 57600 ms), latency samples: 21700
4. upgrading second batch to new version: 93dc6713550d615cf351c71da6e8221a2abf9c44
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 1988 txn/s, latency: 15003 ms, (p50: 15700 ms, p90: 20100 ms, p99: 20500 ms), latency samples: 99440
5. check swarm health
Compatibility test for aptos-node-v1.9.5 ==> 93dc6713550d615cf351c71da6e8221a2abf9c44 passed
Test Ok

Copy link
Contributor

✅ Forge suite realistic_env_max_load success on 93dc6713550d615cf351c71da6e8221a2abf9c44

two traffics test: inner traffic : committed: 8295 txn/s, latency: 4739 ms, (p50: 4500 ms, p90: 5600 ms, p99: 10200 ms), latency samples: 3575500
two traffics test : committed: 100 txn/s, latency: 1893 ms, (p50: 1800 ms, p90: 2000 ms, p99: 7400 ms), latency samples: 1700
Latency breakdown for phase 0: ["QsBatchToPos: max: 0.206, avg: 0.203", "QsPosToProposal: max: 0.257, avg: 0.235", "ConsensusProposalToOrdered: max: 0.458, avg: 0.396", "ConsensusOrderedToCommit: max: 0.325, avg: 0.309", "ConsensusProposalToCommit: max: 0.716, avg: 0.705"]
Max round gap was 1 [limit 4] at version 1670359. Max no progress secs was 4.846879 [limit 15] at version 1670359.
Test Ok

@JoshLind JoshLind merged commit 7325fb8 into main Mar 29, 2024
70 of 82 checks passed
@JoshLind JoshLind deleted the coverage_bug branch March 29, 2024 21:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CICD:build-images when this label is present github actions will start build+push rust images from the PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants