test: use `T.TempDir` to create temporary test directory #151

Juneezee · 2022-05-18T13:39:07Z

Ⅰ. Describe what this PR does

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

Ⅱ. Does this pull request fix one issue?

NONE

Ⅲ. Describe how to verify it

All tests should continue to pass. No leftover temporary directories when the tests finish.

Ⅳ. Special notes for reviews

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>

codecov-commenter · 2022-05-18T13:47:20Z

Codecov Report

Merging #151 (10ed0d1) into main (0cf1616) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #151      +/-   ##
==========================================
+ Coverage   56.89%   56.90%   +0.01%     
==========================================
  Files          86       86              
  Lines        7887     7882       -5     
==========================================
- Hits         4487     4485       -2     
+ Misses       3006     3004       -2     
+ Partials      394      393       -1

Flag	Coverage Δ
unittests	`56.90% <100.00%> (+0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pkg/util/system/util_test_tool.go	`54.11% <100.00%> (+0.78%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0cf1616...10ed0d1. Read the comment docs.

jasonliu747

This is actually a very good suggestion!
/lgtm
/approve

saintube · 2022-05-19T01:47:40Z

pkg/koordlet/resmanager/cgroup_reconcile_test.go

@@ -805,8 +804,7 @@ func TestCgroupResourceReconcile_calculateResources(t *testing.T) {
 			assert.NoError(t, err)
 			defer func() { stop <- struct{}{} }()

-			helper := system.NewFileTestUtil(t)
-			defer helper.Cleanup()
+			_ = system.NewFileTestUtil(t)


can we remove the equation?

saintube · 2022-05-19T01:48:52Z

pkg/koordlet/resmanager/resctrl_reconcile_test.go

-
-			helper := system.NewFileTestUtil(t)
-			defer helper.Cleanup()
+			_ = system.NewFileTestUtil(t)


can we remove the equation

saintube · 2022-05-19T01:57:56Z

pkg/util/system/util_test_tool.go

 func NewFileTestUtil(t *testing.T) *FileTestUtil {
-	tempDir, err := ioutil.TempDir("/tmp", "koordlet_test")
+	tempDir := t.TempDir()


It seems (*FileTestUtil).Cleanup() is useless with this patch and its references are removed.
Can we remove the Cleanup() since its work would be done implicitly?

NewFileTestUtil now uses t.TempDir() to create temporary directory so manual cleanup is no longer necessary. Signed-off-by: Eng Zer Jun <engzerjun@gmail.com>

jasonliu747

/lgtm

saintube

/lgtm

hormes · 2022-05-19T07:31:26Z

/approve

koordinator-bot · 2022-05-19T07:31:30Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hormes, jasonliu747, saintube

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [hormes]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

jasonliu747 · 2022-05-19T10:44:33Z

@Juneezee Hi, when I ran runtime_test.go and docker_runtime_test.go locally on my Mac, it would throw an error something like this: Unix socket path "/var/folders/t3/fm5gy2413t304ksnq2_jd91h0000gn/T/Test_GetRuntimeHandlertest_have_dockerRuntime_but_need_containerd3848019481/001/var/run/docker.sock" is too long.

Did it ever happen to you?

Juneezee · 2022-05-19T10:56:58Z

@jasonliu747 No, I don't get this error on my Linux machine. This is probably a Mac specific issue, see https://unix.stackexchange.com/a/367012/376279

jasonliu747 · 2022-05-19T11:04:32Z

@jasonliu747 No, I don't get this error on my Linux machine. This is probably a Mac specific issue, see https://unix.stackexchange.com/a/367012/376279

Could you try to fix this? I believe majority of programmer are using Mac for development.

Juneezee · 2022-05-19T11:20:54Z

@jasonliu747 PR #156 fixes the issue.

I think the only way to tell t.TempDir() to use /tmp on MacOS is to set the environment variable $TMPDIR to empty so that it uses /tmp (https://github.com/golang/go/blob/128279e5034ca29bad4320eef81a8abd5b40ea7e/src/os/file.go#L379-L390). This might do more harm than good and break other tests.

Another way is to use ioutil.TempDir and specify /tmp explicitly, just like in #156.

koordinator-bot bot requested review from hormes and jasonliu747 May 18, 2022 13:39

koordinator-bot bot added the size/L label May 18, 2022

jasonliu747 approved these changes May 18, 2022

View reviewed changes

koordinator-bot bot assigned jasonliu747 May 18, 2022

koordinator-bot bot added the lgtm label May 18, 2022

saintube requested changes May 19, 2022

View reviewed changes

test: remove (*FileTestUtil).Cleanup()

10ed0d1

NewFileTestUtil now uses t.TempDir() to create temporary directory so manual cleanup is no longer necessary. Signed-off-by: Eng Zer Jun <engzerjun@gmail.com>

koordinator-bot bot removed the lgtm label May 19, 2022

Juneezee requested a review from saintube May 19, 2022 02:19

jasonliu747 reviewed May 19, 2022

View reviewed changes

koordinator-bot bot added the lgtm label May 19, 2022

saintube approved these changes May 19, 2022

View reviewed changes

koordinator-bot bot assigned saintube May 19, 2022

koordinator-bot bot added the approved label May 19, 2022

koordinator-bot bot merged commit 889465a into koordinator-sh:main May 19, 2022

saintube mentioned this pull request May 19, 2022

chore: fix test tempdir generation #156

Merged

This was referenced May 21, 2022

ci: support running unit test on multiple os #162

Merged

[support request] Run GitHub Action on multiple environments #163

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: use `T.TempDir` to create temporary test directory #151

test: use `T.TempDir` to create temporary test directory #151

Juneezee commented May 18, 2022

codecov-commenter commented May 18, 2022 •

edited

Loading

jasonliu747 left a comment

saintube May 19, 2022

saintube May 19, 2022

saintube May 19, 2022

jasonliu747 May 19, 2022

jasonliu747 left a comment

saintube left a comment

hormes commented May 19, 2022

koordinator-bot bot commented May 19, 2022

jasonliu747 commented May 19, 2022 •

edited

Loading

Juneezee commented May 19, 2022

jasonliu747 commented May 19, 2022

Juneezee commented May 19, 2022

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

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

Conversation

Juneezee commented May 18, 2022

Ⅰ. Describe what this PR does

Ⅱ. Does this pull request fix one issue?

Ⅲ. Describe how to verify it

Ⅳ. Special notes for reviews

codecov-commenter commented May 18, 2022 • edited Loading

Codecov Report

jasonliu747 left a comment

Choose a reason for hiding this comment

saintube May 19, 2022

Choose a reason for hiding this comment

saintube May 19, 2022

Choose a reason for hiding this comment

saintube May 19, 2022

Choose a reason for hiding this comment

jasonliu747 May 19, 2022

Choose a reason for hiding this comment

jasonliu747 left a comment

Choose a reason for hiding this comment

saintube left a comment

Choose a reason for hiding this comment

hormes commented May 19, 2022

koordinator-bot bot commented May 19, 2022

jasonliu747 commented May 19, 2022 • edited Loading

Juneezee commented May 19, 2022

jasonliu747 commented May 19, 2022

Juneezee commented May 19, 2022

test: use `T.TempDir` to create temporary test directory #151

test: use `T.TempDir` to create temporary test directory #151

codecov-commenter commented May 18, 2022 •

edited

Loading

jasonliu747 commented May 19, 2022 •

edited

Loading