ISSUE-16 - Further unit testing #126
Conversation
* Update existing testing to use rstest * Add cases for tests where appropriate * Pulling out sub-module from format/table * Improve general test coverage
jmcconnell26
force-pushed
the
ISSUE-16-FurtherUnitTesting
branch
from
125025b
to
cfe186f
Compare
|
#125 is merged now. |
|
Hi @anderejd, I've merged in the branch to master, and this is now ready for review. Thanks, |
|
I've raised a PR here to add code coverage metrics to the pipeline here #127, which can hopefully be merged in any order! Thanks, |
cargo-geiger/Cargo.toml
Outdated
| @@ -20,9 +20,11 @@ cargo-platform = "0.1.1" | |||
| colored = "2.0.0" | |||
| console = "0.11.3" | |||
| env_logger = "0.7.1" | |||
| fake = { version = "2.2", features=["derive"] } | |||
There was a problem hiding this comment.
I'm not sure about this library, seems obfuscate the code rather than add value, in my opinion.
How would the code look without the new fake dependency?
Convince me that I'm wrong :)
There was a problem hiding this comment.
I was 50/50 on this library, I really didn't like the fact that it didn't support being added as a dev-dependency for something which was only used in testing.
I came down on the side of including it for how much simpler it made creating large structs in unit tests, for example https://github.com/rust-secure-code/cargo-geiger/pull/126/files#diff-373099d9a5cf5c2a276c879a94c76bdbd5dab804f2dca1ad72776f3b6ea4cb7bR195 for the relative cost of adding derive statement.
If I wasn't to use this, I'd probably create a fixture method with rstest to initialise the default struct, and then inject it into each test.
There was a problem hiding this comment.
I was 50/50 on this library, I really didn't like the fact that it didn't support being added as a dev-dependency for something which was only used in testing.
This alone makes it unfit for use, imho. Making users wait for extra bloat to compile while installing is a bad user experience. Let's go for some extra manually written test code and avoid the fake crate.
There was a problem hiding this comment.
Yeah, that definitely makes sense, I've removed it in the last commit.
Signed-off-by: joshmc <josh-mcc@tiscali.co.uk>
jmcconnell26
force-pushed
the
ISSUE-16-FurtherUnitTesting
branch
from
df00ed7
to
91b8aee
Compare
| #[rstest( | ||
| input_none_detected_forbids_unsafe, | ||
| input_none_detected_allows_unsafe, | ||
| input_unsafe_detected, | ||
| expected_crate_detection_status, | ||
| case(0, 0, 1, CrateDetectionStatus::UnsafeDetected), | ||
| case(1, 0, 0, CrateDetectionStatus::NoneDetectedForbidsUnsafe), | ||
| case(4, 1, 0, CrateDetectionStatus::NoneDetectedAllowsUnsafe) | ||
| )] | ||
| fn total_package_counts_get_total_detection_status_tests( | ||
| input_none_detected_forbids_unsafe: i32, | ||
| input_none_detected_allows_unsafe: i32, | ||
| input_unsafe_detected: i32, | ||
| expected_crate_detection_status: CrateDetectionStatus, | ||
| ) { | ||
| let total_detection_status = TotalPackageCounts { | ||
| none_detected_forbids_unsafe: input_none_detected_forbids_unsafe, | ||
| none_detected_allows_unsafe: input_none_detected_allows_unsafe, | ||
| unsafe_detected: input_unsafe_detected, | ||
| total_counter_block: CounterBlock::default(), | ||
| total_unused_counter_block: CounterBlock::default(), | ||
| }; | ||
|
|
||
| assert_eq!( | ||
| total_package_counts_unsafe_detected.get_total_detection_status(), | ||
| CrateDetectionStatus::UnsafeDetected | ||
| ); | ||
|
|
||
| let total_package_counts_none_detected_forbids_unsafe = | ||
| TotalPackageCounts { | ||
| none_detected_forbids_unsafe: 1, | ||
| none_detected_allows_unsafe: 0, | ||
| unsafe_detected: 0, | ||
| total_counter_block: CounterBlock::default(), | ||
| total_unused_counter_block: CounterBlock::default(), | ||
| }; | ||
|
|
||
| assert_eq!( | ||
| total_package_counts_none_detected_forbids_unsafe | ||
| .get_total_detection_status(), | ||
| CrateDetectionStatus::NoneDetectedForbidsUnsafe | ||
| ); | ||
|
|
||
| let total_package_counts_none_detected_allows_unsafe = | ||
| TotalPackageCounts { | ||
| none_detected_forbids_unsafe: 4, | ||
| none_detected_allows_unsafe: 1, | ||
| unsafe_detected: 0, | ||
| total_counter_block: CounterBlock::default(), | ||
| total_unused_counter_block: CounterBlock::default(), | ||
| }; | ||
|
|
||
| assert_eq!( | ||
| total_package_counts_none_detected_allows_unsafe | ||
| .get_total_detection_status(), | ||
| CrateDetectionStatus::NoneDetectedAllowsUnsafe | ||
| total_detection_status.get_total_detection_status(), | ||
| expected_crate_detection_status |
There was a problem hiding this comment.
Nice and compact! A clear improvement 👍
| pub fn create_table_from_text_tree_lines( | ||
| geiger_context: &GeigerContext, | ||
| package_set: &PackageSet, | ||
| print_config: &PrintConfig, | ||
| rs_files_used: &HashSet<PathBuf>, | ||
| table_parameters: &TableParameters, |
| TextTreeLine::ExtraDepsGroup { | ||
| kind: dep_kind, | ||
| tree_vines, | ||
| } => handle_text_tree_line_extra_deps_group( | ||
| dep_kind, | ||
| &mut table_lines, | ||
| tree_vines, | ||
| ), | ||
| TextTreeLine::Package { | ||
| id: package_id, | ||
| tree_vines, | ||
| } => handle_text_tree_line_package( | ||
| &emoji_symbols, | ||
| &mut handle_package_parameters, | ||
| package_id, | ||
| package_set, | ||
| &mut table_lines, | ||
| table_parameters, | ||
| tree_vines, | ||
| ), |
There was a problem hiding this comment.
Much easier to read with the cases factored out as functions 👍
cargo-geiger/Cargo.toml
Outdated
| @@ -23,6 +23,7 @@ env_logger = "0.7.1" | |||
| geiger = { path = "../geiger", version = "0.4.5" } | |||
| petgraph = "0.5.1" | |||
| pico-args = "0.3.3" | |||
| rand = "0.7.3" | |||
There was a problem hiding this comment.
Can this be a dev-dependency instead?
There was a problem hiding this comment.
Ahh, apologies, I had meant to remove that, I have taken out in the last commit.
Signed-off-by: joshmc <josh-mcc@tiscali.co.uk>
Adding further unit testing + small refactoring.
Waiting until #125 is merged, and can then merge into this branch.