-
Notifications
You must be signed in to change notification settings - Fork 353
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
Improve mocks #1146
Improve mocks #1146
Conversation
…eleased-package-#1141
…emove-dual-dispatchers
…eat/remove-mocks-from-released-package-#1141
* feat: remove modules * fix: mock * fix: linter * fix: tests * fix: mock * feat: apply review suggestions
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1146 +/- ##
=======================================
Coverage 91.51% 91.51%
=======================================
Files 46 46
Lines 1178 1178
=======================================
Hits 1078 1078
Misses 100 100
Continue to review full report in Codecov by Sentry.
|
…om-released-package-#1141
…eat/remove-mocks-from-released-package-#1141
…eat/remove-mocks-from-released-package-#1141
…eat/remove-mocks-from-released-package-#1141
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing work, Eric! This for sure improves maintainability. I left some comments—really, only one actionable thing
LGTM!
scarb 2.8.4 (2aa4e193e 2024-10-07) | ||
cairo: 2.8.4 (https://crates.io/crates/cairo-lang-compiler/2.8.4) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❤️
[[test]] | ||
name = "openzeppelin_account_unittest" | ||
build-external-contracts = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the name a requirement? Not that I'm against it if it's not, I'm just curious
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is, because of a known issue with foundry and scarb.
mod mocks; | ||
|
||
#[cfg(test)] | ||
mod test_account; | ||
#[cfg(test)] | ||
mod test_erc1155; | ||
#[cfg(test)] | ||
mod test_erc20; | ||
#[cfg(test)] | ||
mod test_erc721; | ||
#[cfg(test)] | ||
mod test_eth_account; | ||
#[cfg(test)] | ||
mod test_universal_deployer; | ||
#[cfg(test)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Soooo much better 😄
Fixes #1141
Depends on #1163
This PR centralizes mocks into the
test_common
package to avoid repetition and make them easier to maintain.