-
Notifications
You must be signed in to change notification settings - Fork 110
ci(l1): fail L1 job if hive tests fail #4827
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
Conversation
afc11b1
to
6c2a849
Compare
Lines of code reportTotal lines added: Detailed view
|
c4a5f99
to
9dae1ce
Compare
@@ -0,0 +1,192 @@ | |||
#!/usr/bin/env bash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This script searches for the log files of failed tests and updates them to an artifact
- name: "Rpc Compat tests" | ||
simulation: ethereum/rpc-compat | ||
limit: "" | ||
limit: "rpc-compat/(debug_[^/]+/.*|eth_blobBaseFee/.*|eth_blockNumber/.*|eth_call/.*|eth_chainId/.*|eth_createAccessList/.*|eth_estimateGas/.*|eth_feeHistory/.*|eth_getBalance/.*|eth_getBlockByHash/.*|eth_getBlockByNumber/.*|eth_getBlockReceipts/.*|eth_getBlockTransactionCountByHash/.*|eth_getBlockTransactionCountByNumber/.*|eth_getCode/.*|eth_getLogs/.*|eth_getProof/.*|eth_getStorageAt/.*|eth_getTransactionByBlockHashAndIndex/.*|eth_getTransactionByBlockNumberAndIndex/.*|eth_getTransactionByHash/.*|eth_getTransactionCount/.*|eth_getTransactionReceipt/.*|eth_sendRawTransaction/.*)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
eth-syncing is now failing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a comment mentioning this?
simulation: devp2p | ||
limit: discv4|eth|snap/Ping|Findnode/WithoutEndpointProof|Findnode/PastExpiration|Amplification|Status|StorageRanges|ByteCodes|GetBlockHeaders|SimultaneousRequests|SameRequestID|ZeroRequestID|GetBlockBodies|MaliciousHandshake|MaliciousStatus|Transaction|NewPooledTxs|GetBlockReceipts|LargeTxRequest|InvalidTxs|BlockRangeUpdate | ||
# AccountRange and GetTrieNodes don't pass anymore. | ||
limit: discv4|eth|snap/Ping|Findnode/WithoutEndpointProof|Findnode/PastExpiration|Amplification|Status|StorageRanges|ByteCodes|GetBlockHeaders|SimultaneousRequests|SameRequestID|ZeroRequestID|GetBlockBodies|MaliciousHandshake|MaliciousStatus|NewPooledTxs|GetBlockReceipts|BlockRangeUpdate|GetTrieNodes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there were some regressions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR modifies the CI workflow to properly fail L1 jobs when Hive tests fail, addressing an issue where the workflow would continue even when tests failed due to the use of ethpandaops/hive-github-action
.
- Adds a custom script to parse Hive JSON results and fail the workflow when tests fail
- Updates Hive versions and consolidates duplicate sync tests
- Adds artifact upload for failed test logs with GitHub summary integration
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
File | Description |
---|---|
.github/workflows/pr-main_l1.yaml |
Updates workflow to check Hive results, upload failure logs, and consolidates test configurations |
.github/scripts/check-hive-results.sh |
New script that parses Hive JSON results, identifies failures, and creates detailed failure reports |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
- name: Run unit tests | ||
run: | | ||
make test | ||
Copilot
AI
Oct 14, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The conditional logic skipping EF tests for merge groups lacks documentation. Consider adding a comment explaining why EF tests are skipped for merge group events.
# EF tests are skipped for merge group events to reduce CI load and avoid redundant test runs, | |
# since merge group events are used for speculative merges and do not require full EF test coverage. |
Copilot uses AI. Check for mistakes.
suite_slug="$(printf '%s' "${suite_slug_raw}" | tr '[:upper:]' '[:lower:]')" | ||
suite_slug="$(printf '%s' "${suite_slug}" | sed -E 's/[^a-z0-9._-]+/-/g')" | ||
suite_slug="${suite_slug#-}" | ||
suite_slug="${suite_slug%-}" |
Copilot
AI
Oct 14, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Multiple separate calls to external commands for string manipulation could be combined into a single operation or use bash parameter expansion for better performance.
suite_slug="$(printf '%s' "${suite_slug_raw}" | tr '[:upper:]' '[:lower:]')" | |
suite_slug="$(printf '%s' "${suite_slug}" | sed -E 's/[^a-z0-9._-]+/-/g')" | |
suite_slug="${suite_slug#-}" | |
suite_slug="${suite_slug%-}" | |
suite_slug="${suite_slug_raw,,}" # lowercase | |
suite_slug="${suite_slug//[^a-z0-9._-]/-}" # replace non-matching chars with '-' | |
suite_slug="${suite_slug#-}" # trim leading dash | |
suite_slug="${suite_slug%-}" # trim trailing dash |
Copilot uses AI. Check for mistakes.
|
||
for search_root in "${search_roots[@]}"; do | ||
[ -d "${search_root}" ] || continue | ||
found_log="$(find "${search_root}" -type f -name "$(basename "${log_rel}")" -print -quit 2>/dev/null || true)" |
Copilot
AI
Oct 14, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The find command with basename could be inefficient for large directory trees. Consider using more specific search patterns or limiting search depth if the log structure is known.
found_log="$(find "${search_root}" -type f -name "$(basename "${log_rel}")" -print -quit 2>/dev/null || true)" | |
found_log="$(find "${search_root}" -maxdepth 2 -type f -name "$(basename "${log_rel}")" -print -quit 2>/dev/null || true)" |
Copilot uses AI. Check for mistakes.
hive_repository: ethereum/hive | ||
hive_version: 2b7a9c007770b10cb1a242a6c6de88c87a383e5a | ||
artifact_prefix: engine_cancun | ||
# - name: "Cancun Engine tests" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
regression
hive_repository: ethereum/hive | ||
hive_version: 2b7a9c007770b10cb1a242a6c6de88c87a383e5a | ||
artifact_prefix: sync_snap | ||
# Investigate this test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
regression
Motivation
When a Hive job failed, the L1 workflow didn't fail. This is because we started using
ethpandaops/hive-github-action
Description
Here is an example of a run with failing hive tests:
https://github.com/lambdaclass/ethrex/actions/runs/18411165213?pr=4827