Add test documentation and expand CLI test coverage (#46)

* Add test documentation and expand CLI test coverage

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.

* Align exit codes with GNU tar and modernize tests

- Refactor `uumain` to manually handle `clap` errors, ensuring exit code 64 for command-line syntax errors and exit code 2 for semantic/fatal errors.
- Add mutual exclusion between `--create` and `--extract` operations.
- Update `README.md` and `tests/README.md` with corrected exit code documentation and clarified testing philosophy.
- Modernize `tests/by-util/test_tar.rs`:
    - Define `TAR_BLOCK_SIZE` constant (512 bytes).
    - Use `uutests` convenience methods (`at.touch()`, `at.rmdir()`, etc.).
    - Improve `test_verbose` to use `PathBuf`.
    - Replace `.no_stderr()` with `.no_output()`.
    - Implement a root-user check in `test_create_permission_denied`.
    - Remove redundant or outdated roundtrip and verbose format tests.

* Add missing [test] annotation.

---------

Co-authored-by: Sylvestre Ledru <sylvestre.ledru@gmail.com>

README.md

@@ -22,6 +22,20 @@ cargo build --release
 cargo run --release
 ```
 
+## 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).
+
+See [tests/README.md](tests/README.md) for comprehensive documentation.
+
+```bash
+# Run all tests
+cargo test --all
+
+# Run specific test
+cargo test test_create_single_file
+```
+
 ## License
 
 tar is licensed under the MIT License - see the `LICENSE` file for details

src/uu/tar/src/tar.rs

@@ -31,14 +31,39 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> {
         args_vec
     };
 
-    let matches = uu_app().try_get_matches_from(args_to_parse)?;
+    let matches = match uu_app().try_get_matches_from(args_to_parse) {
+        Ok(matches) => matches,
+        Err(err) => {
+            let kind = err.kind();
+            match kind {
+                clap::error::ErrorKind::DisplayHelp | clap::error::ErrorKind::DisplayVersion => {
+                    let _ = err.print();
+                    return Ok(());
+                }
+                _ => {
+                    let code = match kind {
+                        clap::error::ErrorKind::ArgumentConflict => 2,
+                        clap::error::ErrorKind::UnknownArgument
+                        | clap::error::ErrorKind::MissingRequiredArgument
+                        | clap::error::ErrorKind::TooFewValues
+                        | clap::error::ErrorKind::TooManyValues
+                        | clap::error::ErrorKind::WrongNumberOfValues => 64,
+                        clap::error::ErrorKind::InvalidValue
+                        | clap::error::ErrorKind::ValueValidation => 2,
+                        _ => 2,
+                    };
+                    return Err(uucore::error::USimpleError::new(code, err.to_string()));
+                }
+            }
+        }
+    };
 
     let verbose = matches.get_flag("verbose");
 
     // Handle extract operation
     if matches.get_flag("extract") {
         let archive_path = matches.get_one::<PathBuf>("file").ok_or_else(|| {
-            uucore::error::USimpleError::new(1, "option requires an argument -- 'f'")
+            uucore::error::USimpleError::new(64, "option requires an argument -- 'f'")
         })?;
 
         return operations::extract::extract_archive(archive_path, verbose);
@@ -47,7 +72,7 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> {
     // Handle create operation
     if matches.get_flag("create") {
         let archive_path = matches.get_one::<PathBuf>("file").ok_or_else(|| {
-            uucore::error::USimpleError::new(1, "option requires an argument -- 'f'")
+            uucore::error::USimpleError::new(64, "option requires an argument -- 'f'")
         })?;
 
         let files: Vec<&Path> = matches
@@ -57,7 +82,7 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> {
 
         if files.is_empty() {
             return Err(uucore::error::USimpleError::new(
-                1,
+                2,
                 "Cowardly refusing to create an empty archive",
             ));
         }
@@ -67,7 +92,7 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> {
 
     // If no operation specified, show error
     Err(uucore::error::USimpleError::new(
-        1,
+        2,
         "You must specify one of the '-c' or '-x' options",
     ))
 }
@@ -82,12 +107,14 @@ pub fn uu_app() -> Command {
         .disable_help_flag(true)
         .args([
             // Main operation modes
-            arg!(-c --create "Create a new archive"),
+            arg!(-c --create "Create a new archive").conflicts_with("extract"),
             // arg!(-d --diff "Find differences between archive and file system").alias("compare"),
             // arg!(-r --append "Append files to end of archive"),
             // arg!(-t --list "List contents of archive"),
             // arg!(-u --update "Only append files newer than copy in archive"),
-            arg!(-x --extract "Extract files from archive").alias("get"),
+            arg!(-x --extract "Extract files from archive")
+                .alias("get")
+                .conflicts_with("create"),
             // Archive file
             arg!(-f --file <ARCHIVE> "Use archive file or device ARCHIVE")
                 .value_parser(clap::value_parser!(PathBuf)),

tests/README.md

@@ -0,0 +1,53 @@
+# Testing the tar Application
+
+This directory contains tests for the `tar` command-line utility.
+
+## Philosophy
+
+The `tar` utility is built on top of the `tar-rs` library, which is an external crate we depend on. Because of this, we split our testing into two distinct areas:
+
+1. **The External Library (`tar-rs`)**: This is an independent library that handles 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 the `tar-rs` crate itself.
+
+2. **The Application (`tar`)**: This is where we test the user interface. These tests ensure that the command-line arguments are parsed correctly, that the program exits with the right status codes, and that basic operations like creating and extracting archives actually work from a user's perspective.
+
+## Writing Tests for the Application
+
+When writing tests here, focus on the **user experience**.
+
+* **Do** check that flags like `-c`, `-x`, `-v`, and `-f` do what they say.
+* **Do** check that invalid combinations of flags produce a helpful error message and exit code 2.
+* **Do** check that serious errors (like file not found) return exit code 2.
+* **Do** perform "smoke tests" — create an archive and make sure the file appears; extract an archive and make sure the files come out.
+
+* **Don't** inspect the internal bytes of the archive to verify header fields. Trust that `tar-rs` handles that.
+* **Don't** write complex tests for edge cases in file system permissions or encoding, unless they are specifically related to a CLI flag.
+
+### Example
+
+If you are testing that `tar -cf archive.tar file.txt` works:
+
+* **Good**: Run the command, assert it succeeds (exit code 0), and assert that `archive.tar` exists on disk.
+* **Bad**: Run the command, open `archive.tar` with a library, parse the headers, and assert that the checksum is correct.
+
+## Running Tests
+
+You can run these tests just like any other Rust project:
+
+```bash
+cargo test --all
+```
+
+To run a specific test:
+
+```bash
+cargo test test_name
+```
+
+## Exit Codes
+
+We follow GNU tar conventions for exit codes:
+
+* **0**: Success.
+* **1**: Some files differ (used in compare mode, not yet implemented).
+* **2**: Fatal error (file not found, permission denied, conflicting options like `-c -x`, invalid option values).
+* **64**: Command line usage error (unknown option, missing required argument, wrong number of values for an option).

tests/by-util/test_tar.rs

@@ -7,11 +7,20 @@ use std::path::{self, PathBuf};
 
 use uutests::{at_and_ucmd, new_ucmd};
 
-// Basic CLI Tests
+/// Size of a single tar block in bytes (per POSIX specification).
+// TODO: This should be exported from the tar crate instead of being redefined here.
+// The tar crate has `BLOCK_SIZE` defined but it's marked `pub(crate)`.
+const TAR_BLOCK_SIZE: usize = 512;
+
+// Basic CLI tests
 
 #[test]
 fn test_invalid_arg() {
-    new_ucmd!().arg("--definitely-invalid").fails().code_is(1);
+    new_ucmd!()
+        .arg("--definitely-invalid")
+        .fails()
+        .code_is(64)
+        .stderr_contains("unexpected argument");
 }
 
 #[test]
@@ -32,10 +41,30 @@ fn test_version() {
         .stdout_contains("tar");
 }
 
+#[test]
+fn test_conflicting_operations() {
+    new_ucmd!()
+        .args(&["-c", "-x", "-f", "archive.tar"])
+        .fails()
+        .code_is(2)
+        .stderr_contains("cannot be used with");
+}
+
+#[test]
+fn test_no_operation_specified() {
+    new_ucmd!()
+        .args(&["-f", "archive.tar"])
+        .fails()
+        .code_is(2)
+        .stderr_contains("must specify one");
+}
+
+// Create operation tests
 #[test]
 fn test_create_dir_verbose() {
     let (at, mut ucmd) = at_and_ucmd!();
 
+    // TODO(jeffbailey): Use PathBuf here instead
     let separator = path::MAIN_SEPARATOR;
     let dir1_path = "dir1";
     let dir2_path = format!("{dir1_path}{separator}dir2");
@@ -55,17 +84,18 @@ fn test_create_dir_verbose() {
         .stdout_contains(file2_path);
 }
 
-// Create operation tests
-
 #[test]
 fn test_create_single_file() {
     let (at, mut ucmd) = at_and_ucmd!();
 
     at.write("file1.txt", "test content");
 
-    ucmd.args(&["-cf", "archive.tar", "file1.txt"]).succeeds();
+    ucmd.args(&["-cf", "archive.tar", "file1.txt"])
+        .succeeds()
+        .no_output();
 
     assert!(at.file_exists("archive.tar"));
+    assert!(at.read_bytes("archive.tar").len() > TAR_BLOCK_SIZE); // Basic sanity check
 }
 
 #[test]
@@ -74,11 +104,63 @@ fn test_create_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"])
-        .succeeds();
+    ucmd.args(&["-cf", "archive.tar", "file1.txt", "file2.txt", "file3.txt"])
+        .succeeds()
+        .no_output();
 
     assert!(at.file_exists("archive.tar"));
+    assert!(at.read_bytes("archive.tar").len() > TAR_BLOCK_SIZE); // Basic sanity check
+}
+
+#[test]
+fn test_create_directory() {
+    let (at, mut ucmd) = at_and_ucmd!();
+
+    at.mkdir("dir1");
+    at.write("dir1/file1.txt", "content1");
+    at.write("dir1/file2.txt", "content2");
+    at.mkdir("dir1/subdir");
+    at.write("dir1/subdir/file3.txt", "content3");
+
+    ucmd.args(&["-cf", "archive.tar", "dir1"])
+        .succeeds()
+        .no_output();
+
+    assert!(at.file_exists("archive.tar"));
+    assert!(at.read_bytes("archive.tar").len() > TAR_BLOCK_SIZE); // Basic sanity check
+}
+
+#[test]
+fn test_create_verbose() {
+    let (at, mut ucmd) = at_and_ucmd!();
+
+    at.write("file1.txt", "content");
+
+    ucmd.args(&["-cvf", "archive.tar", "file1.txt"])
+        .succeeds()
+        .stdout_contains("file1.txt");
+
+    assert!(at.file_exists("archive.tar"));
+}
+
+#[test]
+fn test_create_empty_archive_fails() {
+    new_ucmd!()
+        .args(&["-cf", "archive.tar"])
+        .fails()
+        .code_is(2)
+        .stderr_contains("empty archive");
+}
+
+#[test]
+fn test_create_nonexistent_file_fails() {
+    new_ucmd!()
+        .args(&["-cf", "archive.tar", "nonexistent.txt"])
+        .fails()
+        .code_is(2)
+        .stderr_contains("nonexistent.txt");
 }
 
 #[test]
@@ -114,12 +196,49 @@ fn test_extract_single_file() {
         .arg("-xf")
         .arg(at.plus("archive.tar"))
         .current_dir(at.as_string())
-        .succeeds();
+        .succeeds()
+        .no_output();
 
     assert!(at.file_exists("original.txt"));
     assert_eq!(at.read("original.txt"), "test content");
 }
 
+#[test]
+fn test_extract_verbose() {
+    let (at, mut ucmd) = at_and_ucmd!();
+
+    // Create an 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 verbose output
+    assert!(stdout.contains("file1.txt"));
+    assert!(stdout.contains("file2.txt"));
+    assert!(stdout.contains("file3.txt"));
+
+    // Verify files were extracted
+    assert!(at.file_exists("file1.txt"));
+    assert!(at.file_exists("file2.txt"));
+    assert!(at.file_exists("file3.txt"));
+}
+
 #[test]
 fn test_extract_multiple_files() {
     let (at, mut ucmd) = at_and_ucmd!();
@@ -152,7 +271,282 @@ fn test_extract_nonexistent_archive() {
     new_ucmd!()
         .args(&["-xf", "nonexistent.tar"])
         .fails()
-        .code_is(2);
+        .code_is(2)
+        .stderr_contains("nonexistent.tar");
+}
+
+#[test]
+fn test_extract_directory_structure() {
+    let (at, mut ucmd) = at_and_ucmd!();
+
+    // Create directory structure
+    at.mkdir("testdir");
+    at.write("testdir/file1.txt", "content1");
+    at.mkdir("testdir/subdir");
+    at.write("testdir/subdir/file2.txt", "content2");
+
+    // Create archive
+    ucmd.args(&["-cf", "archive.tar", "testdir"]).succeeds();
+
+    // Remove directory contents and directory itself
+    at.remove("testdir/subdir/file2.txt");
+    at.remove("testdir/file1.txt");
+    at.rmdir("testdir/subdir");
+    at.rmdir("testdir");
+
+    // Extract (extracts to current directory)
+    new_ucmd!()
+        .arg("-xf")
+        .arg(at.plus("archive.tar"))
+        .current_dir(at.as_string())
+        .succeeds();
+
+    // Verify structure
+    assert!(at.dir_exists("testdir"));
+    assert!(at.file_exists("testdir/file1.txt"));
+    assert!(at.dir_exists("testdir/subdir"));
+    assert!(at.file_exists("testdir/subdir/file2.txt"));
+    assert_eq!(at.read("testdir/file1.txt"), "content1");
+    assert_eq!(at.read("testdir/subdir/file2.txt"), "content2");
+}
+
+// Round-trip tests
+
+#[test]
+fn test_roundtrip_empty_files() {
+    let (at, mut ucmd) = at_and_ucmd!();
+
+    // Create empty files
+    at.touch("empty1.txt");
+    at.touch("empty2.txt");
+
+    // Create archive
+    ucmd.args(&["-cf", "archive.tar", "empty1.txt", "empty2.txt"])
+        .succeeds();
+
+    // Remove originals
+    at.remove("empty1.txt");
+    at.remove("empty2.txt");
+
+    // Extract
+    new_ucmd!()
+        .arg("-xf")
+        .arg(at.plus("archive.tar"))
+        .current_dir(at.as_string())
+        .succeeds();
+
+    // Verify empty files exist and are still empty
+    assert!(at.file_exists("empty1.txt"));
+    assert!(at.file_exists("empty2.txt"));
+    assert_eq!(at.read("empty1.txt"), "");
+    assert_eq!(at.read("empty2.txt"), "");
+}
+
+// Error handling and exit code tests
+
+#[test]
+#[cfg(unix)]
+fn test_create_permission_denied() {
+    // SAFETY: `libc::geteuid()` is a pure function that returns the effective
+    // user ID of the calling process. It has no side effects and cannot cause
+    // undefined behavior.
+    if unsafe { libc::geteuid() } == 0 {
+        eprintln!("skipping: running as root");
+        return;
+    }
+
+    let (at, mut ucmd) = at_and_ucmd!();
+
+    at.write("file.txt", "content");
+    at.mkdir("readonly");
+
+    // Make directory read-only
+    at.set_readonly("readonly");
+
+    ucmd.args(&["-cf", "readonly/archive.tar", "file.txt"])
+        .fails()
+        .code_is(2)
+        .stderr_contains("readonly/archive.tar");
+
+    // Cleanup - restore permissions so test cleanup can work
+    at.set_mode("readonly", 0o755);
+}
+
+#[test]
+fn test_extract_corrupted_archive() {
+    let (at, mut ucmd) = at_and_ucmd!();
+
+    // Create a corrupted tar file (invalid header)
+    at.write("corrupted.tar", "This is not a valid tar file content");
+
+    ucmd.args(&["-xf", "corrupted.tar"]).fails().code_is(2);
+}
+
+#[test]
+fn test_create_with_dash_in_filename() {
+    let (at, mut ucmd) = at_and_ucmd!();
+
+    // Create files starting with dash
+    at.write("-dash-file.txt", "content with dash");
+    at.write("normal.txt", "normal content");
+
+    ucmd.args(&["-cf", "archive.tar", "--", "-dash-file.txt", "normal.txt"])
+        .succeeds();
+
+    assert!(at.file_exists("archive.tar"));
+
+    // Verify extraction works
+    at.remove("-dash-file.txt");
+    at.remove("normal.txt");
+
+    new_ucmd!()
+        .arg("-xf")
+        .arg(at.plus("archive.tar"))
+        .current_dir(at.as_string())
+        .succeeds();
+
+    assert!(at.file_exists("-dash-file.txt"));
+    assert_eq!(at.read("-dash-file.txt"), "content with dash");
+}
+
+// CLI argument handling tests
+
+#[test]
+fn test_mixed_short_and_long_options() {
+    let (at, mut ucmd) = at_and_ucmd!();
+
+    at.write("file.txt", "content");
+
+    // Test mixing -x with --file
+    ucmd.args(&["-c", "--file=archive.tar", "file.txt"])
+        .succeeds();
+
+    assert!(at.file_exists("archive.tar"));
+
+    at.remove("file.txt");
+
+    // Test extraction with mixed options
+    new_ucmd!()
+        .args(&["-x", "--file", "archive.tar"])
+        .current_dir(at.as_string())
+        .succeeds();
+
+    assert!(at.file_exists("file.txt"));
+}
+
+#[test]
+fn test_option_order_variations() {
+    let (at, mut ucmd) = at_and_ucmd!();
+
+    at.write("file.txt", "content");
+
+    // Test standard -cf order
+    ucmd.args(&["-cf", "archive1.tar", "file.txt"]).succeeds();
+
+    assert!(at.file_exists("archive1.tar"));
+
+    // Test separate options
+    new_ucmd!()
+        .args(&["-c", "-f", "archive2.tar", "file.txt"])
+        .current_dir(at.as_string())
+        .succeeds();
+
+    assert!(at.file_exists("archive2.tar"));
+
+    // Test long form
+    new_ucmd!()
+        .args(&["--create", "--file", "archive3.tar", "file.txt"])
+        .current_dir(at.as_string())
+        .succeeds();
+
+    assert!(at.file_exists("archive3.tar"));
+}
+
+#[test]
+fn test_extract_overwrites_existing_by_default() {
+    let (at, mut ucmd) = at_and_ucmd!();
+
+    // Create original file and archive
+    at.write("file.txt", "original content");
+    ucmd.args(&["-cf", "archive.tar", "file.txt"]).succeeds();
+
+    // Modify the file
+    at.write("file.txt", "modified content");
+
+    // Extract should overwrite
+    new_ucmd!()
+        .arg("-xf")
+        .arg(at.plus("archive.tar"))
+        .current_dir(at.as_string())
+        .succeeds();
+
+    // Verify original content is restored
+    assert_eq!(at.read("file.txt"), "original content");
+}
+
+// Edge case tests
+
+// TODO(jeffbailey): This should move to tar-rs
+#[test]
+fn test_file_with_spaces_in_name() {
+    let (at, mut ucmd) = at_and_ucmd!();
+
+    // Create files with spaces in names
+    at.write("file with spaces.txt", "content 1");
+    at.write("another file.txt", "content 2");
+
+    // Create archive
+    ucmd.args(&[
+        "-cf",
+        "archive.tar",
+        "file with spaces.txt",
+        "another file.txt",
+    ])
+    .succeeds();
+
+    // Remove originals
+    at.remove("file with spaces.txt");
+    at.remove("another file.txt");
+
+    // Extract
+    new_ucmd!()
+        .arg("-xf")
+        .arg(at.plus("archive.tar"))
+        .current_dir(at.as_string())
+        .succeeds();
+
+    // Verify files extracted correctly
+    assert!(at.file_exists("file with spaces.txt"));
+    assert!(at.file_exists("another file.txt"));
+    assert_eq!(at.read("file with spaces.txt"), "content 1");
+    assert_eq!(at.read("another file.txt"), "content 2");
+}
+
+#[test]
+fn test_large_number_of_files() {
+    let (at, mut ucmd) = at_and_ucmd!();
+
+    // Create 100 files
+    let num_files = 100;
+    for i in 0..num_files {
+        at.write(&format!("file{i}.txt"), &format!("content {i}"));
+    }
+
+    // Collect file names for archive creation
+    let files: Vec<String> = (0..num_files).map(|i| format!("file{i}.txt")).collect();
+    let mut args = vec!["-cf", "archive.tar"];
+    args.extend(files.iter().map(String::as_str));
+
+    // Create archive
+    ucmd.args(&args).succeeds();
+
+    // Verify archive was created with reasonable size
+    assert!(at.file_exists("archive.tar"));
+    let archive_size = at.read_bytes("archive.tar").len();
+    assert!(
+        archive_size > TAR_BLOCK_SIZE * num_files,
+        "Archive should contain data for {num_files} files"
+    );
 }
 
 #[test]