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

The ABI and attr properties of the mocked functions have been changed. Associated with issiue 501. #504

Merged

Conversation

Enes1313
Copy link
Contributor

@Enes1313 Enes1313 commented Jul 23, 2023

I was encountering an issue with mocking a ffi variadic function. I checked with cargo expand and i saw the reason behind this issue was that the mock function was lacking the extern "C". I checked mockall source code and i saw the ABI of function signatures was not set in the condition of "Item::ForeignMod" in the "impl From for MockItemModule" code block in mock_item.rs file. These functions were under "extern "C" and I thought the mocks of these functions should be like this so I made the change.

My primary goal was to write unit tests in Rust for C source code, which required the mock functions to be compatible with C. For this reason, I added the no_mangle attribute. However, it's possible that a few adjustments are needed in the code, as I am not very familiar with Rust (I'm new :) ).

r please @asomers

@Enes1313 Enes1313 requested a review from asomers as a code owner July 23, 2023 20:51
@asomers
Copy link
Owner

asomers commented Jul 23, 2023

Using a Rust ABI for the mock function is deliberate; since mocked functions are replaced at compile-time, there's no need for them to use the C ABI. But I never considered that some users might want to export Mockall-generated code to C. That's an interesting use-case. I think this should probably work. But I want to review it in more detail when I get back from vacation. Feel free to ping me if I don't review the PR by 31-July.

@Enes1313
Copy link
Contributor Author

Thank you for your comment. I didn't know it was intentional to use Rust ABI by default. When I set it to be C ABI (related changes) everything worked fine :).

For functions inside "extern", I could make the use of the relevant ABI optional. But it could be confusing for me. So I kept the change simple. Waiting for your detailed review.

Wishing you a good vacation.

My use case for others to read

build.rs ->
a.c includes a.h and b.h.
With bindgen, a.rs binding is created from a.h.
With bindgen, mock_b.rs binding is created from b.h and automock is added for functions (all in one extern "C").
a.c is compiled and the link settings are made.

and under src, the unit test of module "a" is performed.

This use case needs extern "C" and no_mangle to work. There is no need to do anything in mockall for variadic functions. It is sufficient to specify the c_variadic attribute in lib.rs only.

@Enes1313
Copy link
Contributor Author

@asomers :)

Copy link
Owner

@asomers asomers left a comment

Choose a reason for hiding this comment

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

This is really cool! It's somewhat amazing that the change is so simple. I'd just like a few small changes:

  • Only emit the #[no_mangle] attribute for the mock method. As-is, this PR also emits it for the XXX_context() method as well as some places where it isn't even valid, like the hidden module and even some {} code blocks.
  • Add a CHANGELOG entry for this new feature.
  • We need some test coverage to ensure that this doesn't regress. Including a C file in the Mockall crate just to test this sounds difficult. But casting the function to a C function pointer is almost as good. Adding a test case like this to automock_foreign_c.rs should do it. I haven't yet figured out a way to check that a function name isn't mangled from Rust.
/// Ensure that the mock function can be called from C by casting it to a C
/// function pointer.
#[test]
fn c_abi() {
    let ctx = mock_ffi::foo_context();
    ctx.expect().returning(i64::from);
    let p: unsafe extern "C" fn(u32) -> i64 = mock_ffi::foo;
    assert_eq!(42, unsafe{p(42)});
}

@asomers
Copy link
Owner

asomers commented Jul 31, 2023

BTW, how did you get the variadic functions to work? Could you please point me to your code for that? Or even better, open a separate PR with an example or test case for it?

@enes-aydin
Copy link

Thank you :)

about #[no_mangle]: I was actually planning to make such a change. I kept it brief while opening the PR to save time. I'll look into it and commit the changes soon, even though it might take a little more time.
about CHANGELOG: ok
possible test scenario: It seems logical to do it for a function from the standard C library. what do you think?

I didn't do anything for variadic functions :) I just added c_variadic feature. (The variadic parameters of the variadic function will not be tested.) I think I could make a test case.

@asomers
Copy link
Owner

asomers commented Jul 31, 2023

possible test scenario: It seems logical to do it for a function from the standard C library. what do you think?

The hard part is, how would you convince some C code to call that function? It isn't possible unless you compile the C code as part of Mockall's test suite.

@enes-aydin
Copy link

You're right. My fault. I couldn't think because I was sleepy.

@Enes1313 Enes1313 requested a review from asomers August 1, 2023 14:31
CHANGELOG.md Outdated Show resolved Hide resolved
mockall/tests/automock_foreign_c.rs Outdated Show resolved Hide resolved
mockall_derive/src/mock_function.rs Outdated Show resolved Hide resolved
@Enes1313 Enes1313 requested a review from asomers August 1, 2023 17:11
@Enes1313
Copy link
Contributor Author

Enes1313 commented Aug 1, 2023

I don't know how this error is caused. It failed once. Then it was successful. now it failed again.

test returning ... FAILED

@asomers
Copy link
Owner

asomers commented Aug 1, 2023

Oh, it's a race condition, caused by two tests trying to modify the same Context. For static functions, Context is global. The solution is to prevent those two test cases from running concurrently by using a Mutex. See mockall/tests/mock_struct_with_static_method.rs for an example.

@Enes1313
Copy link
Contributor Author

Enes1313 commented Aug 1, 2023

I learned new things :)

.with(predicate::eq(4))
.returning(i64::from);
unsafe{ mock_ffi::foo1(5) };
ctx.expect().with(predicate::eq(4)).returning(i64::from);
Copy link
Owner

Choose a reason for hiding this comment

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

Please don't combine content changes and formatting changes in the same commit. You can undo the change to this function.

@Enes1313 Enes1313 requested a review from asomers August 1, 2023 19:08
Copy link
Owner

@asomers asomers left a comment

Choose a reason for hiding this comment

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

LGTM! And BTW it causes no unexpected code generation changes. You can check for yourself like this:

cd mockall
sh tools/allgen.sh pp_old
git checkout your_branch_name
sh tools/allgen.sh pp
diff -dur pp_old pp

@asomers asomers merged commit 1adf23c into asomers:master Aug 1, 2023
3 checks passed
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.

3 participants