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.

Sign up for GitHub

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

Jump to bottom

Add assertoor tests to CI #6192

Open
wants to merge 19 commits into
base: unstable
Choose a base branch
from
Open

Add assertoor tests to CI #6192

wants to merge 19 commits into from
+37 −6

Conversation

eserilev
Copy link
Collaborator

@eserilev eserilev commented Jul 26, 2024 •
edited
Loading

Add assertoor tests to CI

This PR adds a new step to the local_testnet CI. This new step spins up a kurtosis testnet (defined in assertoor_network_params.yml) and runs assertoor tests as a github action via kurtosis-assertoor-github-action

Issues w/ assertoor github action

Note that the repository ppa:rmescandon/yq is no longer maintained and does not contain the most up to date version of yq

mikefarah/yq#1308

Installation recommendations for yq can be found here:
https://github.com/mikefarah/yq?tab=readme-ov-file#install

I've updated the local_testnet.yml script to install yq via snap

Choosing which tests to run

The list of assertoor tests can be found here: https://github.com/ethpandaops/assertoor-test/tree/master/assertoor-tests

Theres a few tests that are broken at the momemnt (mev-block-proposal as an example) and a few that take 48 hours to complete. I've picked the most relevant tests that finish running at a reasonable time.

Theres some electra tests that we may want to add in the future

@eserilev eserilev added ready-for-review The code is ready for review test improvement Improve tests infra-ci labels Jul 26, 2024
@eserilev eserilev changed the title Add asertoor tests in CI Add assertoor tests in CI Jul 26, 2024
@eserilev eserilev changed the title Add assertoor tests in CI Add assertoor tests to CI Jul 26, 2024
@eserilev eserilev force-pushed the add-assertoor-github-action branch from 01ced96 to c6b10ce Compare July 31, 2024 18:55
@eserilev eserilev force-pushed the add-assertoor-github-action branch from c6b10ce to 766dbd0 Compare July 31, 2024 19:18
@realbigsean
Copy link
Member

I don't think we need opcodes tests, those sound like they'd be EVM specific

big-calldata-tx-test might be useful for us as it effects blocks size

stability-check and synchronized-check also sound interesting but not sure what they are

@eserilev eserilev added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels Aug 5, 2024
@eserilev
Copy link
Collaborator Author

eserilev commented Oct 16, 2024 •
edited
Loading

the mev-block-proposal-check test fails for both lighthouse and prysm across multiple EL combos. not sure whats up with that, I've left that test out for now

@eserilev eserilev added the ready-for-review The code is ready for review label Oct 18, 2024
@eserilev eserilev removed the waiting-on-author The reviewer has suggested changes and awaits thier implementation. label Oct 18, 2024
@dapplion
Copy link
Collaborator

Can you upload the logs to the CI run artifacts? To debug failures

@eserilev
Copy link
Collaborator Author

logs should now be uploaded with a 3 day retention window

jimmygchen
jimmygchen reviewed Oct 22, 2024
View reviewed changes
.github/workflows/local-testnet.yml
Comment on lines +76 to +78
path: |
scripts/local_testnet/logs
retention-days: 3
Copy link
Member

@jimmygchen jimmygchen Oct 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Looks like these keys are invalid for this github action. (see warnings on the run)
The step below defines the artifact path and retention, I think this can be removed?

Copy link
Member

Choose a reason for hiding this comment

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

We already upload the local testnet logs, did you mean to upload the assertoor logs? They could be included in the enclave logs too but i haven't checked

jimmygchen
jimmygchen reviewed Oct 22, 2024
View reviewed changes
scripts/tests/assertoor_network_params.yaml
run_block_proposal_check: true
run_blob_transaction_test: true
tests:
- https://raw.githubusercontent.com/ethpandaops/assertoor-test/master/assertoor-tests/big-calldata-tx-test.yaml
Copy link
Member

Choose a reason for hiding this comment

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

Looks like the big-calldata-tx-test.yaml is the one taking the most time (>20m) and doesn't run concurrently with the other 3 tests. Maybe we can exclude this one from the CI job and have it as a recurring test on GitHub action?

@jimmygchen jimmygchen added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels Oct 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jimmygchen jimmygchen jimmygchen left review comments

Assignees
No one assigned
Labels
infra-ci test improvement Improve tests waiting-on-author The reviewer has suggested changes and awaits thier implementation.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@eserilev @realbigsean @dapplion @jimmygchen