-
Notifications
You must be signed in to change notification settings - Fork 60
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
test: add test framework for the CLI #291
Conversation
This introduce a testing framework for the CLI, which as completely uncovered by tests, allowing regression issues. More tests will be introduced in later commits.
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.
Hi @plusvic and reviewers,
I'm currently reviewing this pull request and will provide my feedback in a few minutes. In the meantime, I'm providing this summary of the pull request's content to help you and other reviewers quickly understand the changes and intent.
This pull request adds a test framework for the YARA CLI. The author notes that the CLI previously lacked tests, leading to potential regression issues. This change aims to prevent such issues by introducing a testing framework and some initial tests. More tests are planned for future commits.
Here's a summary of the changes:
.gitattributes
(lines 6-11): Added rules to specify line endings for.stdout
and.stderr
files, ensuring consistency across different operating systems..github/workflows/tests.yaml
(lines 114-125): Modified the CI workflow to separate the build and test steps. This allows for more granular control and easier debugging.Cargo.lock
(multiple changes): Added several new dependencies required for the testing framework:assert_cmd
,difflib
,doc-comment
,predicates
,predicates-core
,predicates-tree
,termtree
, andwait-timeout
.cli/Cargo.toml
(lines 14-25 and 66-79): Addedtest = false
to the[bin]
section to prevent the CLI tests from running by default withcargo test
. Also addedassert_cmd
,globwalk
, andgoldenfile
as dev-dependencies.cli/src/main.rs
(lines 3-11): Added atests
module (conditionally compiled for test environments).cli/src/tests/mod.rs
(lines 1-33): This file contains the core test logic, usingassert_cmd
andgoldenfile
to compare the CLI's output against expected values. The tests are driven by.in
files in thetestdata
directory.cli/src/tests/rules/*.yar
(multiple changes): Added YARA rule files used as input for the CLI tests.cli/src/tests/testdata/*.in
,cli/src/tests/testdata/*.stderr
,cli/src/tests/testdata/*.stdout
(multiple changes): These files contain test inputs and expected outputs for the CLI tests.
I'll be looking at the specifics of the test implementation and coverage shortly.
And now, a little poem to brighten your day:
Tests now run,
Bugs are caught before they fly,
Code's a joyful dance.
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.
This pull request introduces a testing framework for the CLI, which is a much-needed addition. The approach of using a test data directory with input files and expected output files is generally sound. However, the current implementation generates the .stderr
and .stdout
files during the test execution. This can make it difficult to review test failures, as the reviewer needs to manually run the tests to see the actual output. I recommend modifying the tests to use assert_cmd
's assertions directly, which provides more informative output on failures and simplifies the review process.
cli/src/tests/mod.rs
Outdated
#[test] | ||
fn test_cli() { | ||
let mut mint = goldenfile::Mint::new("."); | ||
|
||
for entry in globwalk::glob("src/tests/testdata/*.in").unwrap().flatten() { | ||
// Path to the .in file. | ||
let in_path = entry.into_path(); | ||
let stderr_path = in_path.with_extension("stderr"); | ||
let stdout_path = in_path.with_extension("stdout"); | ||
|
||
let args = fs::read_to_string(&in_path).expect("unable to read"); | ||
|
||
let mut cmd = Command::cargo_bin("yr").unwrap(); | ||
|
||
cmd.args(args.split(" ")); | ||
|
||
let mut stderr_file = mint.new_goldenfile(stderr_path).unwrap(); | ||
let mut stdout_file = mint.new_goldenfile(stdout_path).unwrap(); | ||
|
||
stderr_file | ||
.write_all(&*cmd.output().unwrap().stderr) | ||
.expect("unable to write .stderr file"); | ||
|
||
stdout_file | ||
.write_all(&*cmd.output().unwrap().stdout) | ||
.expect("unable to write .stdout file"); | ||
} | ||
} |
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.
Instead of generating golden files, use assert_cmd
's assertions for validating the CLI output. This makes tests more self-contained and easier to understand. Also, consider using tempfile
for creating temporary files and directories for test data, ensuring tests don't leave behind any files after execution.
#[test]
fn test_scan_help() {
Command::cargo_bin("yr").unwrap()
.arg("scan")
.arg("--help")
.assert()
.success()
.stdout(predicate::str::contains("Scan a file or directory"));
}
#[test]
fn test_scan_always_true() {
Command::cargo_bin("yr").unwrap()
.arg("scan")
.arg("src/tests/rules/always_true.yar")
.arg("src/tests/rules/always_true.yar")
.assert()
.success()
.stderr(predicate::str::contains("warning[invariant_expr]: invariant boolean expression"))
.stdout(predicate::str::contains("always_true src/tests/rules/always_true.yar"));
}
#[test]
fn test_scan_foo() {
Command::cargo_bin("yr").unwrap()
.arg("scan")
.arg("-s")
.arg("src/tests/rules/foo.yar")
.arg("src/tests/rules/foo.yar")
.assert()
.success()
.stderr(predicate::str::contains("warning[text_as_hex]: hex pattern could be written as text literal"))
.stdout(predicate::str::contains("foo src/tests/rules/foo.yar"));
}
The CLI was not covered by tests, allowing regression issues that could be prevented. This puts in place a testing framework for the CLI with a few basic tests. More tests will come later.