Add test documentation and expand CLI test coverage #46

kaladron · 2025-11-23T13:21:41Z

Document testing philosophy and expand test suite with better CLI-focused tests and improved error verification.

Changes:

Add tests/README.md documenting testing philosophy separating application (CLI interface) from library (archive format) testing concerns
Update README.md with Testing section linking to test documentation
Add 23 new CLI-focused tests covering:
- Basic CLI: conflicting operations, missing operation specification
- Create operations: directory archiving, verbose output, empty archive handling, nonexistent file errors
- Extract operations: verbose output, directory structure preservation
- Round-trip tests: single/multiple files, directories, empty files, special characters in filenames
- Error handling: permission denied, corrupted archives, dash-prefixed filenames with -- separator
- Verbose output: format verification for create/extract operations
- CLI parsing: mixed short/long options, option order variations, default overwrite behavior
- Edge cases: filenames with spaces, large file counts (100 files)
Improve all 8 existing tests with better error verification:
- Add exit code checks and stderr pattern matching
- Add no_stderr() assertions for successful operations
- Add sanity checks for archive file sizes
- Add TODO comments noting exit code mismatches (usage errors currently return 1 instead of documented 64)
Reorganize tests with clear section headers:
1. Basic CLI Tests
2. Create Operation Tests
3. Extract Operation Tests
4. Round-trip Tests
5. Error Handling and Exit Code Tests
6. Verbose Output Format Tests
7. CLI Argument Handling Tests
8. Edge Case Tests

All 31 tests pass. Test suite now has clear documentation explaining what should be tested at the application level vs library level.

Document testing philosophy and expand test suite with better CLI-focused tests and improved error verification. Changes: - Add tests/README.md documenting testing philosophy separating application (CLI interface) from library (archive format) testing concerns - Update README.md with Testing section linking to test documentation - Add 23 new CLI-focused tests covering: * Basic CLI: conflicting operations, missing operation specification * Create operations: directory archiving, verbose output, empty archive handling, nonexistent file errors * Extract operations: verbose output, directory structure preservation * Round-trip tests: single/multiple files, directories, empty files, special characters in filenames * Error handling: permission denied, corrupted archives, dash-prefixed filenames with -- separator * Verbose output: format verification for create/extract operations * CLI parsing: mixed short/long options, option order variations, default overwrite behavior * Edge cases: filenames with spaces, large file counts (100 files) - Improve all 8 existing tests with better error verification: * Add exit code checks and stderr pattern matching * Add no_stderr() assertions for successful operations * Add sanity checks for archive file sizes * Add TODO comments noting exit code mismatches (usage errors currently return 1 instead of documented 64) - Reorganize tests with clear section headers: 1. Basic CLI Tests 2. Create Operation Tests 3. Extract Operation Tests 4. Round-trip Tests 5. Error Handling and Exit Code Tests 6. Verbose Output Format Tests 7. CLI Argument Handling Tests 8. Edge Case Tests All 31 tests pass. Test suite now has clear documentation explaining what should be tested at the application level vs library level.

kaladron · 2025-11-23T13:25:54Z

@sylvestre Here's the rest of the tests that I had held back from the initial commit since it would've been a huge review.

One thing in particular to note: Some of the tests confirm an exit code of 1, which we know is incorrect. I'd like to leave those at the moment (the tests are marked with a TODO) because this is the current behaviour of the code and I'm trying to keep the reviews to a manageable size.

After that, I want to modify the test harness to test against the system tar. That way mistakes like these will be caught. It will be convenient to have a mismatch to prove that the tests are working. Then I will fix any gaps between them.

Thanks!

cakebaker · 2025-11-24T15:07:00Z

README.md


+## Testing
+
+The tar application has a focused testing philosophy that separates concerns between the application (CLI interface, error handling, user experience) and the underlying tar-rs library (archive format correctness, encoding, permissions).


A detail:

Suggested change

The tar application has a focused testing philosophy that separates concerns between the application (CLI interface, error handling, user experience) and the underlying tar-rs library (archive format correctness, encoding, permissions).

The tar application has a focused testing philosophy that separates concerns between the application (CLI interface, error handling, user experience) and the underlying `tar-rs` library (archive format correctness, encoding, permissions).

cakebaker · 2025-11-24T15:14:12Z

tests/README.md

