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

ISSUE-16 - Further unit testing #126

Merged
merged 5 commits into from
Oct 21, 2020
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
279 changes: 176 additions & 103 deletions Cargo.lock

Large diffs are not rendered by default.

2 changes: 2 additions & 0 deletions cargo-geiger/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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"] }
Copy link
Contributor

Choose a reason for hiding this comment

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

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 :)

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 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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that definitely makes sense, I've removed it in the last commit.

geiger = { path = "../geiger", version = "0.4.5" }
petgraph = "0.5.1"
pico-args = "0.3.3"
rand = "0.7.3"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be a dev-dependency instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh, apologies, I had meant to remove that, I have taken out in the last commit.

Copy link
Contributor

Choose a reason for hiding this comment

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

Great!

serde = { version = "1.0.116", features = ["derive"] }
serde_json = "1.0.57"
strum = "0.19.2"
Expand Down
130 changes: 98 additions & 32 deletions cargo-geiger/src/args.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
use crate::format::print::OutputFormat;
use crate::format::print_config::OutputFormat;
use crate::format::Charset;

use fake::{Dummy, Fake};
use pico_args::Arguments;
use std::path::PathBuf;

pub const HELP: &str =
Expand Down Expand Up @@ -54,6 +56,7 @@ OPTIONS:
-V, --version Prints version information.
";

