envvar_test TestConfiguration_Get questions

[wcwynn]

Thanks for this example and the supporting blog posts and videos -- its great and I really appreciate these resources you're providing.

I have a couple very minor questions about envvar_test.go.

The tests seems to use fixture data here which raises the following questions:

1. why clearing ENVVAR_OK in teardown here?
2. why clearing ENVVAR_OK1 in teardown here?
3. why clearing ENVVAR_OK1_SECURE and ENVVAR_ERR_SECURE with Setenv() instead of Unsetenv() here and here? (Perhaps it doesn't matter much but if the intention is to restore the env before the test was run it might be safer to LookupEnv() during setup and then in teardown call Setenv or Unsetenv as appropriate)

As it is, the code seems slightly inconsistent and its unclear if the intention is to use/rely on the fixtures data or not.

Perhaps it would be clearer to either:

1. Always rely on the fixture data and eliminate the code that sets/unsets environment variables in setup/teardown for each test.
2. Eliminate the use of the fixture data and always set/unset the relevant environment variables in setup/teardown

What do you think?

(It also seems the return value from loading the fixtures data is ignored... for my experiments, i was missing the fixtures data but i didn't realize it for a while 😅)

It may be clearer to just eliminate the use of fixture data for the TestConfiguration_Get (and someday add a separate test for the Load).

[MarioCarrion]

Hello @wcwynn!

So the answer to all your 3 questions: when creating tests that involve leaving some sort of state I like restoring that to the original system value; however you're right I should have used os.LookupEnv instead.

With that being said, I'm glad you reached out because what I did right here does not make sense anymore if Go 1.17 is being used, so I updated the code to use the new t.Setenv method, take a look when you have some free time, I think it's cleaner this way.

Best!

[wcwynn]

Hi @MarioCarrion thanks for the explanation and code update to use t.Setenv -- it makes sense! The only minor drawback is that parallel tests are no longer supported but I agree its cleaner this way.

Thanks for quickly taking a look and sharing about this feature of Go 1.17.

[MarioCarrion]

You're welcome, and yes not using parallel is the tradeoff but I guess that's fine for better readability. Take care.