Skip to content

Conversation

@lukemassa
Copy link
Contributor

@lukemassa lukemassa commented Nov 16, 2025

what

Remove two tests from command_runner_test that check to make sure policy approver is the policy set owner.

why

This logic (and associated tests) was moved to the project command runner: #3086, command_runner no longer has a notion of owner of a policy config.

The reason these tests have continued to pass is somewhat subtle.

For TestApprovedPoliciesUpdateFailedPolicyStatus, the issue was that it was checking to make sure the policy passed, which, after the logic was moved, it always would, regardless of what user was specified.

atlantis % git diff                                                                 
diff --git a/server/events/command_runner_test.go b/server/events/command_runner_test.go
index 84da1ffc..fabfe84d 100644
--- a/server/events/command_runner_test.go
+++ b/server/events/command_runner_test.go
@@ -1148,7 +1148,7 @@ func TestApprovedPoliciesUpdateFailedPolicyStatus(t *testing.T) {
                        CommandName: command.ApprovePolicies,
                        PolicySets: valid.PolicySets{
                                Owners: valid.PolicyOwners{
-                                       Users: []string{testdata.User.Username},
+                                       Users: []string{"some different name that shouldn't work"},
                                },
                        },
                },
atlantis % go test ./server/events -run TestApprovedPoliciesUpdateFailedPolicyStatus
ok  	github.com/runatlantis/atlantis/server/events	0.412s

So the question is, why was TestFailedApprovalCreatesFailedStatusUpdate passing? It was checking to make sure that the command failed, so it should have failed immediately once the policy check was removed. The issue is that this test had a bug: it never specified a return value for ApprovePolicies, so got the "default" value of a ProjectResult. Part of the determination of whether pullStatus is "success" is the command name, and since ProjectResult contains command name, it got the "default" command name, which is Apply.

This is why I noticed it, because I am trying to remove command name from ProjectResult to prevent exactly this kind of bug (#5962).

atlantis % git diff                                                                 
diff --git a/server/events/command_runner_test.go b/server/events/command_runner_test.go
index 84da1ffc..f7dc2c74 100644
--- a/server/events/command_runner_test.go
+++ b/server/events/command_runner_test.go
@@ -1104,6 +1104,14 @@ func TestFailedApprovalCreatesFailedStatusUpdate(t *testing.T) {
        }, nil)
 
        When(workingDir.GetPullDir(testdata.GithubRepo, testdata.Pull)).ThenReturn(tmp, nil)
