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

feat: factory skeleton #66

Merged
merged 26 commits into from
Jul 1, 2024
Merged

feat: factory skeleton #66

merged 26 commits into from
Jul 1, 2024

Conversation

petrovska-petro
Copy link
Collaborator

No description provided.

Copy link

codecov bot commented Jun 24, 2024

Codecov Report

Attention: Patch coverage is 95.91837% with 2 lines in your changes missing coverage. Please review.

Project coverage is 97.97%. Comparing base (cc32c04) to head (21ec122).
Report is 1 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main      #66       +/-   ##
===========================================
+ Coverage   87.27%   97.97%   +10.70%     
===========================================
  Files           3        2        -1     
  Lines         220      148       -72     
  Branches       23       27        +4     
===========================================
- Hits          192      145       -47     
+ Misses         27        1       -26     
- Partials        1        2        +1     
Files Coverage Δ
src/RoboSaverVirtualModule.sol 99.19% <100.00%> (+10.11%) ⬆️
src/RoboSaverVirtualModuleFactory.sol 91.66% <91.66%> (ø)

... and 2 files with indirect coverage changes

Base automatically changed from feat/cl-migration to main June 24, 2024 11:06
@petrovska-petro
Copy link
Collaborator Author

wanted to check this prev, but it seems that in the test suite even if there were tests which failed still all checks are labelled as sucessful instead of blocking the merging? 🤔

@gosuto-inzasheru
Copy link
Contributor

wanted to check this prev, but it seems that in the test suite even if there were tests which failed still all checks are labelled as sucessful instead of blocking the merging? 🤔

hmm you are right, this is due to foundry-rs/foundry#3900

easy fix would be to run it twice; forge t && forge coverage

i will push a commit

Copy link
Contributor

@gosuto-inzasheru gosuto-inzasheru left a comment

Choose a reason for hiding this comment

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

also note heavy negative change in coverage

src/RoboSaverVirtualModule.sol Outdated Show resolved Hide resolved
src/RoboSaverVirtualModule.sol Outdated Show resolved Hide resolved
src/RoboSaverVirtualModule.sol Outdated Show resolved Hide resolved
test/BaseFixture.sol Outdated Show resolved Hide resolved
src/RoboSaverVirtualModuleFactory.sol Outdated Show resolved Hide resolved
src/RoboSaverVirtualModuleFactory.sol Outdated Show resolved Hide resolved
src/RoboSaverVirtualModuleFactory.sol Outdated Show resolved Hide resolved
src/RoboSaverVirtualModuleFactory.sol Show resolved Hide resolved
@gosuto-inzasheru
Copy link
Contributor

Co-authored-by: gosuto.eth <gosuto@inzasheru.email>
@petrovska-petro
Copy link
Collaborator Author

that's great news ! great to see that increase of coverage, it can allow us to raise our standards of the MIN before merging

@petrovska-petro petrovska-petro marked this pull request as ready for review June 28, 2024 08:20
@gosuto-inzasheru
Copy link
Contributor

i pushed a bit hacky way to ignore the scripts from showing up in the codecov report

better will be the merging of this branch: foundry-rs/foundry#7301

@gosuto-inzasheru
Copy link
Contributor

hmm i guess we want to ignore test/ as well for the coverage report..?

@petrovska-petro
Copy link
Collaborator Author

hmm i guess we want to ignore test/ as well for the coverage report..?

yes, makes sense to exclude test/ & script/ completely

@petrovska-petro petrovska-petro merged commit 0033d27 into main Jul 1, 2024
2 checks passed
@petrovska-petro petrovska-petro deleted the feat/factory branch July 1, 2024 09:16
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