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

op-e2e: Support specifying allocs in tests #12216

Merged
merged 4 commits into from
Oct 1, 2024

Conversation

mslipper
Copy link
Collaborator

@mslipper mslipper commented Sep 30, 2024

Overview

Previously, the E2E test suite read a hardcoded set of allocs from the .devnet directory. There could only be one logical set of allocs per test suite run. To get around this limitation, we added matrix jobs to the CI pipeline that swapped in different versions of the allocs for alt-DA, MT Cannon, fault proofs, and the L2OO. This was very inefficient and complex: most tests don't need to run against multiple versions of the allocs, and were running 4 separate build jobs when only one was needed.

This PR makes it possible for an individual test to request a specific allocs file. We can now run tests for multiple different configurations of the OP Stack - e.g. alt-DA, L2OO and fault proofs - in a single test run.

To make this work, I updated the test suite's initialization method to read alloc files from multiple suffixed .devnet- directories. I made a a new make devnet-allocs-tests task to generate them. The allocs are then added to a map, and the rest of the tests use getter methods in the config package to locate the appropriate configuration structs.

Reviewing this PR

This PR seems large, but most of the modified files contain limited changes to comply with the new API for selecting test configuration based on alloc type. For example, an allocType configuration parameter was added to various system config structs and the DefaultRollupTestParams variable was made into a method so that it always returns a copy for easy extension. The important logic happens in the following files:

  • Makefile
  • .circleci/config.yml
  • op-e2e/config/init.go

As part of this PR, I also cleaned up a few issues:

  • I removed the external OP Geth shim. It wasn't used anywhere, and was introducing a lot of complexity for little gain.
  • I refactored some tests to use top-level test methods rather than subtests so that they could be more easily parallelized.
  • I removed references to UseFaultProofs, UseAltDA, and other environment-based test flags from test utilities. We shouldn't be reading from the environment in test utils. Instead, we should pass in the behavior we want to the test utils themselves.

Copy link
Contributor

semgrep-app bot commented Oct 1, 2024

Semgrep found 1 sol-style-input-arg-fmt finding:

  • packages/contracts-bedrock/src/libraries/Encoding.sol

Inputs to functions must be prepended with an underscore (_)

Ignore this finding from sol-style-input-arg-fmt.

Previously, the E2E test suite read a hardcoded set of allocs from the `.devnet` directory. There could only be one logical set of allocs per test suite run. To get around this limitation, we added matrix jobs to the CI pipeline that swapped in different versions of the allocs for alt-DA, MT Cannon, fault proofs, and the L2OO. This was very inefficient and complex: most tests don't need to run against multiple versions of the allocs, and were running 4 separate build jobs when only one was needed.

This PR makes it possible for an individual test to request a specific allocs file. We can now run tests for multiple different configurations of the OP Stack - e.g. alt-DA, L2OO and fault proofs - in a single test run.

To make this work, I updated the test suite's initialization method to read alloc files from multiple suffixed `.devnet-` directories. I made a a new `make devnet-allocs-tests` task to generate them. The allocs are then added to a map, and the rest of the tests use getter methods in the `config` package to locate the appropriate configuration structs.

This PR seems large, but most of the modified files contain limited changes to comply with the new API for selecting test configuration based on alloc type. For example, an `allocType` configuration parameter was added to various system config structs and the `DefaultRollupTestParams` variable was made into a method so that it always returns a copy for easy extension. The important logic happens in the following files:

- Makefile
- .circleci/config.yml
- op-e2e/config/init.go

As part of this PR, I also cleaned up a few issues:

- I removed the external OP Geth shim. It wasn't used anywhere, and was introducing a lot of complexity for little gain.
- I refactored some tests to use top-level test methods rather than subtests so that they could be more easily parallelized.
- I removed references to `UseFaultProofs`, `UseAltDA`, and other environment-based test flags from test utilities. We shouldn't be reading from the environment in test utils. Instead, we should pass in the behavior we want to the test utils themselves.
@mslipper mslipper force-pushed the feat/multiple-allocs-per-test branch from aa8c819 to ff132bc Compare October 1, 2024 01:36
@mslipper mslipper marked this pull request as ready for review October 1, 2024 01:37
@mslipper mslipper requested review from a team as code owners October 1, 2024 01:37
Copy link

codecov bot commented Oct 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.97%. Comparing base (31e244c) to head (d9d2de9).
Report is 3 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #12216      +/-   ##
===========================================
- Coverage    75.19%   74.97%   -0.22%     
===========================================
  Files           49       49              
  Lines         3665     3665              
