studio/flamenco
studio
/
flamenco
12
27
Fork 34
Code Issues 61 Pull Requests 11 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
MichaelC commented 2023-07-06 04:35:24 +02:00
Contributor
Copy Link

Related: #104233

If the mock tests are run by root user then this specific test of inaccessible path fails because root can write files to anywhere on the filesystem. It is not clear that Flamenco Manager test TestCheckSharedStoragePath is checking inaccessible file locations when it fails and that it should be run by an unprivileged user.

Fix is to fail the permission test if the tests are run as a root user.

Related: https://projects.blender.org/studio/flamenco/issues/104233 If the mock tests are run by root user then this specific test of inaccessible path fails because root can write files to anywhere on the filesystem. It is not clear that Flamenco Manager test TestCheckSharedStoragePath is checking inaccessible file locations when it fails and that it should be run by an unprivileged user. Fix is to fail the permission test if the tests are run as a root user.
MichaelC added 1 commit 2023-07-06 04:35:26 +02:00
Sybren A. Stüvel requested changes 2023-07-06 10:47:10 +02:00
Sybren A. Stüvel left a comment
Owner
Copy Link

Fix is to ignore this test if the user is root.

This IMO is not a good idea, especially because it happens silently. If this test is known to require a non-root account, it should simply fail if that requirement is not met.

> Fix is to ignore this test if the user is root. This IMO is not a good idea, especially because it happens silently. If this test is known to require a non-root account, it should simply fail if that requirement is not met.
internal/manager/api_impl/meta_test.go Outdated
@ -198,1 +193,4 @@
currentUser, err := user.Current()
if err != nil {
t.FailNow()
Sybren A. Stüvel commented 2023-07-06 10:46:26 +02:00
Owner
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) ```
MichaelC added 1 commit 2023-07-06 14:59:54 +02:00
MichaelC commented 2023-07-06 15:00:29 +02:00
Author
Contributor
Copy Link

Thanks for the review. I agree with your points and failing on root is a better solution and using your code snippet makes sense.

Tested as root user inside a Docker container...

--- FAIL: TestCheckSharedStoragePath (0.00s)
    meta_test.go:198: 
        	Error Trace:	meta_test.go:198
        	Error:      	Should not be: "0"
        	Test:       	TestCheckSharedStoragePath
        	Messages:   	this test requires running as normal user, not root (0)
Thanks for the review. I agree with your points and failing on root is a better solution and using your code snippet makes sense. Tested as root user inside a Docker container... ``` --- FAIL: TestCheckSharedStoragePath (0.00s) meta_test.go:198: Error Trace: meta_test.go:198 Error: Should not be: "0" Test: TestCheckSharedStoragePath Messages: this test requires running as normal user, not root (0) ```
MichaelC requested review from Sybren A. Stüvel 2023-07-06 15:00:40 +02:00
MichaelC changed title from Flamenco Unit Test - meta_test.go - TestCheckSharedStoragePath ignore permission test if tests are run as root user (linux) to Flamenco Unit Test - meta_test.go - TestCheckSharedStoragePath fail permission test if tests are run as root user (linux) 2023-07-06 15:00:58 +02:00
Sybren A. Stüvel changed title from Flamenco Unit Test - meta_test.go - TestCheckSharedStoragePath fail permission test if tests are run as root user (linux) to Shaman: fail unit test when running as root user (linux) 2023-07-07 14:58:06 +02:00
Sybren A. Stüvel reviewed 2023-07-07 14:58:52 +02:00
internal/manager/api_impl/meta_test.go
@ -203,6 +214,7 @@ func TestCheckSharedStoragePath(t *testing.T) {
assert.Equal(t, testPath, result.Path)
assert.False(t, result.IsUsable)
assert.Contains(t, result.Cause, "Unable to create a file")
Sybren A. Stüvel commented 2023-07-07 14:58:52 +02:00
Owner
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.
Sybren A. Stüvel approved these changes 2023-07-07 14:59:06 +02:00
Sybren A. Stüvel left a comment
Owner
Copy Link

Thanks!

Thanks!
Sybren A. Stüvel manually merged commit b20ede97ea into main 2023-07-07 16:07:17 +02:00
Sign in to join this conversation.
Reviewers
No reviewers
Sybren A. Stüvel
Labels
Clear labels   
Good First Issue
  
Priority
High

  
Priority
Low

  
Priority
Normal

  
Status
Archived

  
Status
Confirmed

  
Status
Needs Info from Developers

  
Status
Needs Information from User

  
Status
Needs Triage

  
Status
Resolved

  
Type
Bug

  
Type
Design

  
Type
Job Type
Custom Flamenco job type

  
Type
Known Issue

  
Type
Patch

  
Type
Report

  
Type
To Do

No Label
Good First Issue
Priority
High
Priority
Low
Priority
Normal
Status
Archived
Status
Confirmed
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Job Type
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: studio/flamenco#104234
No description provided.
Delete Branch "michael-2/flamenco:mgr_api_meta_test_TestCheckSharedStoragePath"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?