+
+The `tar` utility is built on top of the `tar-rs` library. Because of this, we split our testing into two distinct areas:
+
+1.  **The Library (`tar-rs`)**: This is where we test the nitty-gritty details of the tar format. If you want to verify that permissions are preserved correctly, that long paths are handled according to the UStar spec, or that unicode filenames are encoded properly, those tests belong in `tar-rs/tests/`.


I don't know if it's a planned refactoring, but currently there is no tar-rs/tests folder.

Ah, sorry. I have too many checkouts, and this was probably be getting confused. Thanks, I will update.

cakebaker · 2025-11-24T15:36:37Z

tests/by-util/test_tar.rs

+    new_ucmd!()
+        .args(&["-f", "archive.tar"])
+        .fails()
+        .code_is(1) // TODO: align with GNU tar by returning exit code 64


A detail:

Suggested change

.code_is(1) // TODO: align with GNU tar by returning exit code 64

.code_is(1) // TODO: align with GNU tar by returning exit code 2

I'll double check this (and below) but I had tested these by hand for comparison and I think these were the ones returning 64. The GNU tar documentation only document exit 0,1, and 2.

cakebaker · 2025-11-24T15:39:43Z

tests/by-util/test_tar.rs

-    ucmd.args(&["-cf", "archive.tar", "file1.txt"]).succeeds();
+    ucmd.args(&["-cf", "archive.tar", "file1.txt"])
+        .succeeds()
+        .no_stderr();


I would use no_output() to indicate there is no output at all.

Suggested change

.no_stderr();

.no_output();

cakebaker · 2025-11-24T15:41:52Z

tests/by-util/test_tar.rs

-        .succeeds();
+    ucmd.args(&["-cf", "archive.tar", "file1.txt", "file2.txt", "file3.txt"])
+        .succeeds()
+        .no_stderr();


Suggested change

.no_stderr();

.no_output();

cakebaker · 2025-11-24T15:43:37Z

tests/by-util/test_tar.rs

+
+    ucmd.args(&["-cf", "archive.tar", "dir1"])
+        .succeeds()
+        .no_stderr();


Suggested change

.no_stderr();

.no_output();

cakebaker · 2025-11-24T15:48:43Z

tests/by-util/test_tar.rs

+    new_ucmd!()
+        .args(&["-cf", "archive.tar"])
+        .fails()
+        .code_is(1) // TODO: propagate usage exit code 64 once empty archive handling is fixed


Suggested change

.code_is(1) // TODO: propagate usage exit code 64 once empty archive handling is fixed

.code_is(1) // TODO: propagate usage exit code 2 once empty archive handling is fixed

cakebaker · 2025-11-24T15:53:00Z

tests/by-util/test_tar.rs

+    let (_at, mut ucmd) = at_and_ucmd!();
+
+    ucmd.args(&["-cf", "archive.tar", "nonexistent.txt"])


I would use new_ucmd!() here.

Suggested change

let (_at, mut ucmd) = at_and_ucmd!();

ucmd.args(&["-cf", "archive.tar", "nonexistent.txt"])

new_ucmd!()

.args(&["-cf", "archive.tar", "nonexistent.txt"])

cakebaker · 2025-11-24T15:54:15Z

tests/by-util/test_tar.rs

        .current_dir(at.as_string())
-        .succeeds();
+        .succeeds()
+        .no_stderr();


Suggested change

.no_stderr();

.no_output();

cakebaker · 2025-11-24T16:00:29Z

tests/by-util/test_tar.rs

+    std::fs::remove_dir(at.plus("testdir/subdir")).unwrap();
+    std::fs::remove_dir(at.plus("testdir")).unwrap();


An alternative is rmdir:

Suggested change

std::fs::remove_dir(at.plus("testdir/subdir")).unwrap();

std::fs::remove_dir(at.plus("testdir")).unwrap();

at.rmdir("testdir/subdir");

at.rmdir("testdir");

I'm still learning, so sorry for the silly question. Why do we prefer the at. method instead of remove_dir?

There's nothing wrong with using remove_dir, so feel free to leave it as it is ;-) I mentioned rmdir to show the testing framework provides a convenience function for the same functionality that is a bit shorter.

cakebaker · 2025-11-24T16:06:15Z

tests/by-util/test_tar.rs

