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 instead of MkdirTemp #1485

Merged
merged 2 commits into from
Feb 10, 2023
Merged

test: use T.TempDir instead of MkdirTemp #1485

merged 2 commits into from
Feb 10, 2023

Conversation

alexandear
Copy link
Contributor

@alexandear alexandear commented Feb 8, 2023

From the doc:

TempDir returns a temporary directory for the test to use. The directory is automatically removed by Cleanup when the test and all its subtests complete

@erikdubbelboer
Copy link
Collaborator

Seems like it isn't working on windows.

@alexandear
Copy link
Contributor Author

alexandear commented Feb 9, 2023

T.TempDir works on Windows. Seems something wrong with the test or tested code itself.

I reverted PR's changes, replaced defer os.RemoveAll(tempdir) in TestServeFileSmallNoReadFrom with:

defer func() {
	if err := os.RemoveAll(tempdir); err != nil {
		t.Log(err)
	}
}()

run the test on Windows and got the same error as we see in the CI:

go test -v -run TestServeFileSmallNoReadFrom
=== RUN   TestServeFileSmallNoReadFrom
=== PAUSE TestServeFileSmallNoReadFrom
=== CONT  TestServeFileSmallNoReadFrom
    fs_test.go:163: remove C:\Users\builder\AppData\Local\Temp\3\httpexpect1148472821\hello: The process cannot access the file because it is being used by another process.
--- FAIL: TestServeFileSmallNoReadFrom (0.06s)
FAIL
exit status 1
FAIL    github.com/valyala/fasthttp     0.408s

It's needed to investigate further.

@alexandear
Copy link
Contributor Author

@erikdubbelboer fixed it, please review. The problem was that the code doesn't close a file handler. And Windows can't delete a folder with opened files in it.

@erikdubbelboer erikdubbelboer merged commit 8dcbc41 into valyala:master Feb 10, 2023
@erikdubbelboer
Copy link
Collaborator

Thanks!

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

Successfully merging this pull request may close these issues.

3 participants