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

test: refactor fork tests to use a shared contract #1179

Merged
merged 5 commits into from
Feb 20, 2025
Merged

test: refactor fork tests to use a shared contract #1179

merged 5 commits into from
Feb 20, 2025

Conversation

smol-ninja
Copy link
Member

@smol-ninja smol-ninja commented Feb 16, 2025 •
edited
Loading

Closes #1127

Changelog

  • Adds a new Lockup.t.sol abstract contract in tests/fork directory.
  • DRY'ifies fork tests by moving common code into the new Lockup.t.sol abstract contract.
  • Adds string err in assertEq of Lockup.Model for consistency in tests/utils/Assertions.sol
  • Adds assertEq to compare SablierLockupBase states in tests/utils/Assertions.sol
  • Fixes some streamed amount calculations in tests/utils/Calculations.sol
  • Adds 2 new functions in tests/utils/Defaults.sol
  • Fix tests/utils/EstimateMaxCount.sol to not fail in case of default profile

@smol-ninja smol-ninja force-pushed the tests/dryify-fork-code branch from a3ca581 to 6020d16 Compare February 17, 2025 15:01
@andreivladbrg
test(fork): declare initialHolderBalance as uint128
cadad06 
test(fork): rename fuzzed param to create instead of lockup
test(defaults): remove unneeded function
test(assertions): add compare function for LockupLinear.UnlockAmounts
andreivladbrg
andreivladbrg requested changes Feb 20, 2025
View reviewed changes
Copy link
Member

@andreivladbrg andreivladbrg left a comment

Choose a reason for hiding this comment

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

great work, dry-fying the fork tests was a good idea.
it is much cleaner now.

left some comments below (i've made the changes myself, will create a PR on top of it), lmk if you accept it.

@andreivladbrg andreivladbrg mentioned this pull request Feb 20, 2025
Merged
@smol-ninja smol-ninja merged commit 70d3bb3 into staging Feb 20, 2025
9 checks passed
@smol-ninja smol-ninja deleted the tests/dryify-fork-code branch February 20, 2025 15:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andreivladbrg andreivladbrg andreivladbrg requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@smol-ninja @andreivladbrg