+#[test]
+fn test_roundtrip_single_file() {
+    let (at, mut ucmd) = at_and_ucmd!();
+
+    // Create a file
+    at.write("file.txt", "test content");
+
+    // Create archive
+    ucmd.args(&["-cf", "archive.tar", "file.txt"]).succeeds();
+
+    // Remove original
+    at.remove("file.txt");
+
+    // Extract
+    new_ucmd!()
+        .arg("-xf")
+        .arg(at.plus("archive.tar"))
+        .current_dir(at.as_string())
+        .succeeds();
+
+    // Verify content is identical
+    assert!(at.file_exists("file.txt"));
+    assert_eq!(at.read("file.txt"), "test content");
+}


This test is a duplicate of test_extract_single_file

Suggested change

#[test]

fn test_roundtrip_single_file() {

let (at, mut ucmd) = at_and_ucmd!();

// Create a file

at.write("file.txt", "test content");

// Create archive

ucmd.args(&["-cf", "archive.tar", "file.txt"]).succeeds();

// Remove original

at.remove("file.txt");

// Extract

new_ucmd!()

.arg("-xf")

.arg(at.plus("archive.tar"))

.current_dir(at.as_string())

.succeeds();

// Verify content is identical

assert!(at.file_exists("file.txt"));

assert_eq!(at.read("file.txt"), "test content");

}

cakebaker · 2025-11-24T16:10:15Z

tests/by-util/test_tar.rs

+#[test]
+fn test_roundtrip_multiple_files() {
+    let (at, mut ucmd) = at_and_ucmd!();
+
+    // Create multiple files with different content
+    at.write("file1.txt", "content one");
+    at.write("file2.txt", "content two");
+    at.write("file3.txt", "content three");
+
+    // Create archive
+    ucmd.args(&["-cf", "archive.tar", "file1.txt", "file2.txt", "file3.txt"])
+        .succeeds();
+
+    // Remove originals
+    at.remove("file1.txt");
+    at.remove("file2.txt");
+    at.remove("file3.txt");
+
+    // Extract
+    new_ucmd!()
+        .arg("-xf")
+        .arg(at.plus("archive.tar"))
+        .current_dir(at.as_string())
+        .succeeds();
+
+    // Verify all contents are identical
+    assert_eq!(at.read("file1.txt"), "content one");
+    assert_eq!(at.read("file2.txt"), "content two");
+    assert_eq!(at.read("file3.txt"), "content three");
+}


This test is more or less a duplicate of test_extract_multiple_files, with the only difference that this test uses three files.

Suggested change

#[test]

fn test_roundtrip_multiple_files() {

let (at, mut ucmd) = at_and_ucmd!();

// Create multiple files with different content

at.write("file1.txt", "content one");

at.write("file2.txt", "content two");

at.write("file3.txt", "content three");

// Create archive

ucmd.args(&["-cf", "archive.tar", "file1.txt", "file2.txt", "file3.txt"])

.succeeds();

// Remove originals

at.remove("file1.txt");

at.remove("file2.txt");

at.remove("file3.txt");

// Extract

new_ucmd!()

.arg("-xf")

.arg(at.plus("archive.tar"))

.current_dir(at.as_string())

.succeeds();

// Verify all contents are identical

assert_eq!(at.read("file1.txt"), "content one");

assert_eq!(at.read("file2.txt"), "content two");

assert_eq!(at.read("file3.txt"), "content three");

}

cakebaker · 2025-11-24T16:14:09Z

tests/by-util/test_tar.rs

