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

A version of Parser::parse_from that reads a mock of the environment, for use in testing #5104

Closed
2 tasks done
cbeck88 opened this issue Aug 30, 2023 · 3 comments
Closed
2 tasks done
Labels
C-enhancement Category: Raise on the bar on expectations

Comments

@cbeck88
Copy link

cbeck88 commented Aug 30, 2023

Please complete the following tasks

Clap Version

4.3

Describe your use case

I have been using clap for years, and I love it, thank you.

Frequently the pattern with which I use it looks something like this:

#[derive(Clone, Debug, Parser, Serialize)]
pub struct Config {
    #[clap(long, env, default_value = "60", value_parser = parse_duration_in_seconds)]
    pub postgres_idle_timeout: Duration,

    #[clap(long, env, default_value = "120", value_parser = parse_duration_in_seconds)]
    pub postgres_max_lifetime: Duration,

    #[clap(long, env, default_value = "5", value_parser = parse_duration_in_seconds)]
    pub postgres_connection_timeout: Duration,

    #[clap(long, env, default_value = "3")]
    pub postgres_max_connections: u32,
}

impl Default for SqlConnectionConfig {
    fn default() -> Self {
        Self {
            postgres_idle_timeout: Duration::from_secs(60),
            postgres_max_lifetime: Duration::from_secs(120),
            postgres_connection_timeout: Duration::from_secs(5),
            postgres_max_connections: 3,
        }
    }
}

That is, I usually derive Parser and specify defaults for everything, and then separately implement Default myself.

Then, in tests that require the config, I usually use Default to create the test object, or I use the ..Default::default() syntax:

Config {
  postgres_max_connections: 1,
  ..Default::default()
}

This helps me avoid touching all the tests when a new option is added which doesn't affect most of them.

This is less DRY than skipping impl Default and just using Parser::parse_from in these tests to get default or nearly default configs. But that has the problem that it reads from the environment, and so if a developer sets one of these vars and then runs cargo test, the test may run differently or fail, which can be hard to track down.

When I skip the Parser machinery and use Default::default, then I can write tests that are isolated from the enviornment.
However, it is not very dry that I usually end up writing all the defaults out twice, once in derive(Parser) and once in impl Default, so that feels a bit silly.

In another case I ran into, I really did want to write tests of argument parsing:

        // Should work
        Config::try_parse_from([
            "",
            "--smtp-host=smtp.gmail.com",
            "--smtp-username=alice",
            "--smtp-password=hunter42",
        ])
        .unwrap();
        // Should fail because of group requirements
        assert!(Config::try_parse_from([
            "",
            "--smtp-host=smtp.gmail.com",
            "--smtp-username=alice",
        ])
        .is_err());

In this case, I do want to test that I set up the config group properly and it works as expected, but afaict there's no way for me to test this in a way that's isolated from the environment. And so these tests can start failing if one of these env is set, which a developer might easily do for local testing.

This can be worked around of course by unsetting env before parsing:

        // Prevent env from messing with us
        std::env::remove_var("SMTP_HOST");
        std::env::remove_var("SMTP_USERNAME");
        std::env::remove_var("SMTP_PASSWORD");
        std::env::remove_var("DEV_CONFIG");

But this feels dirty, because now I'm manipulating global state in a bunch of tests that are running in parallel, and conceivably they could conflict with eachother. (In my actual use-cases, that is unlikely and this is a fine work around.)

Describe the solution you'd like

One solution that would solve all my problems would be if there were a new function in trait Parser, something like

parse_from_with_env<I, T>(iter: I, env: HashMap<OsString, String>) - Self
    where I: ...,
          T: ...

which would work like parse_from, but refer to my hashmap instead of calling std::env::var.

Then I could write tests that don't implicitly read a bunch of environment variables that I care about.

If this existed, I would also likely stop implementing Default on my parsers, and just use parse_from_with_env(["", "my-test", "--param=foo"], Default::default()) when I need to construct configs in tests.

Alternatives, if applicable

Another approach, which wouldn't solve all of my problems, but would help me be more DRY, would be:

Make a way to use clap-derive to derive Default, based on the default-value that are set.

For example it would be cool if this worked:

#[derive(Clone, Debug, Parser, Serialize)]
#[clap(DeriveDefault)]
pub struct Config {
    #[clap(long, env, default_value = "60", value_parser = parse_duration_in_seconds)]
    pub postgres_idle_timeout: Duration,

    #[clap(long, env, default_value = "120", value_parser = parse_duration_in_seconds)]
    pub postgres_max_lifetime: Duration,

    #[clap(long, env, default_value = "5", value_parser = parse_duration_in_seconds)]
    pub postgres_connection_timeout: Duration,

    #[clap(long, env, default_value = "3")]
    pub postgres_max_connections: u32,
}

and produced the impl of Default for Config that I was going to write, which was based on taking the default_value for each parameter, and running it through the value_parser. (Ideally it wouldn't also look at the os environment.)

Additional Context

No response

@cbeck88 cbeck88 added the C-enhancement Category: Raise on the bar on expectations label Aug 30, 2023
@epage
Copy link
Member

epage commented Aug 30, 2023

Looks like we have #4607 for providing a new source of environment variables. While the use case there is for systems without and environment (wasm), I think enough applies that I'm going to go ahead and close this.

Feel free to add any extra notes you want to that issue to explain your use case!

@epage epage closed this as not planned Won't fix, can't repro, duplicate, stale Aug 30, 2023
@cbeck88
Copy link
Author

cbeck88 commented Aug 30, 2023

thank you, I missed that ticket

@epage
Copy link
Member

epage commented Aug 30, 2023

Easy to do when we have as many as we do

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: Raise on the bar on expectations
Projects
None yet
Development

No branches or pull requests

2 participants