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

refactor: integration tests #1223

Open
wants to merge 55 commits into
base: dev
Choose a base branch
from
Open

Conversation

0xClandestine
Copy link
Member

@0xClandestine 0xClandestine commented Mar 6, 2025

Motivation:

This PR refactors the integration test framework to improve performance, maintainability, and organization. The existing integration test structure had grown complex with multiple responsibilities mixed in different files, making it harder to understand and maintain. The goal is to create a more modular, better organized test framework with clear separation of concerns to make writing and maintaining integration tests easier.

Modifications:

  1. Created two new utility files to better organize the codebase:

    • IntegrationGetters.t.sol: Centralizes all state-fetching functionality, providing methods to get both current and previous state snapshots.
    • IntegrationUtils.t.sol: Contains helper functions for creating users, generating allocation parameters, and other common tasks.
  2. Restructured folder organization:

    • Moved deprecated interfaces from deprecatedInterfaces/ to deprecated/.
    • Relocated utility files from integration to /test/utils/ folder.
    • Moved mocks from integration/mocks/ to test/mocks/.
  3. Renamed test files to follow a more consistent naming convention:

    • e.g., ALM_RegisterAndModify.t.solDeposit_Delegate_Modify.t.sol.
  4. Refactored the TimeMachine.t.sol contract with improved functionality and moved it to utils/ folder to make it accessible beyond integration tests.

  5. Added a new Constants.t.sol file to centralize commonly used constants.

  6. Added a FOUNDRY_FUZZ_SEED to fork tests that updates weekly (this significantly reduces our RPC usage).

  7. Moved _init into setUp() (this means setup happens once per fuzz run, rather than once per test).

  8. Sort strategies array once, rather than JIT (sorting an array in place is time intensive).

  9. Refactored randomness sourcing to use Vm.

  • Previously, we were using a uint24 _random variable combined with hashing to generate subsequent random values.

  • While this pattern simplified test reruns (allowing the reuse of the same _random value for consistency), it has notable drawbacks. Randomness isn't unevenly distributed, leading to modulo bias, and the process is computationally expensive.

Additionally, the same functionality can now be easily achieved through Foundry flags.

Result:

After these changes, the integration test framework will be more modular with clear separation of concerns, making it easier to understand and maintain.

Integration CI times has decreased from 15-25mins -> 5-6mins.

NOTE: There's still quite a bit that could be done, don't want this PR getting too big.

@github-actions github-actions bot added 🧪 Test Test-related changes (unit, integration, etc.). ♻️ Refactor Code improvements without changing functionality. labels Mar 6, 2025
certora-run[bot]

This comment was marked as spam.

This comment was marked as spam.

certora-run[bot]

This comment was marked as spam.

This comment was marked as spam.

@0xClandestine 0xClandestine force-pushed the refactor/integration-tests branch 2 times, most recently from 6598f77 to b57fe21 Compare March 7, 2025 04:33

This comment was marked as spam.

This comment was marked as spam.

certora-run[bot]

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

certora-run[bot]

This comment was marked as spam.

@Layr-Labs Layr-Labs deleted a comment from github-actions bot Mar 7, 2025
@Layr-Labs Layr-Labs deleted a comment from github-actions bot Mar 7, 2025
@github-actions github-actions bot added the 📖 Documentation Improvements or additions to documentation. label Mar 7, 2025

This comment was marked as spam.

certora-run[bot]

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

certora-run[bot]

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

certora-run[bot]

This comment was marked as spam.

This comment was marked as spam.

certora-run[bot]

This comment was marked as spam.

This comment was marked as spam.

@0xClandestine 0xClandestine force-pushed the refactor/integration-tests branch from 9c6a462 to 35dc373 Compare March 7, 2025 17:29
@0xClandestine 0xClandestine force-pushed the refactor/integration-tests branch from 47b7a4c to 8f783e5 Compare March 12, 2025 15:32

// Ensure the staker has at least 64 ETH to deposit.
if (initTokenBalances[0] < 64 ether) {
initTokenBalances[0] = 64 ether;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I think this should be initTokenBalances[0] = initTokenBalances[0] + 64 ether

IAllocationManagerTypes.SlashingParams memory slashingParams;
uint wadToSlash = _randWadToSlash();
slashingParams = avs.slashOperator(operator, operatorSet.id, strategies, wadToSlash.toArrayU256());
SlashingParams memory slashingParams = _genSlashing_Half(operator, operatorSet);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think _randWadToSlash() still has a place in some of these tests - I caught issues using it that I wouldn't have found with a flat half slash

Copy link
Member Author

Choose a reason for hiding this comment

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

Plan to revamp src/utils/Random.t.sol and use it for more varied randomness. I don't think we should be slashing by half anywhere really, originally we were using _randWadToSlash everywhere. Agree it provides better coverage.

@@ -3,27 +3,16 @@ pragma solidity ^0.8.27;

import "src/test/integration/IntegrationChecks.t.sol";
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the ALM_RegisterAndModify type name is a lot less confusing than X_Y_Z. If we're going to do a filename sweep, I'd say we should move towards the former

Copy link
Member Author

Choose a reason for hiding this comment

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

Goal is to combine all the deposit, delegate scenarios into a single contract, have noticed it's pretty costly in both compile time and test time having them all seperated when they all have minor differences in setup.

Agree though, current naming is pretty bad.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📜 Chore Maintenance related changes (CI, QOL, etc.). 📖 Documentation Improvements or additions to documentation. ♻️ Refactor Code improvements without changing functionality. 📜 Script Script-related changes (automation, tooling). 🧪 Test Test-related changes (unit, integration, etc.).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants