studio/flamenco
studio/flamenco
12
30
Fork 38
Code Issues 63 Pull Requests 5 Releases Wiki Activity

Shaman: fail unit test when running as root user (linux) #104234

Manually merged
Sybren A. Stüvel merged 2 commits from michael-2/flamenco:mgr_api_meta_test_TestCheckSharedStoragePath into main 2023-07-07 16:07:17 +02:00
Conversation 1 Commits 2 Files Changed 1 +12
1 changed files with 12 additions and 0 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

12
internal/manager/api_impl/meta_test.go
View File

@ -6,6 +6,7 @@ import (
"io/fs" "io/fs"
"net/http" "net/http"
"os" "os"
"os/user"
"path/filepath" "path/filepath"
"runtime" "runtime"
"testing" "testing"
@ -189,6 +190,16 @@ func TestCheckSharedStoragePath(t *testing.T) {
// that seems consistent. // that seems consistent.
// FIXME: find another way to test with unwritable directories on Windows. // FIXME: find another way to test with unwritable directories on Windows.
if runtime.GOOS != "windows" { if runtime.GOOS != "windows" {
// Root can always create directories, so this test would fail with a
// confusing message. Instead it's better to refuse running as root at all.
currentUser, err := user.Current()
Sybren A. Stüvel commented 2023-07-06 10:46:26 +02:00
Outdated
Review
Copy Link

I feel there's too much nesting going on here. Also t.FailNow() wouldn't show the actual content of err. Finally, the original code was written before I realised I could do require.NoError(...) instead of assert.NoError(...) and it would immediately stop the test.

This would be better:

// Root can always create directories, so this test would fail with a
// confusing message. Instead it's better to refuse running as root at all.
currentUser, err := user.Current()
require.NoError(t, err)
require.NotEqual(t, "0", currentUser.Uid,
	"this test requires running as normal user, not %s (%s)", currentUser.Username, currentUser.Uid)
require.NotEqual(t, "root", currentUser.Username,
	"this test requires running as normal user, not %s (%s)", currentUser.Username, currentUser.Uid)
I feel there's too much nesting going on here. Also `t.FailNow()` wouldn't show the actual content of `err`. Finally, the original code was written before I realised I could do `require.NoError(...)` instead of `assert.NoError(...)` and it would immediately stop the test. This would be better: ```go // Root can always create directories, so this test would fail with a // confusing message. Instead it's better to refuse running as root at all. currentUser, err := user.Current() require.NoError(t, err) require.NotEqual(t, "0", currentUser.Uid, "this test requires running as normal user, not %s (%s)", currentUser.Username, currentUser.Uid) require.NotEqual(t, "root", currentUser.Username, "this test requires running as normal user, not %s (%s)", currentUser.Username, currentUser.Uid) ```
require.NoError(t, err)
require.NotEqual(t, "0", currentUser.Uid,
"this test requires running as normal user, not %s (%s)", currentUser.Username, currentUser.Uid)
require.NotEqual(t, "root", currentUser.Username,
"this test requires running as normal user, not %s (%s)", currentUser.Username, currentUser.Uid)
parentPath := filepath.Join(mf.tempdir, "deep") parentPath := filepath.Join(mf.tempdir, "deep")
testPath := filepath.Join(parentPath, "nesting") testPath := filepath.Join(parentPath, "nesting")
if err := os.Mkdir(parentPath, fs.ModePerm); !assert.NoError(t, err) { if err := os.Mkdir(parentPath, fs.ModePerm); !assert.NoError(t, err) {
@ -203,6 +214,7 @@ func TestCheckSharedStoragePath(t *testing.T) {
assert.Equal(t, testPath, result.Path) assert.Equal(t, testPath, result.Path)
assert.False(t, result.IsUsable) assert.False(t, result.IsUsable)
assert.Contains(t, result.Cause, "Unable to create a file") assert.Contains(t, result.Cause, "Unable to create a file")
Sybren A. Stüvel commented 2023-07-07 14:58:52 +02:00
Review
Copy Link

Please keep unrelated formatting changes out of a PR. I'll remove this before landing; this is just a request for other/future PRs.

Please keep unrelated formatting changes out of a PR. I'll remove this before landing; this is just a request for other/future PRs.
} }
} }