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

test: use T.Setenv to set env vars in tests #43517

Merged
merged 1 commit into from
Apr 28, 2022
Merged

test: use T.Setenv to set env vars in tests #43517

merged 1 commit into from
Apr 28, 2022

Conversation

Juneezee
Copy link
Contributor

- What I did
A testing cleanup.

This PR replaces os.Setenv with t.Setenv. Starting from Go 1.17, we can use t.Setenv to set environment variable in test. The environment variable is automatically restored to its original value when the test and all its subtests complete.

Reference: https://pkg.go.dev/testing#T.Setenv

- How I did it

Search for usages of os.Setenv in *_test.go using GoLand, and replace them with t.Setenv accordingly.

- How to verify it

All tests should continue to pass, except those that are already failing before this PR.

- Description for the changelog

test: use `T.SetEnv` to set env vars in tests

- A picture of a cute animal (not mandatory but encouraged)

This commit replaces `os.Setenv` with `t.Setenv` in tests. The
environment variable is automatically restored to its original value
when the test and all its subtests complete.

Reference: https://pkg.go.dev/testing#T.Setenv
Signed-off-by: Eng Zer Jun <engzerjun@gmail.com>
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@cpuguy83 cpuguy83 merged commit b3332b8 into moby:master Apr 28, 2022
Copy link
Member

@tianon tianon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems fine to me too -- it will potentially cause issues with some packagers who will have to backport Go 1.17 if they want to backport newer Docker releases, but I don't think there's much more we can do (reasonably) to help there.

@thaJeztah
Copy link
Member

Thanks @Juneezee !

As Tianon mentioned in the previous comment; we discussed this change in our maintainers call, and agreed that it's ok to make these changes (and it's nice to use more of Go's built-in functionality).

As a follow-up we should have a look at existing uses of gotest.tools env.Patch(), which can now be replaced by t.SetEnv() (and has been deprecated recently in favour of that); https://github.com/gotestyourself/gotest.tools/blob/9c6ef764e11bed6bc0d9455a775d8cac665e1d1d/env/env.go#L18-L26

(We'd still need to keep / want gotest.tools env.PatchAll())

Let me know if you're interested to work on that!

@Juneezee
Copy link
Contributor Author

Thanks @Juneezee !

As Tianon mentioned in the previous comment; we discussed this change in our maintainers call, and agreed that it's ok to make these changes (and it's nice to use more of Go's built-in functionality).

As a follow-up we should have a look at existing uses of gotest.tools env.Patch(), which can now be replaced by t.SetEnv() (and has been deprecated recently in favour of that); https://github.com/gotestyourself/gotest.tools/blob/9c6ef764e11bed6bc0d9455a775d8cac665e1d1d/env/env.go#L18-L26

(We'd still need to keep / want gotest.tools env.PatchAll())

Let me know if you're interested to work on that!

@thaJeztah Thanks for accepting the PR!

Sure, I'd love to work on replacing env.Patch() as well

@thaJeztah
Copy link
Member

oh! I just realised I was touching much of those in #43340, so perhaps I should just include that change in that one 😅

@thaJeztah thaJeztah added this to the 22.06.0 milestone Aug 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants