Skip to content

Commit

Permalink
[autosubmit] Do not remove label when author is a member (#2922)
Browse files Browse the repository at this point in the history
This is a quality of life change. I often find myself needlessly coming back to PRs to only add the autosubmit label. Instead, I'd like to add when I open my trivial PRs.

This is to align with how Google's autosubmit works.
  • Loading branch information
Casey Hillers authored Jul 11, 2023
1 parent f1216a8 commit 4f9d324
Show file tree
Hide file tree
Showing 4 changed files with 14 additions and 8 deletions.
11 changes: 8 additions & 3 deletions auto_submit/lib/validations/approval.dart
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,10 @@ class Approval extends Validation {

bool approved = false;
String message = '';
Action action = Action.REMOVE_LABEL;
if (repositoryConfiguration.autoApprovalAccounts.contains(author)) {
approved = true;
log.info('PR ${slug.fullName}/${messagePullRequest.number} approved by roller account: $author');
return ValidationResult(approved, Action.REMOVE_LABEL, '');
return ValidationResult(true, Action.REMOVE_LABEL, '');
} else {
final GithubService githubService = await config.createGithubService(slug);
final bool authorIsFlutterHacker =
Expand Down Expand Up @@ -67,12 +67,17 @@ class Approval extends Validation {
approvedMessage = approved
? 'This PR has met approval requirements for merging.\n'
: 'This PR has not met approval requirements for merging. $flutterHackerMessage and need ${approver.remainingReviews} more review(s) in order to merge this PR.\n';
if (!approved && authorIsFlutterHacker) {
// Flutter hackers are aware of the review requirements, and can add
// the autosubmit label without waiting on review.
action = Action.IGNORE_TEMPORARILY;
}
}

message = approved ? approvedMessage : '$approvedMessage\n${Config.pullRequestApprovalRequirementsMessage}';
}

return ValidationResult(approved, Action.REMOVE_LABEL, message);
return ValidationResult(approved, action, message);
}
}

Expand Down
3 changes: 2 additions & 1 deletion auto_submit/test/requests/check_pull_request_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -755,7 +755,8 @@ void main() {
await checkPullRequest.get();
expectedOptions.add(flutterOption);
verifyQueries(expectedOptions);
assert(pubsub.messagesQueue.isEmpty);
// Re-add to queue to poll for reviews
assert(pubsub.messagesQueue.isNotEmpty);
});

test('All messages are pulled', () async {
Expand Down
4 changes: 2 additions & 2 deletions auto_submit/test/service/validation_service_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -62,15 +62,15 @@ void main() {
});
});

test('Removes label and post comment when no approval', () async {
test('Removes label and post comment when no approval for non-flutter hacker', () async {
final PullRequestHelper flutterRequest = PullRequestHelper(
prNumber: 0,
lastCommitHash: oid,
reviews: <PullRequestReviewHelper>[],
);
githubService.checkRunsData = checkRunsMock;
githubService.createCommentData = createCommentMock;
githubService.isTeamMemberMockMap['author1'] = true;
githubService.isTeamMemberMockMap['author1'] = false;
githubService.isTeamMemberMockMap['member'] = true;
final FakePubSub pubsub = FakePubSub();
final PullRequest pullRequest = generatePullRequest(prNumber: 0, repoName: slug.name);
Expand Down
4 changes: 2 additions & 2 deletions auto_submit/test/validations/approval_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ void main() {
final ValidationResult result = await computeValidationResult(review);

expect(result.result, isFalse);
expect(result.action, Action.REMOVE_LABEL);
expect(result.action, Action.IGNORE_TEMPORARILY);
expect(result.message.contains('This PR has not met approval requirements for merging.'), isTrue);
expect(result.message.contains('need 1 more review'), isTrue);
});
Expand Down Expand Up @@ -149,7 +149,7 @@ void main() {
final ValidationResult result = await computeValidationResult(review);

expect(result.result, isFalse);
expect(result.action, Action.REMOVE_LABEL);
expect(result.action, Action.IGNORE_TEMPORARILY);
expect(result.message.contains('This PR has not met approval requirements for merging.'), isTrue);
expect(result.message.contains('need 1 more review'), isTrue);
});
Expand Down

0 comments on commit 4f9d324

Please sign in to comment.