Repository: snowflakedb/gosnowflake
Pull request #647 (2022-08-19 03:08:35)

test: use `T.TempDir` to create temporary test directory
### Description
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

```go
func TestFoo(t *testing.T) {
	// before
	tmpDir, err := os.MkdirTemp("", "")
	if err != nil {
		t.Error(err)
	}
	defer os.RemoveAll(tmpDir)

	// now
	tmpDir := t.TempDir()
}
```

### Checklist
- [x] Code compiles correctly
- [x] Run ``make fmt`` to fix inconsistent formats
- [x] Run ``make lint`` to get lint errors and fix all of them
- [ ] Created tests which fail without the change (if possible)
- [ ] All tests passing
- [ ] Extended the README / documentation, if necessary


github-actions[bot]: **CLA Assistant Lite bot:** <br/>Thank you for your submission, we really appreciate it. Like many open-source projects, we ask that you sign our [Contributor License Agreement](https://github.com/Snowflake-Labs/CLA/blob/main/README.md) before we can accept your contribution. You can sign the CLA by just posting a Pull Request Comment same as the below format.<br/>
   - - -
   I have read the CLA Document and I hereby sign the CLA
   - - -
   <sub>You can retrigger this bot by commenting **recheck** in this Pull Request</sub>
Juneezee: I have read the CLA Document and I hereby sign the CLA
Juneezee: recheck
Juneezee: Hello @sfc-gh-dszmolka @sfc-gh-igarish . I apologize for the explicit mention, but I wanted to follow up on this pull request I created some time ago. 

I would be grateful if you could take some time to review the changes and provide me with your feedback. If there are any specific concerns or questions you have about the changes, I am more than willing to help address them.

Thank you for your time and consideration. I truly appreciate it. :heart: 
sfc-gh-igarish: @Juneezee In original code it was using pattern (the second argument) in each MkdirTemp call. How the replacement call works for the same?
Juneezee: > @Juneezee In original code it was using pattern (the second argument) in each MkdirTemp call. How the replacement call works for the same?

@sfc-gh-igarish `t.TempDir()` uses the test name as the directory name, You can see an example here: https://go.dev/play/p/OtvRbfHywtk
Juneezee: friendly ping @sfc-gh-igarish 
Juneezee: Hi @sfc-gh-igarish, can you re-run those failing tests? I think those failures are not related to this PR. Thanks!
sfc-gh-igarish: @Juneezee Please update the branch. Even if it's not related to this PR, it's a merge gate. I will try again once you update the branch.
sfc-gh-igarish: @Juneezee These are the failures, which I believe related to this PR:
 RUN   TestEncryptDecryptFilePadding
    testing.go:1097: TempDir RemoveAll cleanup: remove C:\Users\RUNNER~1\AppData\Local\Temp\TestEncryptDecryptFilePadding3894548605\006\file65544: The process cannot access the file because it is being used by another process.
--- FAIL: TestEncryptDecryptFilePadding (3.80s)
=== RUN   TestEncryptDecryptLargeFile
    testing.go:1097: TempDir RemoveAll cleanup: remove C:\Users\RUNNER~1\AppData\Local\Temp\TestEncryptDecryptLargeFile100552289\001\file0: The process cannot access the file because it is being used by another process.
--- FAIL: TestEncryptDecryptLargeFile (2.09s)
Juneezee: @sfc-gh-igarish 

Windows handle open files differently so we need to close all open file descriptors explicitly. I've found a few places where we missed adding `.Close()`, and I have added them in the latest commit. Let's try running test again. Thanks!

Patch:
diff --git a/encrypt_util.go b/encrypt_util.go
index 435ecba..0817989 100644
--- a/encrypt_util.go
+++ b/encrypt_util.go
@@ -176,10 +176,13 @@ func encryptFile(
 	if err != nil {
 		return nil, "", err
 	}
+	defer tmpOutputFile.Close()
 	infile, err := os.OpenFile(filename, os.O_CREATE|os.O_RDONLY, readWriteFileMode)
 	if err != nil {
 		return nil, "", err
 	}
+	defer infile.Close()
+
 	meta, err := encryptStream(sfe, infile, tmpOutputFile, chunkSize)
 	if err != nil {
 		return nil, "", err


Test patch:
diff --git a/encrypt_util_test.go b/encrypt_util_test.go
index bfcb6e5..03f075d 100644
--- a/encrypt_util_test.go
+++ b/encrypt_util_test.go
@@ -91,11 +91,7 @@ func TestEncryptDecryptFilePadding(t *testing.T) {
 
 	for _, test := range testcases {
 		t.Run(fmt.Sprintf("%v_%v", test.numberOfBytesInEachRow, test.numberOfLines), func(t *testing.T) {
-			tmpDir, err := os.MkdirTemp("", "data")
-			if err != nil {
-				t.Error(err)
-			}
-			tmpDir, err = generateKLinesOfNByteRows(test.numberOfLines, test.numberOfBytesInEachRow, tmpDir)
+			tmpDir, err := generateKLinesOfNByteRows(test.numberOfLines, test.numberOfBytesInEachRow, t.TempDir())
 			if err != nil {
 				t.Error(err)
 			}
@@ -114,11 +110,7 @@ func TestEncryptDecryptLargeFile(t *testing.T) {
 
 	numberOfFiles := 1
 	numberOfLines := 10000
-	tmpDir, err := os.MkdirTemp("", "data")
-	if err != nil {
-		t.Error(err)
-	}
-	tmpDir, err = generateKLinesOfNFiles(numberOfLines, numberOfFiles, false, tmpDir)
+	tmpDir, err := generateKLinesOfNFiles(numberOfLines, numberOfFiles, false, t.TempDir())
 	if err != nil {
 		t.Error(err)
 	}
@@ -127,7 +119,6 @@ func TestEncryptDecryptLargeFile(t *testing.T) {
 }
 
 func encryptDecryptFile(t *testing.T, encMat snowflakeFileEncryption, expected int, tmpDir string) {
-	defer os.RemoveAll(tmpDir)
 	files, err := filepath.Glob(filepath.Join(tmpDir, "file*"))
 	if err != nil {
 		t.Error(err)
@@ -150,6 +141,8 @@ func encryptDecryptFile(t *testing.T, encMat snowflakeFileEncryption, expected i
 	if err != nil {
 		t.Error(err)
 	}
+	defer fd.Close()
+
 	scanner := bufio.NewScanner(fd)
 	for scanner.Scan() {
 		cnt++
@@ -163,12 +156,6 @@ func encryptDecryptFile(t *testing.T, encMat snowflakeFileEncryption, expected i
 }
 
 func generateKLinesOfNByteRows(numLines int, numBytes int, tmpDir string) (string, error) {
-	if tmpDir == "" {
-		_, err := os.MkdirTemp(tmpDir, "data")
-		if err != nil {
-			return "", err
-		}
-	}
 	fname := path.Join(tmpDir, "file"+strconv.FormatInt(int64(numLines*numBytes), 10))
 	f, err := os.Create(fname)
 	if err != nil {
@@ -185,12 +172,6 @@ func generateKLinesOfNByteRows(numLines int, numBytes int, tmpDir string) (strin
 }
 
 func generateKLinesOfNFiles(k int, n int, compress bool, tmpDir string) (string, error) {
-	if tmpDir == "" {
-		_, err := os.MkdirTemp(tmpDir, "data")
-		if err != nil {
-			return "", err
-		}
-	}
 	for i := 0; i < n; i++ {
 		fname := path.Join(tmpDir, "file"+strconv.FormatInt(int64(i), 10))
 		f, err := os.Create(fname)
@@ -248,6 +229,8 @@ func generateKLinesOfNFiles(k int, n int, compress bool, tmpDir string) (string,
 					return "", err
 				}
 				w.Close()
+				fOut.Close()
+				fIn.Close()
 			}
 		}
 	}
diff --git a/put_get_test.go b/put_get_test.go
index 730efbc..fee0516 100644
--- a/put_get_test.go
+++ b/put_get_test.go
@@ -34,19 +34,17 @@ func TestPutError(t *testing.T) {
 	if isWindows {
 		t.Skip("permission model is different")
 	}
-	tmpDir, err := os.MkdirTemp("", "putfiledir")
-	if err != nil {
-		t.Error(err)
-	}
-	defer os.RemoveAll(tmpDir)
+	tmpDir := t.TempDir()
 	file1 := filepath.Join(tmpDir, "file1")
 	remoteLocation := filepath.Join(tmpDir, "remote_loc")
 	f, err := os.Create(file1)
 	if err != nil {
 		t.Error(err)
 	}
+	defer f.Close()
 	f.WriteString("test1")
 	os.Chmod(file1, 0000)
+	defer os.Chmod(file1, 0644)
 
 	data := &execResponseData{
 		Command:           string(uploadCommand),
@@ -253,11 +251,7 @@ func TestPutWithAutoCompressFalse(t *testing.T) {
 	if runningOnGithubAction() && !runningOnAWS() {
 		t.Skip("skipping non aws environment")
 	}
-	tmpDir, err := os.MkdirTemp("", "put")
-	if err != nil {
-		t.Error(err)
-	}
-	defer os.RemoveAll(tmpDir)
+	tmpDir := t.TempDir()
 	testData := filepath.Join(tmpDir, "data.txt")
 	f, err := os.Create(testData)
 	if err != nil {
@@ -294,11 +288,7 @@ func TestPutWithAutoCompressFalse(t *testing.T) {
 }
 
 func TestPutOverwrite(t *testing.T) {
-	tmpDir, err := os.MkdirTemp("", "data")
-	if err != nil {
-		t.Error(err)
-	}
-	defer os.RemoveAll(tmpDir)
+	tmpDir := t.TempDir()
 	testData := filepath.Join(tmpDir, "data.txt")
 	f, err := os.Create(testData)
 	if err != nil {
@@ -388,11 +378,7 @@ func TestPutGetStream(t *testing.T) {
 }
 
 func testPutGet(t *testing.T, isStream bool) {
-	tmpDir, err := os.MkdirTemp("", "put_get")
-	if err != nil {
-		t.Error(err)
-	}
-	defer os.RemoveAll(tmpDir)
+	tmpDir := t.TempDir()
 	fname := filepath.Join(tmpDir, "test_put_get.txt.gz")
 	originalContents := "123,test1\n456,test2\n"
 	tableName := randomString(5)
@@ -401,23 +387,19 @@ func testPutGet(t *testing.T, isStream bool) {
 	gzw := gzip.NewWriter(&b)
 	gzw.Write([]byte(originalContents))
 	gzw.Close()
-	if err = os.WriteFile(fname, b.Bytes(), readWriteFileMode); err != nil {
+	if err := os.WriteFile(fname, b.Bytes(), readWriteFileMode); err != nil {
 		t.Fatal("could not write to gzip file")
 	}
 
 	runDBTest(t, func(dbt *DBTest) {
 		dbt.mustExec("create or replace table " + tableName +
 			" (a int, b string)")
+		defer dbt.mustExec("drop table " + tableName)
 		fileStream, err := os.Open(fname)
 		if err != nil {
 			t.Error(err)
 		}
-		defer func() {
-			defer dbt.mustExec("drop table " + tableName)
-			if fileStream != nil {
-				fileStream.Close()
-			}
-		}()
+		defer fileStream.Close()
 
 		var sqlText string
 		var rows *RowsExtended
@@ -489,6 +471,7 @@ func testPutGet(t *testing.T, isStream bool) {
 		if err != nil {
 			t.Error(err)
 		}
+		defer gz.Close()
 		var contents string
 		for {
 			c := make([]byte, defaultChunkBufferSize)
diff --git a/put_get_user_stage_test.go b/put_get_user_stage_test.go
index 1f8021f..f959260 100644
--- a/put_get_user_stage_test.go
+++ b/put_get_user_stage_test.go
@@ -14,29 +14,24 @@ func TestPutGetFileSmallDataViaUserStage(t *testing.T) {
 	if os.Getenv("AWS_ACCESS_KEY_ID") == "" {
 		t.Skip("this test requires to change the internal parameter")
 	}
-	putGetUserStage(t, "", 5, 1, false)
+	putGetUserStage(t, 5, 1, false)
 }
 
 func TestPutGetStreamSmallDataViaUserStage(t *testing.T) {
 	if os.Getenv("AWS_ACCESS_KEY_ID") == "" {
 		t.Skip("this test requires to change the internal parameter")
 	}
-	putGetUserStage(t, "", 1, 1, true)
+	putGetUserStage(t, 1, 1, true)
 }
 
-func putGetUserStage(t *testing.T, tmpDir string, numberOfFiles int, numberOfLines int, isStream bool) {
+func putGetUserStage(t *testing.T, numberOfFiles int, numberOfLines int, isStream bool) {
 	if os.Getenv("AWS_SECRET_ACCESS_KEY") == "" {
 		t.Fatal("no aws secret access key found")
 	}
-	tmpDir, err := os.MkdirTemp(tmpDir, "data")
+	tmpDir, err := generateKLinesOfNFiles(numberOfLines, numberOfFiles, false, t.TempDir())
 	if err != nil {
 		t.Error(err)
 	}
-	tmpDir, err = generateKLinesOfNFiles(numberOfLines, numberOfFiles, false, tmpDir)
-	if err != nil {
-		t.Error(err)
-	}
-	defer os.RemoveAll(tmpDir)
 	var files string
 	if isStream {
 		list, err := os.ReadDir(tmpDir)
diff --git a/put_get_with_aws_test.go b/put_get_with_aws_test.go
index b5753d2..1bc499d 100644
--- a/put_get_with_aws_test.go
+++ b/put_get_with_aws_test.go
@@ -91,11 +91,7 @@ func TestPutWithInvalidToken(t *testing.T) {
 		if !runningOnAWS() {
 			t.Skip("skipping non aws environment")
 		}
-		tmpDir, err := os.MkdirTemp("", "aws_put")
-		if err != nil {
-			t.Error(err)
-		}
-		defer os.RemoveAll(tmpDir)
+		tmpDir := t.TempDir()
 		fname := filepath.Join(tmpDir, "test_put_get_with_aws.txt.gz")
 		originalContents := "123,test1\n456,test2\n"
 
@@ -189,11 +185,7 @@ func TestPretendToPutButList(t *testing.T) {
 	if runningOnGithubAction() && !runningOnAWS() {
 		t.Skip("skipping non aws environment")
 	}
-	tmpDir, err := os.MkdirTemp("", "aws_put")
-	if err != nil {
-		t.Error(err)
-	}
-	defer os.RemoveAll(tmpDir)
+	tmpDir := t.TempDir()
 	fname := filepath.Join(tmpDir, "test_put_get_with_aws.txt.gz")
 	originalContents := "123,test1\n456,test2\n"
 
@@ -244,11 +236,7 @@ func TestPutGetAWSStage(t *testing.T) {
 		t.Skip("skipping non aws environment")
 	}
 
-	tmpDir, err := os.MkdirTemp("", "put_get")
-	if err != nil {
-		t.Error(err)
-	}
-	defer os.RemoveAll(tmpDir)
+	tmpDir := t.TempDir()
 	name := "test_put_get.txt.gz"
 	fname := filepath.Join(tmpDir, name)
 	originalContents := "123,test1\n456,test2\n"
@@ -258,7 +246,7 @@ func TestPutGetAWSStage(t *testing.T) {
 	gzw := gzip.NewWriter(&b)
 	gzw.Write([]byte(originalContents))
 	gzw.Close()
-	if err = os.WriteFile(fname, b.Bytes(), readWriteFileMode); err != nil {
+	if err := os.WriteFile(fname, b.Bytes(), readWriteFileMode); err != nil {
 		t.Fatal("could not write to gzip file")
 	}