chore: Remove invalid policy owner tests from command_runner_test #5963
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.So the question is, why was
TestFailedApprovalCreatesFailedStatusUpdatepassing? 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).
tests
See above.
In general I'm removing unit tests so shouldn't cause an issue.
references
Found while working on #5962