+       When(projectCommandRunner.ApprovePolicies(Any[command.ProjectContext]())).Then(func(_ []Param) ReturnValues {
+               return ReturnValues{
+                       command.ProjectResult{
+                               Command:            command.PolicyCheck,
+                               PolicyCheckResults: &models.PolicyCheckResults{},
+                       },
+               }
+       })
 
        ch.RunCommentCommand(testdata.GithubRepo, &testdata.GithubRepo, &testdata.Pull, testdata.User, testdata.Pull.Num, &events.CommentCommand{Name: command.ApprovePolicies})
        commitUpdater.VerifyWasCalledOnce().UpdateCombinedCount(
atlantis % go test ./server/events -run TestFailedApprovalCreatesFailedStatusUpdate 
--- FAIL: TestFailedApprovalCreatesFailedStatusUpdate (0.04s)
    command_runner_test.go:1072: if "atlantis approve_policies" is run by non policy owner policy check status fails.
    logger.go:146: 2025-11-16T00:56:45.134-0500	DEBUG	updating DB with pull results	{"repo": "runatlantis/atlantis", "pull": "1"}
    logger.go:146: 2025-11-16T00:56:45.142-0500	DEBUG	timer	{"name": "atlantis_comment_approve_policies_execution_time", "value": "8.708583ms", "tags": {}, "type": "timer"}
    testing_t_support.go:41: 
        	/Users/lmassa/go/pkg/mod/github.com/petergtz/pegomock/[email protected]/testing_t_support.go:40 +0x48
        github.com/petergtz/pegomock/v4.(*GenericMock).Verify(0x1400007e198, 0x0, {0x101d6d7e0, 0x1400001ebd0}, {0x101858735, 0x13}, {0x14000294cb0, 0x7, 0x7}, {0x140005c3048?, ...})
        	/Users/lmassa/go/pkg/mod/github.com/petergtz/pegomock/[email protected]/dsl.go:153 +0x520
        github.com/runatlantis/atlantis/server/events/mocks.(*VerifierMockCommitStatusUpdater).UpdateCombinedCount(0x140005c3880, {_, _}, {{_, _}, {_, _}, {_, _}, {_, ...}, ...}, ...)
        	/Users/lmassa/atlantis/server/events/mocks/mock_commit_status_updater.go:182 +0x23c
        github.com/runatlantis/atlantis/server/events_test.TestFailedApprovalCreatesFailedStatusUpdate(0x14000092e00)
        	/Users/lmassa/atlantis/server/events/command_runner_test.go:1117 +0x12e8
        testing.tRunner(0x14000092e00, 0x101d5c220)
        	/Users/lmassa/go/pkg/mod/golang.org/[email protected]/src/testing/testing.go:1934 +0xc8
        created by testing.(*T).Run in goroutine 1
        	/Users/lmassa/go/pkg/mod/golang.org/[email protected]/src/testing/testing.go:1997 +0x364
        
        Mock invocation count for UpdateCombinedCount(Any(logging.SimpleLogging), Any(models.Repo), Any(models.PullRequest), Eq(1), Eq(3), Eq(0), Eq(2)) does not match expectation.
        
        	Expected: 1; but got: 0
        
        	Actual interactions with this mock were:
        	UpdateCombined(&logging.StructuredLogger{z:(*zap.SugaredLogger)(0x14000014078), level:zap.AtomicLevel{l:(*atomic.Int32)(0x140000ab040)}, keepHistory:true, history:bytes.Buffer{buf:[]uint8{0x5b, 0x44, 0x42, 0x55, 0x47, 0x5d, 0x20, 0x75, 0x70, 0x64, 0x61, 0x74, 0x69, 0x6e, 0x67, 0x20, 0x44, 0x42, 0x20, 0x77, 0x69, 0x74, 0x68, 0x20, 0x70, 0x75, 0x6c, 0x6c, 0x20, 0x72, 0x65, 0x73, 0x75, 0x6c, 0x74, 0x73, 0xa}, off:0, lastRead:0}}, models.Repo{FullName:"runatlantis/atlantis", Owner:"runatlantis", Name:"atlantis", CloneURL:"https://user:[email protected]/runatlantis/atlantis.git", SanitizedCloneURL:"https://github.com/runatlantis/atlantis.git", VCSHost:models.VCSHost{Hostname:"github.com", Type:0}}, models.PullRequest{Num:1, HeadCommit:"", URL:"", HeadBranch:"", BaseBranch:"", Author:"", State:0, BaseRepo:models.Repo{FullName:"runatlantis/atlantis", Owner:"runatlantis", Name:"atlantis", CloneURL:"https://user:[email protected]/runatlantis/atlantis.git", SanitizedCloneURL:"https://github.com/runatlantis/atlantis.git", VCSHost:models.VCSHost{Hostname:"github.com", Type:0}}}, 0, 3)
        	UpdateCombinedCount(&logging.StructuredLogger{z:(*zap.SugaredLogger)(0x14000014078), level:zap.AtomicLevel{l:(*atomic.Int32)(0x140000ab040)}, keepHistory:true, history:bytes.Buffer{buf:[]uint8{0x5b, 0x44, 0x42, 0x55, 0x47, 0x5d, 0x20, 0x75, 0x70, 0x64, 0x61, 0x74, 0x69, 0x6e, 0x67, 0x20, 0x44, 0x42, 0x20, 0x77, 0x69, 0x74, 0x68, 0x20, 0x70, 0x75, 0x6c, 0x6c, 0x20, 0x72, 0x65, 0x73, 0x75, 0x6c, 0x74, 0x73, 0xa}, off:0, lastRead:0}}, models.Repo{FullName:"runatlantis/atlantis", Owner:"runatlantis", Name:"atlantis", CloneURL:"https://user:[email protected]/runatlantis/atlantis.git", SanitizedCloneURL:"https://github.com/runatlantis/atlantis.git", VCSHost:models.VCSHost{Hostname:"github.com", Type:0}}, models.PullRequest{Num:1, HeadCommit:"", URL:"", HeadBranch:"", BaseBranch:"", Author:"", State:0, BaseRepo:models.Repo{FullName:"runatlantis/atlantis", Owner:"runatlantis", Name:"atlantis", CloneURL:"https://user:[email protected]/runatlantis/atlantis.git", SanitizedCloneURL:"https://github.com/runatlantis/atlantis.git", VCSHost:models.VCSHost{Hostname:"github.com", Type:0}}}, 1, 3, 2, 2)
FAIL
FAIL	github.com/runatlantis/atlantis/server/events	0.380s
FAIL

tests

See above.

In general I'm removing unit tests so shouldn't cause an issue.

references

Found while working on #5962

@dosubot dosubot bot added the go Pull requests that update Go code label Nov 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

go Pull requests that update Go code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant