Skip to content

DO NOT MERGE: Demonstrate lacking test coverage #163

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

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

tyll
Copy link

@tyll tyll commented Apr 15, 2025

To demonstrate that the current test strategy is lacking test coverage,
break the MinMtu data type without breaking the existing test cases.

The second commit introduces Boundary Value Analysis based test cases using doc tests in lack of a better alternative. This is not a proposal for doing the tests exactly as this as I don't have the experience with Rust in how to organize tests best and it might also not be the best tool to use this. This is merely a demonstration about how tests catch problem.

As a note, a boundary value analysis for FailOverMac with [-1, 3] as the bad cases and [0, 2] might have led to using an Enum from the beginning instead of u8, avoiding the later cleanup in 3270863

tyll added 2 commits April 15, 2025 13:01
To demonstrate that the current test strategy is lacking test coverage,
break the MinMtu data type without breaking the existing test cases.
I was not able to execute them from the tests/ directory. They are
executed with `cargo test --doc`.
@cathay4t
Copy link
Member

cathay4t commented Apr 22, 2025

I think boundary test should be done by rust compiler not in this crate. This crate is not responsible for user's fault when using this crate.

@cathay4t
Copy link
Member

Whether the data type is correct or not should be determined by reviewer checking linux kernel code on specified netlink attribute.

@tyll
Copy link
Author

tyll commented Apr 23, 2025

I think boundary test should be done by rust compiler not in this crate. This crate is not responsible for user's fault when using this crate.

It seems to me that there is a confusion about Boundary Value Analysis as this is not something that a specific component is responsible for but a technique to identify a minimum set of useful test cases. If at all, the rust compile should have test cases identified using the Boundary Value Analysis.

Whether the data type is correct or not should be determined by reviewer checking linux kernel code on specified netlink attribute.

I agree that this should be the source to identify the correct value range for properties. However, ensuring that the code is correct according to the deducted specification from the kernel code is the responsibility of unit tests. See for example: https://www.reddit.com/r/rust/comments/sdag9x/comment/hudzvn0/ - "You write a piece of code in concert with "unit tests" that exercise the code and documents all of its behavior. ". This is currently not happening. https://github.com/rust-netlink/netlink-packet-route#development requests to use unit tests, though.

Also note, that the tests in this PR are not necessarily the best approach to test this as there are other possibilities as well. There could be examples for (de)serialization that use values identified using the Boundary Value Analysis instead of using arbitrary values inside an equivalence partition and the de-serialization test for the boundary values would most likely catch the correct tests as well. However, https://github.com/rust-netlink/netlink-packet-route#development also state that no panic is allowed, so if invalid values lead to a compile error instead of a panic, testing for compile errors is a result of the language design if this is the expected behavior of a crate.

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