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.

Sign up for GitHub

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

Jump to bottom

test: use T.TempDir to create temporary test directory #12853

Merged
merged 3 commits into from
May 12, 2022
Merged

test: use T.TempDir to create temporary test directory #12853

merged 3 commits into from
May 12, 2022

Conversation

Juneezee
Copy link
Contributor

@Juneezee Juneezee commented May 3, 2022 •
edited
Loading

A testing cleanup.

This pull request replaces ioutil.TempDir with t.TempDir. We can use the T.TempDir function from the testing package to create temporary directory. The directory created by T.TempDir is automatically removed when the test and all its subtests complete.

This saves us at least 2 lines (error check, and cleanup) on every instance, or in some cases adds cleanup that we forgot.

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

func TestFoo(t *testing.T) {
	// before
	tmpDir, err := ioutil.TempDir("", "")
	require.NoError(t, err)
	defer require.NoError(t, os.RemoveAll(tmpDir))

	// now
	tmpDir := t.TempDir()
}

Related PR in Terraform: hashicorp/terraform#30803

@Juneezee
test: use T.TempDir to create temporary test directory
b992a99 
This commit replaces `ioutil.TempDir` with `t.TempDir` in tests. The
directory created by `t.TempDir` is automatically removed when the test
and all its subtests complete.

Prior to this commit, temporary directory created using `ioutil.TempDir`
needs to be removed manually by calling `os.RemoveAll`, which is omitted
in some tests. The error handling boilerplate e.g.
	defer func() {
		if err := os.RemoveAll(dir); err != nil {
			t.Fatal(err)
		}
	}
is also tedious, but `t.TempDir` handles this for us nicely.

Reference: https://pkg.go.dev/testing#T.TempDir
Signed-off-by: Eng Zer Jun <engzerjun@gmail.com>
@Juneezee
test: fix TestLogmon_Start_restart on Windows
f910a73 
Signed-off-by: Eng Zer Jun <engzerjun@gmail.com>
@Juneezee
test: fix failing TestConsul_Integration
e8c06b5 
t.TempDir fails to perform the cleanup properly because the folder is
still in use

testing.go:967: TempDir RemoveAll cleanup: unlinkat /tmp/TestConsul_Integration2837567823/002/191a6f1a-5371-cf7c-da38-220fe85d10e5/web/secrets: device or resource busy

Signed-off-by: Eng Zer Jun <engzerjun@gmail.com>
@Juneezee Juneezee marked this pull request as ready for review May 3, 2022 08:55
@tgross tgross self-requested a review May 3, 2022 12:33
tgross
tgross requested changes May 3, 2022
View reviewed changes
Copy link
Member

@tgross tgross left a comment

Choose a reason for hiding this comment

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

Thanks for this PR @Juneezee!

I checked the golang changelog and t.Tempdir was introduced in go1.15, so this is safe for us to backport as well. I've left two questions about additional changes you've included beyond that.

client/logmon/logmon_test.go
Comment on lines +217 to +219
t.Cleanup(func() {
require.NoError(lm.Stop())
})
Copy link
Member

Choose a reason for hiding this comment

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

The commit message for the changes in this file was only:

test: fix TestLogmon_Start_restart on Windows

Can you explain what was happening here? If you think there's a Windows-specific issue here it may actually be a Windows-specific bug! Keeping the PR to a single topic will also make it easier for us to backport it to earlier versions to make future bug fix backports easier.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a Windows-specific issue, but not a bug. You can see the full test log here: https://app.circleci.com/pipelines/github/hashicorp/nomad/23525/workflows/812a1d80-37b1-4251-8fc4-b7f192a7bf20/jobs/250935/tests#failed-test-0

The problem here is that Windows does not allow removing an open file (golang/go#50510 (comment)). .So we have to close os.File properly, which is what the two t.Cleanup below are doing

	t.Cleanup(func() {
		require.NoError(stdout.Close())
	})

	t.Cleanup(func() {
		require.NoError(stderr.Close())
	})

command/agent/consul/int_test.go
Comment on lines +130 to +132
t.Cleanup(func() {
r.NoError(allocDir.Destroy())
})
Copy link
Member

Choose a reason for hiding this comment

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

This seems to change the semantics of the test. Why do we need to Destroy this dir instead of relying on the cleanup function to remove it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The full test log: https://app.circleci.com/pipelines/github/hashicorp/nomad/23525/workflows/812a1d80-37b1-4251-8fc4-b7f192a7bf20/jobs/250947/tests#failed-test-0

At the middle of the logs,

2022-05-03T07:40:32.419Z [WARN]  taskrunner/script_check_hook.go:381: task_runner.task_hook.script_checks: updating check failed: task=web error="Unexpected response code: 500 (CheckID \"_nomad-check-fe290e29ae5ee2da59e3bdbaf221c7b18e766e4f\" does not have associated TTL)"
    testing.go:967: TempDir RemoveAll cleanup: unlinkat /tmp/TestConsul_Integration1745651562/002/7f3f8cc1-961a-f83a-7daf-1554fd2cec19/web/secrets: device or resource busy
bootstrap = true: do not enable unless necessary
==> Starting Consul agent...
           Version: '1.8.3'
           Node ID: '2d6af774-8e9b-b8c8-cbe5-557349a513fc'
         Node name: 'node-2d6af774-8e9b-b8c8-cbe5-557349a513fc'
        Datacenter: 'dc1' (Segment: '<all>')
            Server: true (Bootstrap: true)
       Client Addr: [127.0.0.1] (HTTP: 22260, HTTPS: 22261, gRPC: -1, DNS: 22259)
      Cluster Addr: 127.0.0.1 (LAN: 22262, WAN: 22263)
           Encrypt: Gossip: false, TLS-Outgoing: false, TLS-Incoming: false, Auto-Encrypt-TLS: false

==> Log data will now stream in as it occurs:

t.TempDir failed to perform the cleanup because the child folder is still being used. allocDir.Destroy() is required here to unmount the directory

Copy link
Member

Choose a reason for hiding this comment

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

Ok, thanks for following up.

@tgross tgross added the backport/1.3.x backport to 1.3.x release line label May 3, 2022
tgross
tgross approved these changes May 3, 2022
View reviewed changes
Copy link
Member

@tgross tgross left a comment

Choose a reason for hiding this comment

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

LGTM

@tgross tgross added this to the 1.3.1 milestone May 3, 2022
@tgross
Copy link
Member

tgross commented May 3, 2022

Hey @Juneezee just a heads up that we're in the middle of cutting the GA from our release candidate and that'll be shipping shortly. So I'm going to hold off on merging this until we've shipped the GA. Thanks again for contributing this!

@tgross tgross merged commit fca4ee8 into hashicorp:main May 12, 2022
@hc-github-team-nomad-core hc-github-team-nomad-core mentioned this pull request May 12, 2022
Merged
@shoenig
Copy link
Member

shoenig commented May 12, 2022

This is awesome, thanks @Juneezee !

@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 12, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@tgross tgross tgross approved these changes

Assignees
No one assigned
Labels
backport/1.3.x backport to 1.3.x release line
Projects
Nomad - Community Issues Triage
Archived in project
Milestone
1.3.1
Development

Successfully merging this pull request may close these issues.
3 participants
@Juneezee @tgross @shoenig