Panic catching and testing review

For a while, I am shy to share my "work". So my signal to noise in this group is not so high. Then I read replies saying things positive "Don't worry about sharing advice as a beginner." This motivates me to be more open. So please look at my code. Give it review. Be honest please.

This is part of wrapper I have so I don't need to write main() every time I write a program. My programs get command line power and more but just look like.

fn program(options: &Opt) -> Result<(), ()> {}

Anyway, here is part of my code. Please review.

pub fn args<T: clap::Parser>() -> Result<T, return_code::ReturnCode> {
    match std::panic::catch_unwind(T::try_parse) {
        Ok(result) => match result {
            Ok(result) => Ok(result),
            Err(e) => {
                eprintln!("{}", e);
                Err(return_code::PARSE_ARG_ERROR)
            }
        },
        Err(e) => {
            eprintln!("Panic reading command line arguments{:?}", e);
            Err(return_code::PARSE_ARG_PANIC)
        }
    }
}

#[allow(unused_variables)]
#[cfg(test)]
mod tests {
    use super::*;

    #[test]
    fn args_gives_struct() {
        #[derive(clap::Parser, Debug)]
        struct Opt {
            #[clap(flatten)]
            #[cfg(feature = "nomain_control")]
            nomain_controls: control::ControlOptions,
        }

        if let Ok(options) = args::<Opt>() {
            assert!(true);
        } else {
            assert!(false);
        }
    }

    #[test]
    fn args_gives_error() {
        #[derive(clap::Parser, Debug)]
        struct Opt {
            #[clap(long)]
            word: String,
            #[clap(flatten)]
            #[cfg(feature = "nomain_control")]
            nomain_controls: control::ControlOptions,
        }
        if let Err(err) = args::<Opt>() {
            assert_eq!(err, return_code::PARSE_ARG_ERROR);
        } else {
            assert!(false);
        }
    }

    #[test]
    fn args_gives_panic() {
        #[derive(clap::Parser, Debug)]
        struct Opt {
            first: Vec<String>,
            second: Vec<String>,
            #[clap(flatten)]
            #[cfg(feature = "nomain_control")]
            nomain_controls: control::ControlOptions,
        }
        if let Err(err) = args::<Opt>() {
            assert_eq!(err, return_code::PARSE_ARG_PANIC);
        } else {
            assert!(false);
        }
    }
}

This looks like quite a useful piece of code. Especially your little return_code module for common return codes.

Before I add my comments, I'd like to hear more about the decisions that went into this and how the resulting code would feel...

  1. Can you give an example of the main() function that would call this?
  2. Is it common for clap::Parser implementations to panic? I thought clap would be pretty resilient to that sort of thing because it's designed to process unsanitised user input
  3. What benefit would this program() approach have over just returning a Result<(), SomeError> from main()? I can think of some really good reasons, but I'm curious to hear your response
  4. How many applications have you used this in? Does it still fit all your needs, or have you found awkward parts of your design?
  5. How does clap's custom derive feel compared to structopt? I've always used structopt and got excited when I saw clap was making their own custom derive, but haven't had a chance to try it out yet
  6. You are handling panics in the clap::Parser implementation, so does that mean you also wrap the rest of your application's business logic in std::panic::catch_unwind()?

A neat trick to avoid the rightward drift from match Ok(result) => match result is to do all the pattern matching in the one match statement.

match std::panic::catch_unwind(T::try_parse) {
  Ok(Ok(result)) => Ok(result),
  Ok(Err(parse_error)) => {
    eprintln!("{}", parse_error);
    Err(return_code::PARSE_ARG_ERROR)
  },
  Err(panic_error) => {
    eprintln!("Panic reading command line arguments {:?}", panic_error);
    Err(return_code::PARSE_ARG_PANIC)
  }
  }
}

This test feels a little awkward.

    #[test]
    fn args_gives_struct() {
        #[derive(clap::Parser, Debug)]
        struct Opt {
            #[clap(flatten)]
            #[cfg(feature = "nomain_control")]
            nomain_controls: control::ControlOptions,
        }

        if let Ok(options) = args::<Opt>() {
            assert!(true);
        } else {
            assert!(false);
        }
    }

I would probably get rid of the redundant pattern matching and use unwrap() directly because the if Ok(...) = ... { assert!(true) } else { assert!(false) } feels a lot like the if condition { return true } else { return false } memes you see around the place.
You probably also want to check that it parses to the value you expect.

So maybe something like this:

#[test]
fn args_gives_struct() {
  #[derive(clap::Parser, Debug)]
  struct Opt { ... }

  let options: Opt = args().unwrap();

  assert_eq!(options, Opt { ... });
}

As written, your test is also going to parse the arguments given to your test runner, which probably isn't what you want to do (arguments can't be controlled by your test, you can't test different sets of arguments, etc.).

Instead, you can write a parse_args() function which accepts a Vec<String> for the command-line arguments so you can provide a specially crafted set of arguments to the tests (we can add generics to parse_args() to make tests easier to write). That lets you write tests that say "given these arguments, I expect this Opts value".

You would then write the current args() in terms of parse_args() by just collecting the arguments from std::env::args() into a vector and passing them to parse_args().


You can do the same thing with the args_gives_error() test. Instead of the if-let and assert!(false), I would do something like

let err = args::<Opt>().unwrap_err();
assert_eq!(err, return_code::PARSE_ARG_ERROR);

there are a couple reasons why I prefer this form

  • tests are checking a single logical thing, so they should be straight-line code with no branches
  • using unwrap_err() is a lot more easier to debug than assert!(false) - false gives us the useless error message, "assertion failed: false", while unwrap_err() will say "called Result::unwrap_err() on an Ok value: Ok(Opt { ... })".
  • I think assert!(false) looks silly :stuck_out_tongue:
1 Like

Thank you for the positive. I will share all when I finish the long tedious process of writing more tests.

Before I open my editor and look at your suggestions I will answer your questions.

Hmm, 1. Have a macro that takes the program function name and the struct type as arguments. It puts the "program' in the wrapper that is to ugly to share yet. The macro makes a main that looks like this.

fn main() {
            std::process::exit(nomain::nomain(nomain::args(), $program));
}

I do not know. I can make it panic if I create more than one positional Vec. It was easy for me to get clap to panic when I tried to learn how to use it. So I put a panic catcher in that part just to see if I could. Note: My comments will specify that the panic might not catch.

I don't know what SomeError is. I saw a post before but can only read so much and I do not fully groke the std yet.
You can return custom numbers is the simple no feature use. The "and more" part is features.
Many not implemented yet because I am stopping any more features until I finish test and clean ups for rest of code. And comments. Then more features will come. dryrun and runtimes are woking. I think pretty easy, the hardest issues I have are writing comments and doc test. Now got the args test done and figured I get advice before I continue. Anway, features. Dry run mode with passing dryrun to to program so it can do dry run, or not running the program at all. Run_time (multiple runs: #of times, repeat until Ok or Err or Ok(num), or Err(num) and repeat while, Super power of changing program arguments ( run with filename = ["file2", "file.fred", "..."], etc. )
For example a simple stupid doubling program if enabled with dry_run and run_times.

nomain::write_nomain!(double, Opt);

/// Twice as good.
#[derive(clap::Parser, Debug)]
#[clap(name = "double", about = "Double the fun.")]
struct Opt {
    /// Give me a number
    #[clap(name = "goodness")]
    number: i32,
    /// The following two lines are required if any features used
    #[clap(flatten)]
    nomain_controls: nomain::control::ControlOptions,
}

/// Double program.
fn double(options: &Opt) -> Result<(), ()> {
    match options.nomain_controls.dry_run_level() {
        Some(nomain::control::DryRun::Pass) => println!("Not doubling {}.", options.number),
        _ => println!("{}", options.number * 2),
    }
    Ok(())
}

and then in my shell

$: cargo b
$: ./target/debug/double --nomain-run-times 3 22
42
42
42
$: ./target/debug/double --nomain-dry-run 22
Not doubling 22.
$:

Only what needed to check this thing. Three of four as big as the double example I post here. Example program type testing. (newbie integration)
I hate command parsing and error handling and all that stuff. I just want to write programs that make patterns for myself to look at. But when I see a hurdle in that I can create an image fairly easy with math, if I want to change the number and make another one, I need to write shell scripts or whatever. As I made another pattern and changed the code, I realized I should overcome my fear of using crates and see about parsing arguments. Also I have forced myself to do this and do it complete so I am fortifying my git skills also. branches for comments and testing and such.

I started to learn StructOpt because it is "recommended" but as I struggled to figure it out and found some thread where it was joining clap, i just changed over before I know how to use StructOpt fully. I still don't know clap about parsing...

To be honest and forefront I have done nothing with ? yet. But that wasn't what you asked.
No I do not yet wrap the program logic in catch_unwind (yet) because I am still wondering if my methods are sane.

I will read your suggestions, try to understand them, respond to them and put what I like in my code and get back here later.

This part very much

#[cfg(feature = "nomain_control")]
nomain_controls: control::ControlOptions,

I don't want to put that in my struct. I want it automagic from my macro but havent figured out how. Also I only want the controls when they are needed hence the feature nomain_control. But because that feature is used in my library and that line is in the macro that my binaries call, the binaries also must make and enable a feature with the same name for themselves. Hence the feature name is prefixed with nomain. That part is awkward. But... the more I look at it I am glad it happened kind of because if a program does not plan on using any features then leave those lines out and if a program does use any features then those lines are a visible clue that you can use features.

Thanks, I am familiar with that, but find "my way" easier to see. As I am trying to be more aware and able to read others style, I will include your suggestion and get myself use to seeing it that way.

Yes, I agree. I worked around before and used assert_eq! but then I need some derives added to my struct and my library too. Also I am making my test only checking if the args() function works. Not what is contained in returned struct. So just testing if the stuct of the correct type is returned. As the assert!(false) part is for sure awkward, I'll change it to

    let options: Opt = args().unwrap();

Sweet.

Oh, is that it. I though my code was way worse and would have dozens of places that were stupid.

1 Like

Yeah, it's personal preference. I tend to prefer code with minimal nesting and rightward drift because it tends to be simpler.

In a previous workplace, I inherited code from someone who would only put a return at the end of the function and all variable declarations at the top. He also had a tendency to write 500+ line code monsters, so you would routinely have 5 or more levels of if-statements and unreadable garbage.

After that experience, I gained a newfound appreciation for functions that have low computational complexity and can fit on a single screen.

Oh, that reminds me.

It's not really an issue in tests, but using #[allow(unused_variables)] is a code smell (or disabling most lints, for that matter). You might want to write things like let _: Opt = args().unwrap() instead.

To me, it indicates that you wrote some code then forgot to do something with it. Possibly indicating unused results which should be discarded or unfinished work.

Thank you. I agree, it smells there, and I did forget that one unused variable needed an underline. I just started using #[allow(unused_variables)] in another place where it "made sense" to use and did not smell because the variable may or may not exist and depending on features used may exist and not be used.

           #[allow(unused_variables)]
            #[cfg(feature = "nomain_control")]
            let control_options = options.control_options();

But I see now (this smells too) that the warning I am suppressing here is a warning I want shown and should not be suppressed. So I will remove it there too. I put it there to stop a warning I only get when I do wrong things enabling a feature that is not used. And that warning is 'good"

1 Like