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

test: create e2e smoke test flow + use cluster for SP1 proofs #405

Merged
merged 42 commits into from
Mar 19, 2025

Conversation

gjermundgaraba
Copy link
Contributor

@gjermundgaraba gjermundgaraba commented Mar 13, 2025

Description

closes: #402


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Linked to GitHub issue with discussion and accepted design, OR link to spec that describes this work.
  • Wrote unit and integration tests.
  • Added relevant natspec and godoc comments.
  • Provide a conventional commit message to follow the repository standards.
  • Re-reviewed Files changed in the GitHub PR explorer.
  • Review SonarCloud Report in the comment section below once CI passes.

@gjermundgaraba gjermundgaraba requested a review from srdtrk as a code owner March 13, 2025 20:22
Copy link

codecov bot commented Mar 13, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.86%. Comparing base (de19ea5) to head (139da65).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #405   +/-   ##
=======================================
  Coverage   99.86%   99.86%           
=======================================
  Files          15       15           
  Lines         744      744           
=======================================
  Hits          743      743           
  Misses          1        1           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@srdtrk
Copy link
Member

srdtrk commented Mar 14, 2025

Why do we have a separate smoke test ci pipeline? What's the use case?

@gjermundgaraba
Copy link
Contributor Author

The idea is to have a smaller set of tests that always run in POS mode and that can also be run with the mainnet preset. This set can run a small set of POS tests on PRs (maybe only when changes are made to test files or the light client?) and every week run a couple of tests with the mainnet preset.

I did consider putting this into the e2e workflow itself, but it seemed unnecessarily complex to try to make this fit into the existing workflow.

@srdtrk
Copy link
Member

srdtrk commented Mar 14, 2025

@gjermundgaraba I think we should do a full decoupling so that we have 3 sets of separate CI:

  • quick-e2e.yaml: POW and mock
  • minimal-e2e.yaml: POS and minimal
  • full-e2e.yaml: POS and mainnet

These run all the same tests. And we can optimize in which frequency these run later. Maybe initially:

  • every PR
  • every 1-3 days
  • every 5-7 days

We can also perhaps add randomness as well in the future. The reason why I think this will be better is because even one full-e2e is enough to have the CI take a longer time. Since the runtime of the CI is the duration of the longest test.

@gjermundgaraba gjermundgaraba marked this pull request as draft March 15, 2025 16:52
Copy link
Contributor Author

@gjermundgaraba gjermundgaraba left a comment

Choose a reason for hiding this comment

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

This got a bit bigger than expected, but got in a good cleanup here, I think!

@@ -0,0 +1,21 @@
name: 'E2E Test Matrix Generator'
description: 'Dynamically discovers and generates a test matrix for e2e tests'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To avoid having to specify every test every time.
Instead there is now a skip-list, which will be much shorter and easier to maintain

Copy link
Member

Choose a reason for hiding this comment

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

Tbh I actually do like listing. The amount of code added to create the list far exceeds the size of the list by several times, and is significantly harder to read. Likely contains weird edge cases. Why would it be bad to have them listed only in one file such as this one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mostly because it's hard to know if any are missing and why. It's easy to miss if a test was not added to the list, and it seems like an unnecessary maintenance burden to keep around.

Copy link
Member

Choose a reason for hiding this comment

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

