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

Mock builder update. Improved FI coverage #1945

Merged
merged 7 commits into from
Aug 5, 2024
Merged

Conversation

lemunozm
Copy link
Contributor

@lemunozm lemunozm commented Aug 2, 2024

Description

  • Added new version of mock builder with method handlers, supporting check how many times a mock was called.
  • Fixed some wrong configured default-features (I do not know why Cargo did not complained)
  • Improved FI tests checking the times the hooks are called.
  • Cargo update Something is really off with the cargo update. Just doing cargo update on main makes the compilation of runtime fail due to std issues. I was not able to find the guilty one.

@lemunozm lemunozm added the I4-tests Test needs fixing or improving. label Aug 2, 2024
@lemunozm lemunozm requested review from cdamian and wischli August 2, 2024 06:39
@lemunozm lemunozm self-assigned this Aug 2, 2024
@lemunozm lemunozm requested a review from mustermeiszer as a code owner August 2, 2024 06:39
cfg-mocks = { workspace = true, default_features = true }
frame-system = { workspace = true, default_features = true }
mock-builder = { workspace = true, default_features = true }
cfg-mocks = { workspace = true, default-features = true }
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I just noticed the difference here. This is scary lol

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And no warnings at all by cargo 😅

cdamian
cdamian previously approved these changes Aug 2, 2024
Copy link
Contributor

@cdamian cdamian left a comment

Choose a reason for hiding this comment

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

Awesome improvement, thank you @lemunozm!

@lemunozm
Copy link
Contributor Author

lemunozm commented Aug 2, 2024

I've needed to also update all inner dependencies to get rid of a cargo bug 🫤

cdamian
cdamian previously approved these changes Aug 2, 2024
Comment on lines 30 to 33
+ 'static,
) {
register_call!(move |(a, b, c, d, e)| f(a, b, c, d, e));
) -> CallHandler {
register_call!(move |(a, b, c, d, e)| f(a, b, c, d, e))
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Basically, if you leave the ; then the behavior is the same as before. If you remove the ;, then the CallHandler is returned, and you have the new capabilities

@lemunozm lemunozm force-pushed the mock-builder-handler branch from 89d4a92 to f858bbd Compare August 5, 2024 10:03
Copy link

codecov bot commented Aug 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 47.73%. Comparing base (fc2b451) to head (f858bbd).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1945   +/-   ##
=======================================
  Coverage   47.72%   47.73%           
=======================================
  Files         184      184           
  Lines       12969    12969           
=======================================
+ Hits         6190     6191    +1     
+ Misses       6779     6778    -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@lemunozm lemunozm enabled auto-merge (squash) August 5, 2024 11:47
@lemunozm
Copy link
Contributor Author

lemunozm commented Aug 5, 2024

I've touched some file that requires also the approval of any of you @wischli @hieronx 🙏🏻

@lemunozm
Copy link
Contributor Author

lemunozm commented Aug 5, 2024

Could you force @hieronx ? It seems like we also need the Frederik approval

@hieronx hieronx disabled auto-merge August 5, 2024 14:23
@hieronx hieronx merged commit 8245ce2 into main Aug 5, 2024
12 of 13 checks passed
@lemunozm lemunozm deleted the mock-builder-handler branch August 5, 2024 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I4-tests Test needs fixing or improving.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants