-
Notifications
You must be signed in to change notification settings - Fork 355
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
Conversation
Codecov Report
@@ 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 |
316453d
to
f68c2ee
Compare
f68c2ee
to
ddb72cc
Compare
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]>
) -> ::std::os::raw::c_int { | ||
unimplemented!(); | ||
} | ||
} |
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.
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; |
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.
only used in tests, also libc functions trigger the too_many_arguments
clippy warning
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.
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)] |
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.
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, | ||
}; |
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.
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]>
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.
LGTM
A follow-up to #706