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

Added C variadic test #508

Merged
merged 3 commits into from
Aug 2, 2023
Merged

Conversation

Enes1313
Copy link
Contributor

@Enes1313 Enes1313 commented Aug 1, 2023

Add Test Scenario for C Variadics Using Mockall

This PR adds a test file to address the mocking of 'c_variadic' functions. However, please note that testing the variadic parameters themselves is not feasible due to their dynamic nature.

r @asomers

@Enes1313 Enes1313 requested a review from asomers as a code owner August 1, 2023 22:23
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.

Neat! After the last PR, this one is surprisingly easy. Still, it's a shame that we can't match on the variadic args. I took a look at what changes would be required to permit that. It looks like it should be possible. The hard part is dealing with the lifetime in the VaList structure. It might be possible to do it, subject to the restriction that variadic methods may only be matched with .withf(), not .with(), similar to the use of #[concretize].

Still, I'm a little bit reluctant to add support for nightly-only features, because that can be a maintenance burden. How useful do you think it would be?

@Enes1313 Enes1313 closed this Aug 2, 2023
@Enes1313 Enes1313 reopened this Aug 2, 2023
@Enes1313
Copy link
Contributor Author

Enes1313 commented Aug 2, 2023

I guess, the "resolvability of variadic parameters" can vary depending on the specific coding context, and the complexity of the changes to address it may differ based on the application and usage scenarios.

For example;

void foo(const char *format, ...);

What should we do in this function or similar functions that are similar to the signature of the printf function? Types vary. There was no type inference in the c_variadic property of the Rust language. Do you really think it can be done?

The additional changes beyond the previous PR, in my opinion, would complicate things and create maintenance burden. I think this PR should stay as it is, demonstrating this usage.

@Enes1313 Enes1313 requested a review from asomers August 2, 2023 19:36
@asomers
Copy link
Owner

asomers commented Aug 2, 2023

I guess, the "resolvability of variadic parameters" can vary depending on the specific coding context, and the complexity of the changes to address it may differ based on the application and usage scenarios.

For example;

void foo(const char *format, ...);

What should we do in this function or similar functions that are similar to the signature of the printf function? Types vary. There was no type inference in the c_variadic property of the Rust language. Do you really think it can be done?

The additional changes beyond the previous PR, in my opinion, would complicate things and create maintenance burden. I think this PR should stay as it is, demonstrating this usage.

I don't think that Mockall needs to make such decisions. Instead, it should pass a VaList structure to the matchers and returners. The generated code would look like this:

    #[no_mangle]
    pub unsafe extern "C" fn foo(x: i32, y: i32, mut args: ...) -> i32 {
        use ::mockall::{ViaDebug, ViaNothing};
        let no_match_msg = std::format!(
            "{}: No matching expectation found",
            std::format!(
                "mock_ffi::foo({:?}, {:?})",
                (&&::mockall::ArgPrinter(&x)).debug_string(),
                (&&::mockall::ArgPrinter(&y)).debug_string()
            )
        );
        {
            let __mockall_guard = __foo::EXPECTATIONS.lock().unwrap();
            __mockall_guard.call(x, y, args.as_va_list())
        }
        .expect(&no_match_msg)
    }

However, the tricky part is dealing with the lifetime of that VaList object. And I don't think we need to add that right now. This PR is useful even without it.

@asomers asomers merged commit 31709a1 into asomers:master Aug 2, 2023
@Enes1313
Copy link
Contributor Author

Enes1313 commented Aug 2, 2023

Ooo very very coo l. You're right.

And I don't think we need to add that right now. -> Yes. It can be left for later.

Maybe I can do it once I learn the Rust language well :)

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