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

Mock-builder with generic methods support #1321

Merged
merged 7 commits into from
Apr 18, 2023
Merged

Mock-builder with generic methods support #1321

merged 7 commits into from
Apr 18, 2023

Conversation

lemunozm
Copy link
Contributor

Description

This PR gives to mock-builder the capability of mocking methods with generic bounds

Fixes #1316

Changes and Descriptions

  • Added function identification with generics
  • Fully refactorized to reduce macro usage and allows better organization
  • Better error descriptions of what is going wrong.
  • Added new tests

@lemunozm lemunozm added D0-ready Pull request can be merged without special precaution and notification. P5-soon Issue should be addressed soon. labels Apr 13, 2023
@lemunozm lemunozm self-assigned this Apr 13, 2023
@lemunozm
Copy link
Contributor Author

@NunoAlexandre, @mustermeiszer @branan @wischli @thea-leake @cdamian, not sure who can review this part. Any comment/feedback/question is welcome.

🔓 There is no risk of merging it because all this code is used only for testing.

@lemunozm lemunozm force-pushed the mock-builder/generic-methods branch 2 times, most recently from 8607da3 to 5f13cf4 Compare April 14, 2023 06:47
mustermeiszer
mustermeiszer previously approved these changes Apr 18, 2023
View reviewed changes
Copy link
Collaborator

@mustermeiszer mustermeiszer left a comment

Choose a reason for hiding this comment

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

I did no deep deep dive but the changes look sane. But I also think we reached a point with this where moving to procedural macros makes sense in order to have the parsing happen during compilation.

@lemunozm lemunozm force-pushed the mock-builder/generic-methods branch from 164e8ab to acf6440 Compare April 18, 2023 08:27
@lemunozm
Copy link
Contributor Author

lemunozm commented Apr 18, 2023 •
edited
Loading

Thanks for your review!

There is still a pending issue that I would like to tackle before starting the procedural part. Once it is done, we can support any kind of trait. The current state is preferred for debugging. Roadmap

mustermeiszer
mustermeiszer previously approved these changes Apr 18, 2023
View reviewed changes
wischli
wischli previously approved these changes Apr 18, 2023
View reviewed changes
Copy link
Contributor

@wischli wischli left a comment

Choose a reason for hiding this comment

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

Some really minor nit-picks from my side. Definitely not blocking, looks good!

libs/mock-builder/src/lib.rs
/// This function should be called with a locator used as a function identification.
pub fn register<Map, L, F, I, O>(locator: L, f: F)
where
Map: StorageMap<<Blake2_128 as StorageHasher>::Output, CallId>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Just as a food for thought: In the future we should probably support other hashers apart from Blake2_128 and handle that with generics. AFAIK, Substrate supports others as well but in most cases blake is preferred.

Copy link
Contributor Author

@lemunozm lemunozm Apr 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I don't see right now a use case where a caller would want to choose another Hasher in this case. I mean, currently, the own crate is who chooses the Hasher, but I need the Map to be generic enough to support different mocks.

But thanks for your thoughts! I keep that in mind in case it evolves in a way a different Hasher can be useful.

libs/mock-builder/src/location.rs Outdated Show resolved Hide resolved
libs/mock-builder/src/location.rs Outdated Show resolved Hide resolved
libs/mock-builder/src/storage.rs Outdated
}

#[test]
#[should_panic]
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe good to check for a more specific panic?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there any way to check for a concrete panic? Like the message the panic should return?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I've refactored the code a little bit, to support that case, with better error handling 👍🏻

libs/mock-builder/tests/pallet.rs
}

#[test]
#[should_panic]
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above: I would prefer to narrow down the panic.

Copy link
Contributor Author

@lemunozm lemunozm Apr 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This panic comes from a bad macro usage, and I can not check from the test that "bad usage". Only I can expect it to panic at some point in the macro.

But this panic is ok from a use-case perspective. Tells the user they have configured their mock badly.

@lemunozm lemunozm dismissed stale reviews from wischli and mustermeiszer via 8965594 April 18, 2023 14:40
@lemunozm lemunozm requested a review from wischli April 18, 2023 14:40
@lemunozm lemunozm force-pushed the mock-builder/generic-methods branch from 5593c63 to 8965594 Compare April 18, 2023 14:51
@lemunozm lemunozm merged commit 85dd12a into main Apr 18, 2023
@lemunozm lemunozm deleted the mock-builder/generic-methods branch April 18, 2023 18:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mustermeiszer mustermeiszer mustermeiszer approved these changes

@NunoAlexandre NunoAlexandre Awaiting requested review from NunoAlexandre

@branan branan Awaiting requested review from branan

@thea-leake thea-leake Awaiting requested review from thea-leake

@cdamian cdamian Awaiting requested review from cdamian

@wischli wischli Awaiting requested review from wischli

Assignees

@lemunozm lemunozm

Labels
D0-ready Pull request can be merged without special precaution and notification. P5-soon Issue should be addressed soon.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Mock-builder: support trait methods with generic bounds
3 participants
@lemunozm @wischli @mustermeiszer