search

retis-org / retis

Tracing packets in the Linux networking stack & friends
https://retis.readthedocs.io/en/stable/
100 stars 14 forks source link

Remove all #[cfg(not(test))] #325

Closed liuhangbin closed 10 months ago

liuhangbin commented 11 months ago

Close #103

Before: test result: ok. 56 passed; 0 failed; 5 ignored; 0 measured; 0 filtered out; finished in 32.05s After: test result: ok. 56 passed; 0 failed; 5 ignored; 0 measured; 0 filtered out; finished in 30.53s

atenart commented 10 months ago

Hi Hangbin, thanks for the PR!

Looks like build (eg. cargo check is not passing). Making it explicit since it's your first PR; we usually wait for all CI tests to pass before making a review.

Close #103

Before: test result: ok. 56 passed; 0 failed; 5 ignored; 0 measured; 0 filtered out; finished in 32.05s After: test result: ok. 56 passed; 0 failed; 5 ignored; 0 measured; 0 filtered out; finished in 30.53s

Still had a quick look at this and I don't think this can work (eg. BPF maps handling are not skipped anymore while testing). Tests might have worked when ran as root, but they should pass as a normal user.

liuhangbin commented 10 months ago

Looks like build (eg. cargo check is not passing). Making it explicit since it's your first PR; we usually wait for all CI tests to pass before making a review.

OK, I will check it.

Still had a quick look at this and I don't think this can work (eg. BPF maps handling are not skipped anymore while testing). Tests might have worked when ran as root, but they should pass as a normal user.

Thanks for this info. I will run the test with normal user in future.

liuhangbin commented 10 months ago

Still had a quick look at this and I don't think this can work (eg. BPF maps handling are not skipped anymore while testing). Tests might have worked when ran as root, but they should pass as a normal user.

OK, looks all the failures are because the bpf map init failed, which calls libbpf_rs::MapHandle::create() and needs root permission. I don't have any idea how to avoid it except not build it via #[cfg(not(test))], or define it as Option.

I don't know if convert libbpf_rs::MapHandle to Option<libbpf_rs::MapHandle> accepts. Because this would change the code a lot.

atenart commented 10 months ago

I don't know if convert libbpf_rs::MapHandle to Option<libbpf_rs::MapHandle> accepts. Because this would change the code a lot.

Right, let's not make runtime conditionals used for testing only. One way could be to make a stub implementation of those root-dependent objects and to have a noop impl for test but that is not easy to maintain and would extend not only to maps. So maybe the best way is close to the current implementation, which might be improved by better grouping those (if possible, did not looked at the code).

Regarding the probe manager, you can have a look at #309 maybe, as the manager is cleaned up and removing #[cfg(not(test))] might be easier somehow there.

liuhangbin commented 10 months ago

So maybe the best way is close to the current implementation, which might be improved by better grouping those

Since this is hard to remove all #[cfg(not(test))] and we still need to use(or re-implement) the current implementation. I plan to close this PR first. I will re-work this when we have a better solution.