#[derive(Debug, Dummy)]
pub struct Args {
pub all: bool,
pub all_deps: bool,
Expand Down Expand Up @@ -86,51 +89,52 @@ pub struct Args {
}

impl Args {
pub fn parse_args() -> Result<Args, Box<dyn std::error::Error>> {
let mut args = pico_args::Arguments::from_env();
pub fn parse_args(
mut raw_args: Arguments,
) -> Result<Args, Box<dyn std::error::Error>> {
let args = Args {
all: args.contains(["-a", "--all"]),
all_deps: args.contains("--all-dependencies"),
all_features: args.contains("--all-features"),
all_targets: args.contains("--all-targets"),
build_deps: args.contains("--build-dependencies"),
charset: args
all: raw_args.contains(["-a", "--all"]),
all_deps: raw_args.contains("--all-dependencies"),
all_features: raw_args.contains("--all-features"),
all_targets: raw_args.contains("--all-targets"),
build_deps: raw_args.contains("--build-dependencies"),
charset: raw_args
.opt_value_from_str("--charset")?
.unwrap_or(Charset::Utf8),
color: args.opt_value_from_str("--color")?,
dev_deps: args.contains("--dev-dependencies"),
features: args.opt_value_from_str("--features")?,
forbid_only: args.contains(["-f", "--forbid-only"]),
format: args
color: raw_args.opt_value_from_str("--color")?,
dev_deps: raw_args.contains("--dev-dependencies"),
features: raw_args.opt_value_from_str("--features")?,
forbid_only: raw_args.contains(["-f", "--forbid-only"]),
format: raw_args
.opt_value_from_str("--format")?
.unwrap_or_else(|| "{p}".to_string()),
frozen: args.contains("--frozen"),
help: args.contains(["-h", "--help"]),
include_tests: args.contains("--include-tests"),
invert: args.contains(["-i", "--invert"]),
locked: args.contains("--locked"),
manifest_path: args.opt_value_from_str("--manifest-path")?,
no_default_features: args.contains("--no-default-features"),
no_indent: args.contains("--no-indent"),
offline: args.contains("--offline"),
package: args.opt_value_from_str("--manifest-path")?,
prefix_depth: args.contains("--prefix-depth"),
quiet: args.contains(["-q", "--quiet"]),
target: args.opt_value_from_str("--target")?,
unstable_flags: args
frozen: raw_args.contains("--frozen"),
help: raw_args.contains(["-h", "--help"]),
include_tests: raw_args.contains("--include-tests"),
invert: raw_args.contains(["-i", "--invert"]),
locked: raw_args.contains("--locked"),
manifest_path: raw_args.opt_value_from_str("--manifest-path")?,
no_default_features: raw_args.contains("--no-default-features"),
no_indent: raw_args.contains("--no-indent"),
offline: raw_args.contains("--offline"),
package: raw_args.opt_value_from_str("--manifest-path")?,
prefix_depth: raw_args.contains("--prefix-depth"),
quiet: raw_args.contains(["-q", "--quiet"]),
target: raw_args.opt_value_from_str("--target")?,
unstable_flags: raw_args
.opt_value_from_str("-Z")?
.map(|s: String| s.split(' ').map(|s| s.to_owned()).collect())
.unwrap_or_else(Vec::new),
verbose: match (
args.contains("-vv"),
args.contains(["-v", "--verbose"]),
raw_args.contains("-vv"),
raw_args.contains(["-v", "--verbose"]),
) {
(false, false) => 0,
(false, true) => 1,
(true, _) => 2,
},
version: args.contains(["-V", "--version"]),
output_format: if args.contains("--json") {
version: raw_args.contains(["-V", "--version"]),
output_format: if raw_args.contains("--json") {
Some(OutputFormat::Json)
} else {
None
Expand All @@ -139,3 +143,65 @@ impl Args {
Ok(args)
}
}

#[cfg(test)]
pub mod args_tests {
use super::*;

use rstest::*;
use std::ffi::OsString;

#[rstest(
input_argument_vector,
expected_all,
expected_charset,
expected_verbose,
case(
vec![],
false,
Charset::Utf8,
0
),
case(
vec![OsString::from("--all")],
true,
Charset::Utf8,
0,
),
case(
vec![OsString::from("--charset"), OsString::from("ascii")],
false,
Charset::Ascii,
0
),
case(
vec![OsString::from("-v")],
false,
Charset::Utf8,
1
),
case(
vec![OsString::from("-vv")],
false,
Charset::Utf8,
2
)
)]
fn parse_args_test(
input_argument_vector: Vec<OsString>,
expected_all: bool,
expected_charset: Charset,
expected_verbose: u32,
) {
let args_result =
Args::parse_args(Arguments::from_vec(input_argument_vector));

assert!(args_result.is_ok());

let args = args_result.unwrap();

assert_eq!(args.all, expected_all);
assert_eq!(args.charset, expected_charset);
assert_eq!(args.verbose, expected_verbose)
}
}
12 changes: 7 additions & 5 deletions cargo-geiger/src/cli.rs
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,9 @@ pub fn resolve<'a, 'cfg>(
mod cli_tests {
use super::*;

#[test]
use rstest::*;

#[rstest]
fn get_cfgs_test() {
let config = Config::default().unwrap();

Expand All @@ -135,11 +137,11 @@ mod cli_tests {
.filter(|cfg| matches!(cfg, Cfg::KeyPair(_, _)))
.collect();

assert!(names.len() > 0);
assert!(key_pairs.len() > 0);
assert!(!names.is_empty());
assert!(!key_pairs.is_empty());
}

#[test]
#[rstest]
fn get_registry_test() {
let config = Config::default().unwrap();
let workspace = Workspace::new(
Expand All @@ -162,7 +164,7 @@ mod cli_tests {
assert_eq!(package_set.sources().len(), 1);
}

#[test]
#[rstest]
fn get_workspace_test() {
let config = Config::default().unwrap();
let manifest_path: Option<PathBuf> = None;
Expand Down
13 changes: 9 additions & 4 deletions cargo-geiger/src/format.rs
Original file line number Diff line number Diff line change
@@ -1,22 +1,24 @@
pub mod emoji_symbols;
pub mod pattern;
pub mod print;
pub mod print_config;
pub mod table;

mod display;
mod parse;

use cargo::core::dependency::DepKind;
use fake::Dummy;
use std::fmt;
use std::str::{self, FromStr};
use strum_macros::EnumIter;

#[derive(Clone, Copy, Debug, PartialEq)]
#[derive(Clone, Copy, Debug, Dummy, PartialEq)]
pub enum Charset {
Ascii,
Utf8,
}

#[derive(Debug, PartialEq)]
pub enum Chunk {
License,
Package,
Expand All @@ -43,6 +45,7 @@ pub enum CrateDetectionStatus {
UnsafeDetected,
}

#[derive(Debug, PartialEq)]
pub enum RawChunk<'a> {
Argument(&'a str),
Error(&'static str),
Expand Down Expand Up @@ -83,14 +86,16 @@ pub fn get_kind_group_name(dep_kind: DepKind) -> Option<&'static str> {
mod format_tests {
use super::*;

#[test]
use rstest::*;

#[rstest]
fn charset_from_str_test() {
assert_eq!(Charset::from_str("ascii"), Ok(Charset::Ascii));
assert_eq!(Charset::from_str("utf8"), Ok(Charset::Utf8));
assert_eq!(Charset::from_str("invalid_str"), Err("invalid charset"));
}

#[test]
#[rstest]
fn get_kind_group_name_test() {
assert_eq!(
get_kind_group_name(DepKind::Build),
Expand Down
84 changes: 77 additions & 7 deletions cargo-geiger/src/format/display.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,11 @@ impl<'a> fmt::Display for Display<'a> {
fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result {
for chunk in &self.pattern.0 {
match *chunk {
Chunk::Raw(ref s) => (fmt.write_str(s))?,
Chunk::License => {
if let Some(ref license) = self.metadata.license {
(write!(fmt, "{}", license))?
}
}
Chunk::Package => {
(write!(
fmt,
Expand All @@ -24,19 +28,85 @@ impl<'a> fmt::Display for Display<'a> {
self.package.version()
))?
}
Chunk::License => {
if let Some(ref license) = self.metadata.license {
(write!(fmt, "{}", license))?
}
}
Chunk::Raw(ref s) => (fmt.write_str(s))?,
Chunk::Repository => {
if let Some(ref repository) = self.metadata.repository {
(write!(fmt, "{}", repository))?
}
}
}
}

Ok(())
}
}

#[cfg(test)]
pub mod display_tests {
use super::*;

use crate::format::pattern::Pattern;
use crate::format::Chunk;

use cargo::core::manifest::ManifestMetadata;
use cargo::core::{PackageId, SourceId};
use cargo::util::ToSemver;
use rstest::*;

#[rstest(
input_pattern,
expected_formatted_string,
case(
Pattern(vec![Chunk::License]),
"licence_string"
),
case(
Pattern(vec![Chunk::Package]),
"package_name 1.2.3"
),
case(
Pattern(vec![Chunk::Raw(String::from("chunk_value"))]),
"chunk_value"
),
case(
Pattern(vec![Chunk::Repository]),
"repository_string"
)
)]
fn display_format_fmt_test(
input_pattern: Pattern,
expected_formatted_string: &str,
) {
let package_id = PackageId::new(
"package_name",
"1.2.3".to_semver().unwrap(),
SourceId::from_url(
"git+https://github.com/rust-secure-code/cargo-geiger",
)
.unwrap(),
)
.unwrap();

let manifest_metadata = ManifestMetadata {
authors: vec![],
keywords: vec![],
categories: vec![],
license: Some(String::from("licence_string")),
license_file: None,
description: None,
readme: None,
homepage: None,
repository: Some(String::from("repository_string")),
documentation: None,
badges: Default::default(),
links: None,
};

let display = Display {
pattern: &input_pattern,
package: &package_id,
metadata: &manifest_metadata,
};

assert_eq!(format!("{}", display), expected_formatted_string);
}
}
Loading