add a test case that calls the default_fmt_val method

[szabgab]

No description provided.

[ahmadbky]

Hi, thanks for contributing.

Regarding the test relevance, I think this one looks more like an integration test, since it doesn't test only the default_val_fmt method, but also makes calls to methods like define(), init() and get(), and these should be tested in a separate unit.

Also, the tests seem more related to the make_config! macro, so they should be placed in the macros module.

Moreover, the declaration of the struct MyConfig should be made in the body of the test function, for better clarity (and avoid already taken identifier if having multiple test functions).

[ahmadbky]

Move the test of the init() and get() calls into a separate unit.

Also, prefer using the with_env function from the tests module, to setup the $MY_URL env var, for more consistency with the other unit tests.

Make sure to use catch_unwind since the calls to init() and get() may panic.

[ahmadbky]

Instead of overriding the config here, make another unit test in a separate function that calls the make_config! macro, which specifies default_val_fmt, and test the resulting descriptor.

You could also make this to test description, and make some tweaks between it and default_val_fmt.

[szabgab]

IMHO integration tests are good as that make it easier for a reader of the code to see how can it be used.

[ahmadbky]

IMHO integration tests are good as that make it easier for a reader of the code to see how can it be used.

You're right, we definitely should add integration tests. I am just nitpicky because I find the test relevant, because it affects more what the macro generates, than what the generated code executes in behind.

However IMHO, such test makes more sense as unit tests than integration tests. We could do both, I'm just talking about testing default_fmt_val and description specifically.

That said, if you think this would make too many changes in this PR, you can move the test in a new tests folder at the root, so that it really becomes an integration test.

[szabgab]

I tried to move it to a new tests folder and got error messages:

error[E0616]: field `description` of struct `VarDescriptor` is private
  --> tests/one.rs:24:48
   |
24 |         assert_eq!(config.url.get_descriptor().description, Some("Some URL"));
   |                                                ^^^^^^^^^^^ private field

error[E0616]: field `default_val_fmt` of struct `VarDescriptor` is private
  --> tests/one.rs:26:48
   |
26 |         assert_eq!(config.url.get_descriptor().default_val_fmt, None);
   |                                                ^^^^^^^^^^^^^^^ private field

error[E0616]: field `default_val_fmt` of struct `VarDescriptor` is private
  --> tests/one.rs:29:41
   |
29 |         assert_eq!(url.get_descriptor().default_val_fmt, Some("hi"));
   |                                         ^^^^^^^^^^^^^^^ private field

error[E0616]: field `description` of struct `VarDescriptor` is private
  --> tests/one.rs:30:41
   |
30 |         assert_eq!(url.get_descriptor().description, Some("Some URL"));
   |                                         ^^^^^^^^^^^ private field

If I understand correctly this is because both of the description and default_val_fmt attributes are only public in the crate. pub(crate). see

Am I misunderstanding something?
Would you want to make them pub ?

As I understand default_fmt_val is the setter of default_val_fmt (and btw the difference between the names is a bit confusing) so maybe instead of making those attributes public you'll want to add getters to them?

Alternatively the tests could use the format! macro and then no need to change the code:

assert_eq!(format!("{}", config.url.get_descriptor()), "`MY_URL`: Some URL");

and

assert_eq!(format!("{}", url.get_descriptor()), "`MY_URL`: Some URL (default: hi)");

I am updating the PR in that direction.

[ahmadbky]

You were right about the visibility of the fields description and default_val_fmt. We'll see in the future if they should be turned into pub instead.

And yes, I agree on the fact that it's a bit confusing that default_fmt_val is the setter of the default_val_fmt field, the latter should be renamed to maintain consistency.