test(script): Assert state changes for script tests #79
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
I focused on improving the tests, but also cleaned up and organised some of the scripts to make them easier to follow.
This PR resolves #65
Approach
Not all scripts store their deployed contract addresses in public variables, so I assumed those addresses weren’t accessible. To handle address checks, I added two functions:
For each test, I followed these steps:
Each test goes through all possible branches in the script and each test case covers the deployment of one contract. I also changed the scripts so they start by loading the required env vars, if an env var is missing, the script will fail right away, before starting deployments.
The test process for each script includes:
For the calldata computation scripts, the success tests basically replicate the implementation logic, but they’re useful to confirm everything works as expected, especially if the code is refactored in the future.
To reduce repeated code, I moved common test cases into the base file so they can be reused. This makes the test files cleaner and easier to read.
This approach helped me achieve 100% code and branch coverage for all tested scripts.
ENV issues
I ran into some strange behaviour with how Foundry handles environment variables. I found a closed GitHub issue explaining this problem. The issue is that if env vars are set in both setUp and the test cases, Foundry doesn’t reload them before every test, even though setUp runs before each test. For example, if one test clears an env var set in setUp, the next test might see it as empty. Since Foundry runs tests in non-deterministic order, this causes unexpected results.
To fix this, I created a separate function that can be used to set env vars at the start of each test case. This ensures that the right env vars are always set before the tests run.
Findings
There is a lot of repeated code in the scripts, as shown by the number of reused test cases. This duplication could be reduced by either:
I also noticed that some deployment scripts use a hardcoded salt value of 0. If this wasn’t intentional, it might need to be addressed. (
DeployEmailRecoveryModule
,DeploySafeRecoveryWithAccountHiding
,DeployUniversalEmailRecoveryModule
)