Now we will never know if a test is missing. I think we can make a practice to add every test we write to the CI lisr, and just have the ones we don't use commented out with a comment explaining why. I don't trust these scripts to not miss some tests eventually. Idk, wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can do that of course, but I'd rather have less things to worry about than more - hence the want to reduce the maintenance burden. It's not a big deal, but I do prefer this method which we also use in ibc-go (although there it's a go script). Would you feel more comfortable if it was a go script instead of a bash script?

One possibility is to take the go script from ibc-go, pull it out into a separate repo, clean it up and publish it as an action we can use in both repos (and maybe others could use too)?

@gjermundgaraba gjermundgaraba marked this pull request as ready for review March 16, 2025 23:08
Copy link
Member

@srdtrk srdtrk left a comment

Choose a reason for hiding this comment

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

Did my initial pass

@@ -0,0 +1,21 @@
name: 'E2E Test Matrix Generator'
description: 'Dynamically discovers and generates a test matrix for e2e tests'
Copy link
Member

Choose a reason for hiding this comment

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

Tbh I actually do like listing. The amount of code added to create the list far exceeds the size of the list by several times, and is significantly harder to read. Likely contains weird edge cases. Why would it be bad to have them listed only in one file such as this one?

id: get-matrix
uses: ./.github/actions/e2e-test-matrix
with:
skip-tests: 'TestWithRelayerTestSuite/Test_2_ConcurrentRecvPacketToEth_Groth16,TestWithRelayerTestSuite/TestMultiPeriodClientUpdateToCosmos'
Copy link
Member

Choose a reason for hiding this comment

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

Can these be listed properly rather than be comma delimited? Like:

Suggested change
skip-tests: 'TestWithRelayerTestSuite/Test_2_ConcurrentRecvPacketToEth_Groth16,TestWithRelayerTestSuite/TestMultiPeriodClientUpdateToCosmos'
skip-tests:
- 'TestWithRelayerTestSuite/Test_2_ConcurrentRecvPacketToEth_Groth16'
- 'TestWithRelayerTestSuite/TestMultiPeriodClientUpdateToCosmos'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really. There is no list type for input, so it would have to be either comma separated, or like a json array, but again, as a string '["test1", "test2"]'

Copy link
Member

Choose a reason for hiding this comment

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

Oof

Copy link
Member

Choose a reason for hiding this comment

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

I think we can keep this for now since you don't think the bash script will live forever. When we move this logic to go fully, perhaps we should run all tests, and some tests just skip themselves based on environment variables or other runtime factors. This would also allow us to implement a 50% run chance per test very easily

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Why is this in a separate directory called e2e-test-matrix/? Each action is to have its own dir? If you want to separate these into dirs, just use .github/actions/e2e/ imo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just how github actions are structured. There is one folder per action with a file called action.yml in it.

Copy link
Member

Choose a reason for hiding this comment

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

I see. I really don't like how this turned out. I almost rather prefer we keep what we have now (e2e.yml) and rename it to e2e-minimal.yml (which supports both pow mock and minimal pos like today). And just add a new e2e-full.yml with full code duplication from e2e-minimal.yml but only it runs on pos + mainnet. (And not on every PR). Wdyt?

I'm fine with you moving the setup action though. But this is super confusing now as we have 3 directories and two of them have different types of setup

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd rather not duplicate this. Would naming this differently help? Like install-e2e-deps? It seems like this should be solvable with naming. Got any ideas?

Copy link
Member

Choose a reason for hiding this comment

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

Directory should probably be called e2e-setup/ so that we have the following directories at the end:

  • foundry-setup/
  • e2e-setup/
  • e2e-matrix/

Copy link
Member

Choose a reason for hiding this comment

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

Seems shorter to just list the tests we want.

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's not about shorter, it's about getting to not have to worry about listing tests and maintaining that. We could easily end up in a situation where one is forgotten at some point and never gets into the CI pipeline.

Copy link
Member

@srdtrk srdtrk Mar 17, 2025

Choose a reason for hiding this comment

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

I just don't wanna maintain this script (or even understand it). It is easy to understand a list of tests.
Then you still need to list the test you don't want to be running. I think we shouldn't introduce bash unless it can be inlined in yaml files.

I'm willing to accept this for now though since I don't think listing is a better way, it just looks much cleaner. The CI is starting to look closer to ibc-go's, which is not necessarily good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another option for the tests we don't want to run is to either not have those, or have the skip part of them in code (that they skip on certain conditions, like CI or whatever).

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I want to have skip part in code based on env variables. However, I understand that this might be tough to implement in this PR. I am ok with merging this and addressing it in another PR

@gjermundgaraba gjermundgaraba requested a review from srdtrk March 18, 2025 22:21
@gjermundgaraba
Copy link
Contributor Author

Added type safety to the sp1 config!

Copy link
Member

@srdtrk srdtrk left a comment

Choose a reason for hiding this comment

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

Looking very good. I just have some style suggestions left, we can merge this once we agree on those style decisions

Copy link
Member

Choose a reason for hiding this comment

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

The new config looks really good. You should update the example config as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

runs-on: depot-ubuntu-22.04
steps:
- uses: actions/checkout@v4
- name: "Set up E2E environment"
Copy link
Member

Choose a reason for hiding this comment

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

Some name use quotations and some names don't. I think we should just not use quotations but I'm ok with either for names as long as we are consistent. Ditto everywhere else.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll go through and clean up any remaining ones

id: get-matrix
uses: ./.github/actions/e2e-test-matrix
with:
skip-tests: 'TestWithRelayerTestSuite/Test_2_ConcurrentRecvPacketToEth_Groth16,TestWithRelayerTestSuite/TestMultiPeriodClientUpdateToCosmos'
Copy link
Member

Choose a reason for hiding this comment

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

I think we can keep this for now since you don't think the bash script will live forever. When we move this logic to go fully, perhaps we should run all tests, and some tests just skip themselves based on environment variables or other runtime factors. This would also allow us to implement a 50% run chance per test very easily

Copy link
Member

Choose a reason for hiding this comment

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

I think let's just do e2e-minimal.yml, and we can all the quick one e2e-mock.yml. I like them to be two words. Wdyt:

  • e2e-full.yml
  • e2e-minimal.yml
  • e2e-mock.yml

Copy link
Member

Choose a reason for hiding this comment

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

Directory should probably be called e2e-setup/ so that we have the following directories at the end:

  • foundry-setup/
  • e2e-setup/
  • e2e-matrix/

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I want to have skip part in code based on env variables. However, I understand that this might be tough to implement in this PR. I am ok with merging this and addressing it in another PR

Copy link
Member

@srdtrk srdtrk left a comment

Choose a reason for hiding this comment

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

Lgtm! 🚀

@gjermundgaraba gjermundgaraba merged commit e981ff2 into main Mar 19, 2025
64 checks passed
@gjermundgaraba gjermundgaraba deleted the gjermund/e2e-test-improvements branch March 19, 2025 17:44
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.

Use a private sp1 cluster in the POS CI
2 participants