+}
+
+#[test]
+fn test_roundtrip_directory_structure() {


Same here, this test is more or less the same as test_extract_directory_structure, with more files and directories.

cakebaker · 2025-11-24T16:16:20Z

tests/by-util/test_tar.rs

+    at.write("empty1.txt", "");
+    at.write("empty2.txt", "");


For empty files you can use touch:

Suggested change

at.write("empty1.txt", "");

at.write("empty2.txt", "");

at.touch("empty1.txt");

at.touch("empty2.txt");

cakebaker · 2025-11-24T16:25:36Z

tests/by-util/test_tar.rs

+}
+
+#[test]
+fn test_roundtrip_special_characters_in_names() {


I'm not sure about this test, the chosen "special characters" look rather "normal" to me. I would have expected some file names with spaces or non-UTF-8 characters ;-)

cakebaker · 2025-11-24T16:30:17Z

tests/by-util/test_tar.rs

+    // Make directory read-only
+    let perms = fs::Permissions::from_mode(0o444);
+    fs::set_permissions(at.plus("readonly"), perms).unwrap();


An alternative is set_readonly:

Suggested change

// Make directory read-only

let perms = fs::Permissions::from_mode(0o444);

fs::set_permissions(at.plus("readonly"), perms).unwrap();

at.set_readonly("readonly");

cakebaker · 2025-11-24T16:36:02Z

tests/by-util/test_tar.rs

+#[test]
+fn test_verbose_output_format_matches_gnu() {
+    let (at, mut ucmd) = at_and_ucmd!();
+
+    at.write("file1.txt", "content");
+    at.write("file2.txt", "content");
+
+    let result = ucmd
+        .args(&["-cvf", "archive.tar", "file1.txt", "file2.txt"])
+        .succeeds();
+
+    let stdout = result.stdout_str();
+
+    // Verify verbose output contains filenames
+    assert!(stdout.contains("file1.txt"));
+    assert!(stdout.contains("file2.txt"));
+}


This test is more or less the same as test_create_verbose, with the only difference that this test uses two files.

Suggested change

#[test]

fn test_verbose_output_format_matches_gnu() {

let (at, mut ucmd) = at_and_ucmd!();

at.write("file1.txt", "content");

at.write("file2.txt", "content");

let result = ucmd

.args(&["-cvf", "archive.tar", "file1.txt", "file2.txt"])

.succeeds();

let stdout = result.stdout_str();

// Verify verbose output contains filenames

assert!(stdout.contains("file1.txt"));

assert!(stdout.contains("file2.txt"));

}

cakebaker · 2025-11-24T16:40:58Z

tests/by-util/test_tar.rs

+#[test]
+fn test_extract_verbose_shows_all_files() {
+    let (at, mut ucmd) = at_and_ucmd!();
+
+    // Create archive with multiple files
+    at.write("file1.txt", "content1");
+    at.write("file2.txt", "content2");
+    at.write("file3.txt", "content3");
+
+    ucmd.args(&["-cf", "archive.tar", "file1.txt", "file2.txt", "file3.txt"])
+        .succeeds();
+
+    at.remove("file1.txt");
+    at.remove("file2.txt");
+    at.remove("file3.txt");
+
+    // Extract with verbose
+    let result = new_ucmd!()
+        .arg("-xvf")
+        .arg(at.plus("archive.tar"))
+        .current_dir(at.as_string())
+        .succeeds();
+
+    let stdout = result.stdout_str();
+
+    // Verify all files are listed in output
+    assert!(stdout.contains("file1.txt"));
+    assert!(stdout.contains("file2.txt"));
+    assert!(stdout.contains("file3.txt"));
+}


This test is more or less the same as test_extract_verbose, but with more files. I would merge the tests.

cakebaker reviewed Nov 24, 2025

View reviewed changes


		## Testing

		The tar application has a focused testing philosophy that separates concerns between the application (CLI interface, error handling, user experience) and the underlying tar-rs library (archive format correctness, encoding, permissions).


		The `tar` utility is built on top of the `tar-rs` library. Because of this, we split our testing into two distinct areas:

		1. The Library (`tar-rs`): This is where we test the nitty-gritty details of the tar format. If you want to verify that permissions are preserved correctly, that long paths are handled according to the UStar spec, or that unicode filenames are encoded properly, those tests belong in `tar-rs/tests/`.

	.code_is(1) // TODO: align with GNU tar by returning exit code 64
	.code_is(1) // TODO: align with GNU tar by returning exit code 2

	.code_is(1) // TODO: propagate usage exit code 64 once empty archive handling is fixed
	.code_is(1) // TODO: propagate usage exit code 2 once empty archive handling is fixed

		let (_at, mut ucmd) = at_and_ucmd!();

		ucmd.args(&["-cf", "archive.tar", "nonexistent.txt"])

		std::fs::remove_dir(at.plus("testdir/subdir")).unwrap();
		std::fs::remove_dir(at.plus("testdir")).unwrap();

Add test documentation and expand CLI test coverage #46

Are you sure you want to change the base?

Add test documentation and expand CLI test coverage #46

Uh oh!

Conversation

kaladron commented Nov 23, 2025

Uh oh!

kaladron commented Nov 23, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cakebaker Nov 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cakebaker Nov 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

cakebaker Nov 24, 2025 •

edited

Loading

cakebaker Nov 24, 2025 •

edited

Loading