===========================================
- Hits          2756     2748       -8     
- Misses         736      745       +9     
+ Partials       173      172       -1     
Flag Coverage Δ
cannon-go-tests 74.97% <ø> (-0.22%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 1 file with indirect coverage changes

Copy link
Contributor

@ajsutton ajsutton left a comment

Choose a reason for hiding this comment

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

LGTM. Seems like a big improvement in flexibility that we can keep building on to support completely custom configs at some point without having to pre-generate them.

I'm not entirely sure what the mt-cannon tests are doing - they seem to still have some special treatment. Given we now have multicannon which will select the right version of cannon to run automatically based on the prestate, we should be able to just have tests that actually run cannon setup to run their tests with both Standard and MtCannon allocs and have it just work.

I also put up #12221 which reads the AllocType from the System when configuring the challenger so it doesn't need to be pulled from the env which maybe helps with part of that too.

.circleci/config.yml Outdated Show resolved Hide resolved
op-e2e/actions/batcher/l2_batcher_test.go Show resolved Hide resolved
op-e2e/actions/helpers/l2_proposer.go Outdated Show resolved Hide resolved
op-e2e/actions/helpers/l2_proposer.go Outdated Show resolved Hide resolved
op-e2e/actions/helpers/l2_proposer.go Outdated Show resolved Hide resolved
op-e2e/system/helpers/withdrawal_helper.go Show resolved Hide resolved
op-e2e/system/helpers/withdrawal_helper.go Outdated Show resolved Hide resolved
op-e2e/system/helpers/withdrawal_helper.go Outdated Show resolved Hide resolved
op-e2e/system/helpers/withdrawal_helper.go Outdated Show resolved Hide resolved
op-e2e/system/helpers/withdrawal_helper.go Outdated Show resolved Hide resolved
@ajsutton
Copy link
Contributor

ajsutton commented Oct 1, 2024

Confirmed that the MT cannon tests can just run as a standard alloc type with #12221. No need for special env vars. 🎉
We can then setup the cannon tests to run both variants pretty easily. They are our most expensive tests though so may want to think carefully about which tests we actually want to test both variants on - often they're testing the dispute game and challenger's ability to run cannon more than cannon itself.

@mslipper mslipper force-pushed the feat/multiple-allocs-per-test branch from 962ba73 to 16537d9 Compare October 1, 2024 15:18
@mslipper mslipper added this pull request to the merge queue Oct 1, 2024
Merged via the queue into develop with commit c2dc0ab Oct 1, 2024
62 checks passed
@mslipper mslipper deleted the feat/multiple-allocs-per-test branch October 1, 2024 16:50
samlaf pushed a commit to samlaf/optimism that referenced this pull request Nov 10, 2024
* op-e2e: Support specifying allocs in tests

Previously, the E2E test suite read a hardcoded set of allocs from the `.devnet` directory. There could only be one logical set of allocs per test suite run. To get around this limitation, we added matrix jobs to the CI pipeline that swapped in different versions of the allocs for alt-DA, MT Cannon, fault proofs, and the L2OO. This was very inefficient and complex: most tests don't need to run against multiple versions of the allocs, and were running 4 separate build jobs when only one was needed.

This PR makes it possible for an individual test to request a specific allocs file. We can now run tests for multiple different configurations of the OP Stack - e.g. alt-DA, L2OO and fault proofs - in a single test run.

To make this work, I updated the test suite's initialization method to read alloc files from multiple suffixed `.devnet-` directories. I made a a new `make devnet-allocs-tests` task to generate them. The allocs are then added to a map, and the rest of the tests use getter methods in the `config` package to locate the appropriate configuration structs.

This PR seems large, but most of the modified files contain limited changes to comply with the new API for selecting test configuration based on alloc type. For example, an `allocType` configuration parameter was added to various system config structs and the `DefaultRollupTestParams` variable was made into a method so that it always returns a copy for easy extension. The important logic happens in the following files:

- Makefile
- .circleci/config.yml
- op-e2e/config/init.go

As part of this PR, I also cleaned up a few issues:

- I removed the external OP Geth shim. It wasn't used anywhere, and was introducing a lot of complexity for little gain.
- I refactored some tests to use top-level test methods rather than subtests so that they could be more easily parallelized.
- I removed references to `UseFaultProofs`, `UseAltDA`, and other environment-based test flags from test utilities. We shouldn't be reading from the environment in test utils. Instead, we should pass in the behavior we want to the test utils themselves.

* code review updates

* fix gastoken test

* fixes
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.

2 participants