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

Adds more mocked tests #711

Merged
merged 2 commits into from
Mar 7, 2022
Merged

Adds more mocked tests #711

merged 2 commits into from
Mar 7, 2022

Conversation

128f
Copy link
Contributor

@128f 128f commented Feb 14, 2022

A follow-up to #706

@codecov-commenter
Copy link

codecov-commenter commented Feb 14, 2022

Codecov Report

Merging #711 (c3fcd02) into main (96a50d9) will increase coverage by 0.36%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #711      +/-   ##
==========================================
+ Coverage   71.86%   72.23%   +0.36%     
==========================================
  Files          87       86       -1     
  Lines       11854    11756      -98     
==========================================
- Hits         8519     8492      -27     
+ Misses       3335     3264      -71     

This commit builds on previous work using the mockall library.
We add a mock module with signatures of functions we want to mock from
`libc` and `libbpf`, and then those mocks are used from the `bpf` module.

Tests for the `bpf_query` function required things like setting errors and
manipulating the content of c-arrays in the `returning` callback.

Signed-off-by: Christian Burke <[email protected]>
@128f 128f force-pushed the devices_tests_2 branch from 36f10af to 3e95c84 Compare March 6, 2022 18:00
) -> ::std::os::raw::c_int {
unimplemented!();
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I needed a stubbed module to apply the mock to.

Do we already do something like this elsewhere?

#[cfg(test)]
#[allow(clippy::too_many_arguments)]
#[cfg_attr(coverage, feature(no_coverage))]
pub mod mocks;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

only used in tests, also libc functions trigger the too_many_arguments clippy warning

Copy link
Contributor Author

@128f 128f left a comment

Choose a reason for hiding this comment

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

Added some annotations for things I want to call out for discussion.

pub fn load(license: &str, insns: &[u8]) -> Result<RawFd> {
let insns_cnt = insns.len() / std::mem::size_of::<bpf_insn>();
let insns = insns as *const _ as *const bpf_insn;
#[allow(unused_unsafe)]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't like this one, but the mocked stuff isn't actually unsafe.

We could probably make it look unsafe though...

#[cfg(not(test))]
use libbpf_sys::{
bpf_load_program, bpf_prog_attach, bpf_prog_detach2, bpf_prog_get_fd_by_id, bpf_prog_query,
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the above, we are importing mocked versions or not based on if we are in a test environment

Also move extraneous `use` statement.

Signed-off-by: Christian Burke <[email protected]>
@128f 128f force-pushed the devices_tests_2 branch from ec371ec to c3fcd02 Compare March 6, 2022 18:15
@utam0k utam0k assigned utam0k and unassigned utam0k Mar 6, 2022
@utam0k utam0k self-requested a review March 6, 2022 23:45
Copy link
Member

@utam0k utam0k left a comment

Choose a reason for hiding this comment

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

LGTM

@utam0k utam0k merged commit 800d4f5 into youki-dev:main Mar 7, 2022
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