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.TempDir to create temporary test directory #647

Merged
merged 3 commits into from
Oct 16, 2023
Merged

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

merged 3 commits into from
Oct 16, 2023

Conversation

Juneezee
Copy link
Contributor

@Juneezee Juneezee commented Aug 19, 2022

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

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

  • Code compiles correctly
  • Run make fmt to fix inconsistent formats
  • 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
Copy link

github-actions bot commented Sep 25, 2022

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@Juneezee
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

@Juneezee Juneezee requested a review from a team as a code owner March 13, 2023 01:23
@Juneezee
Copy link
Contributor Author

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. ❤️

@sfc-gh-igarish
Copy link
Collaborator

@Juneezee In original code it was using pattern (the second argument) in each MkdirTemp call. How the replacement call works for the same?

@Juneezee
Copy link
Contributor Author

@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
Copy link
Contributor Author

Juneezee commented Jun 1, 2023

friendly ping @sfc-gh-igarish

Copy link
Collaborator

@sfc-gh-igarish sfc-gh-igarish left a comment

Choose a reason for hiding this comment

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

LGTM

@Juneezee
Copy link
Contributor Author

Juneezee commented Jun 8, 2023

Hi @sfc-gh-igarish, please re-run the failing tests. I think the failing tests are not related to this PR. Thanks.

@sfc-gh-igarish
Copy link
Collaborator

@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
Copy link
Collaborator

@Juneezee These are the failures, which I believe related to this PR:
RUN TestEncryptDecryptFilePadding
testing.go:1097: TempDir RemoveAll cleanup: remove C:\Users\RUNNER1\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
Copy link
Contributor Author

Juneezee commented Jun 9, 2023

@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!

put_get_test.go Outdated Show resolved Hide resolved
put_get_test.go Show resolved Hide resolved
Copy link
Collaborator

@sfc-gh-pfus sfc-gh-pfus left a comment

Choose a reason for hiding this comment

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

LGTM

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 <[email protected]>
This commit fixes the 3 failing tests on Windows [1]
	1. TestEncryptDecryptFilePadding
	2. TestEncryptDecryptLargeFile
	3. TestPutGetFile

They all failed with the same reason:
  testing.go:1097: TempDir RemoveAll cleanup: remove ... The process cannot access the file because it is being used by another process.

Windows handle open files differently [2] so we need to close all open
file descriptors explicitly.

[1]: https://github.com/snowflakedb/gosnowflake/actions/runs/5214296899/jobs/9410395563?pr=647
[2]: golang/go#50510 (comment)

Signed-off-by: Eng Zer Jun <[email protected]>
@sfc-gh-pfus sfc-gh-pfus merged commit fcdd18a into snowflakedb:master Oct 16, 2023
24 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Oct 16, 2023
@sfc-gh-pfus
Copy link
Collaborator

Thank you very much @Juneezee for your input! I merged your PR :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants