-
Notifications
You must be signed in to change notification settings - Fork 11
Implementing mocking of host functions for tests #102
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
Conversation
mvadari
left a comment
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.
If I'm understanding this approach right, you have to clear mocks at the end of each test, right? Is it possible to do that automatically upon the end of the test, perhaps via Drop?
|
@mvadari Yes currently manual clear is required, Looking into the Drop approach. Thank you |
mvadari
left a comment
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.
|
|
||
| let result = unsafe { super::super::get_nft_transfer_fee(ptr::null(), 0) }; | ||
| assert_eq!(result, 42); | ||
| } |
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.
Would you mind adding a test to check actually passing some data into the pointer (the way the host functions actually work)?
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.
Bump @tekvyy
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.
@mvadari Pushed the test, please review
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.
@mvadari let me know if anything else is needed.
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.
@mvadari Let me know if we can merge this, i will create a followup PR with tests for other modules :)
sappenin
left a comment
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.
Hi @tekvyy - thanks for submitting this PR. It's been on my TODO list for a while to review, but I hadn't had time until yesterday (apologies for that delay).
After thinking things through yesterday, I've become convinced that we should use a proper mocking library for our own unit testing in this library (rather than create our own using macros, which will likely work in the shorter term, but be more difficult to maintain in the longer term).
To that end, I created a series of 3 draft PRs as a sort of "alternative route" for mocking using a well-known mocking library (mockall).
Take a look at #117 and let me know what you think? If you're agreeable, I suggest we close this PR in-favor of #117, #118, and #119.
Hey @sappenin |
High Level Overview of Change
Context of Change
Currently, testing code that depends on host functions is difficult to test because the host bindings return hardcoded values in test mode.
This makes it impossible to test error handling paths or specific return values.
The solution:
A lightweight, macro-based mocking system that allows tests to control host function behavior without modifying production code.
The mock infrastructure:
#[cfg(test)]mode (zero runtime overhead)mock_host!macro for setupType of Change
Test Plan
Future Tasks
If this approach is approved: