Skip to content

Commit cf5db89

Browse files
jtmcdolejtmcdole
authored
Fix merge group not closing on Neutral status (#4990)
A neutral "status" is completed and treated as successful in the case of testing Follow up PR to refactor more of this + more tests.
1 parent 49d3355 commit cf5db89

10 files changed

Lines changed: 256 additions & 6 deletions

File tree

‎app_dart/lib/src/model/common/presubmit_completed_check.dart‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -86,13 +86,14 @@ class PresubmitCompletedCheck {
8686
/// Creates a [PresubmitCompletedCheck] from a BuildBucket [Build].
8787
factory PresubmitCompletedCheck.fromBuild(
8888
Build build,
89-
PresubmitUserData userData,
90-
) {
89+
PresubmitUserData userData, {
90+
TaskStatus? status,
91+
}) {
9192
return PresubmitCompletedCheck(
9293
name: build.builder.builder,
9394
sha: userData.commit.sha,
9495
slug: userData.commit.slug,
95-
status: build.status.toTaskStatus(),
96+
status: status ?? build.status.toTaskStatus(),
9697
isMergeGroup: _isMergeGroup(userData.commit.branch),
9798
checkRunId: userData.guardCheckRunId ?? userData.checkRunId!,
9899
checkSuiteId: userData.checkSuiteId,

‎app_dart/lib/src/request_handlers/presubmit_luci_subscription.dart‎

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import 'dart:convert';
66

77
import 'package:archive/archive.dart';
88
import 'package:buildbucket/buildbucket_pb.dart' as bbv2;
9+
import 'package:cocoon_common/task_status.dart';
910
import 'package:cocoon_server/logging.dart';
1011
import 'package:github/github.dart';
1112

@@ -133,9 +134,9 @@ final class PresubmitLuciSubscription extends SubscriptionHandler {
133134
);
134135
}
135136
}
137+
CheckRunConclusion? override;
136138
if (!isUnifiedCheckRun) {
137139
String? suppressedMessage;
138-
CheckRunConclusion? override;
139140
if (build.status.isTaskFailed() && !rescheduled) {
140141
// If a test is suppressed; we avoid setting a failing status.
141142
final isSuppressed = await cache.isTestSuppressed(
@@ -164,7 +165,13 @@ final class PresubmitLuciSubscription extends SubscriptionHandler {
164165
// Process to the check-run status in the merge queue document during
165166
// the LUCI callback.
166167
if (config.flags.closeMqGuardAfterPresubmit || isUnifiedCheckRun) {
167-
final check = PresubmitCompletedCheck.fromBuild(build, userData);
168+
final check = PresubmitCompletedCheck.fromBuild(
169+
build,
170+
userData,
171+
status: override == CheckRunConclusion.neutral
172+
? TaskStatus.neutral
173+
: null,
174+
);
168175
await _scheduler.processCheckRunCompleted(check);
169176
}
170177
}

‎app_dart/test/request_handlers/presubmit_luci_subscription_neutral_test.dart‎

Lines changed: 224 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,224 @@
1+
// Copyright 2026 The Flutter Authors. All rights reserved.
2+
// Use of this source code is governed by a BSD-style license that can be
3+
// found in the LICENSE file.
4+
5+
import 'package:buildbucket/buildbucket_pb.dart' as bbv2;
6+
import 'package:cocoon_common/task_status.dart';
7+
import 'package:cocoon_integration_test/testing.dart';
8+
import 'package:cocoon_server_test/mocks.dart';
9+
import 'package:cocoon_server_test/test_logging.dart';
10+
import 'package:cocoon_service/cocoon_service.dart';
11+
import 'package:cocoon_service/src/model/commit_ref.dart';
12+
import 'package:cocoon_service/src/model/common/presubmit_completed_check.dart';
13+
import 'package:cocoon_service/src/service/luci_build_service/user_data.dart';
14+
import 'package:fixnum/fixnum.dart';
15+
import 'package:github/github.dart' as github;
16+
import 'package:mockito/mockito.dart';
17+
import 'package:test/test.dart';
18+
19+
import '../src/request_handling/subscription_tester.dart';
20+
21+
void main() {
22+
useTestLoggerPerTest();
23+
24+
late PresubmitLuciSubscription handler;
25+
late FakeConfig config;
26+
late MockGitHub mockGitHubClient;
27+
late FakeHttpRequest request;
28+
late SubscriptionTester tester;
29+
late MockRepositoriesService mockRepositoriesService;
30+
late MockGithubChecksService mockGithubChecksService;
31+
late FakeCiYamlFetcher ciYamlFetcher;
32+
late MockScheduler mockScheduler;
33+
late FakeFirestoreService firestore;
34+
35+
setUp(() async {
36+
firestore = FakeFirestoreService();
37+
38+
config = FakeConfig(
39+
dynamicConfig: DynamicConfig.fromJson({
40+
'closeMqGuardAfterPresubmit': true,
41+
}),
42+
);
43+
mockGithubChecksService = MockGithubChecksService();
44+
mockScheduler = MockScheduler();
45+
46+
ciYamlFetcher = FakeCiYamlFetcher(
47+
ciYaml: examplePresubmitRescheduleFusionConfig,
48+
);
49+
50+
handler = PresubmitLuciSubscription(
51+
cache: CacheService(inMemory: true),
52+
config: config,
53+
luciBuildService: FakeLuciBuildService(
54+
config: config,
55+
firestore: firestore,
56+
),
57+
githubChecksService: mockGithubChecksService,
58+
authProvider: FakeDashboardAuthentication(),
59+
scheduler: mockScheduler,
60+
ciYamlFetcher: ciYamlFetcher,
61+
firestore: firestore,
62+
);
63+
request = FakeHttpRequest();
64+
65+
tester = SubscriptionTester(request: request);
66+
67+
mockGitHubClient = MockGitHub();
68+
mockRepositoriesService = MockRepositoriesService();
69+
when(mockGitHubClient.repositories).thenReturn(mockRepositoriesService);
70+
config.githubClient = mockGitHubClient;
71+
});
72+
73+
test(
74+
'Requests when task failed and is suppressed reports neutral to scheduler',
75+
() async {
76+
final userData = PresubmitUserData(
77+
commit: CommitRef(
78+
sha: 'abc',
79+
branch: 'master',
80+
slug: github.RepositorySlug('flutter', 'flutter'),
81+
),
82+
checkRunId: 1,
83+
checkSuiteId: 2,
84+
);
85+
86+
// Setup Firestore to mark the test as suppressed
87+
firestore.putDocument(
88+
SuppressedTest(
89+
name: 'Linux A',
90+
repository: 'flutter/flutter',
91+
issueLink: 'https://github.com/flutter/flutter/issues/123',
92+
isSuppressed: true,
93+
createTimestamp: DateTime.now(),
94+
)
95+
..name = firestore.resolveDocumentName(
96+
SuppressedTest.kCollectionId,
97+
'suppressed_1',
98+
),
99+
);
100+
101+
when(
102+
mockGithubChecksService.updateCheckStatus(
103+
build: anyNamed('build'),
104+
checkRunId: anyNamed('checkRunId'),
105+
luciBuildService: anyNamed('luciBuildService'),
106+
slug: anyNamed('slug'),
107+
conclusionOverride: github.CheckRunConclusion.neutral,
108+
summaryPrepend: anyNamed('summaryPrepend'),
109+
),
110+
).thenAnswer((_) async => true);
111+
112+
when(
113+
mockScheduler.processCheckRunCompleted(any),
114+
).thenAnswer((_) async => true);
115+
116+
tester.message = createPushMessage(
117+
Int64(1),
118+
status: bbv2.Status.FAILURE,
119+
builder: 'Linux A',
120+
userData: userData,
121+
);
122+
123+
await tester.post(handler);
124+
125+
// Verify that updateCheckStatus was called with neutral override
126+
verify(
127+
mockGithubChecksService.updateCheckStatus(
128+
build: anyNamed('build'),
129+
checkRunId: 1,
130+
luciBuildService: anyNamed('luciBuildService'),
131+
slug: github.RepositorySlug('flutter', 'flutter'),
132+
conclusionOverride: github.CheckRunConclusion.neutral,
133+
summaryPrepend: argThat(
134+
contains(
135+
'### ⚠️ Test failed but marked as suppressed on dashboard',
136+
),
137+
named: 'summaryPrepend',
138+
),
139+
),
140+
).called(1);
141+
142+
// Verify that processCheckRunCompleted was called with TaskStatus.neutral
143+
final captured = verify(
144+
mockScheduler.processCheckRunCompleted(captureAny),
145+
).captured;
146+
expect(captured, hasLength(1));
147+
expect(
148+
captured[0],
149+
isA<PresubmitCompletedCheck>().having(
150+
(e) => e.status,
151+
'status',
152+
TaskStatus.neutral,
153+
),
154+
);
155+
},
156+
);
157+
158+
test(
159+
'Requests when task failed and is NOT suppressed reports failure to scheduler',
160+
() async {
161+
final userData = PresubmitUserData(
162+
commit: CommitRef(
163+
sha: 'abc',
164+
branch: 'master',
165+
slug: github.RepositorySlug('flutter', 'flutter'),
166+
),
167+
checkRunId: 1,
168+
checkSuiteId: 2,
169+
);
170+
171+
// Suppression is NOT set up in Firestore
172+
173+
when(
174+
mockGithubChecksService.updateCheckStatus(
175+
build: anyNamed('build'),
176+
checkRunId: anyNamed('checkRunId'),
177+
luciBuildService: anyNamed('luciBuildService'),
178+
slug: anyNamed('slug'),
179+
conclusionOverride: null,
180+
summaryPrepend: null,
181+
),
182+
).thenAnswer((_) async => true);
183+
184+
when(
185+
mockScheduler.processCheckRunCompleted(any),
186+
).thenAnswer((_) async => true);
187+
188+
tester.message = createPushMessage(
189+
Int64(1),
190+
status: bbv2.Status.FAILURE,
191+
builder: 'Linux A',
192+
userData: userData,
193+
);
194+
195+
await tester.post(handler);
196+
197+
// Verify that updateCheckStatus was called without neutral override
198+
verify(
199+
mockGithubChecksService.updateCheckStatus(
200+
build: anyNamed('build'),
201+
checkRunId: 1,
202+
luciBuildService: anyNamed('luciBuildService'),
203+
slug: github.RepositorySlug('flutter', 'flutter'),
204+
conclusionOverride: null,
205+
summaryPrepend: null,
206+
),
207+
).called(1);
208+
209+
// Verify that processCheckRunCompleted was called with TaskStatus.failed
210+
final captured = verify(
211+
mockScheduler.processCheckRunCompleted(captureAny),
212+
).captured;
213+
expect(captured, hasLength(1));
214+
expect(
215+
captured[0],
216+
isA<PresubmitCompletedCheck>().having(
217+
(e) => e.status,
218+
'status',
219+
TaskStatus.failed,
220+
),
221+
);
222+
},
223+
);
224+
}

‎dashboard/lib/logic/task_sorting.dart‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,9 @@ int _statusPriority(TaskStatus status) {
4444
return 5;
4545
case TaskStatus.skipped:
4646
return 6;
47-
case TaskStatus.succeeded:
47+
case TaskStatus.neutral:
4848
return 7;
49+
case TaskStatus.succeeded:
50+
return 8;
4951
}
5052
}

‎dashboard/lib/service/data_seeder.dart‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -460,6 +460,8 @@ class DataSeeder {
460460
TaskStatus.waitingForBackfill => null,
461461
TaskStatus.skipped =>
462462
'[INFO] Starting task $buildName...\n[INFO] Test skipped: Dummy Tests',
463+
TaskStatus.neutral =>
464+
'[INFO] Starting task $buildName...\n[INFO] Test status neutral: Dummy Tests',
463465
},
464466
startTime: creationTime + 30000,
465467
endTime: creationTime + 60000,

‎dashboard/lib/views/presubmit_view.dart‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -747,6 +747,8 @@ class _CheckItem extends StatelessWidget {
747747
color: TaskBox.statusColor[status],
748748
size: 18,
749749
);
750+
case TaskStatus.neutral:
751+
return Icon(Icons.flaky, color: TaskBox.statusColor[status], size: 18);
750752
case TaskStatus.cancelled:
751753
return Icon(
752754
Icons.block_outlined,

‎dashboard/lib/widgets/task_overlay.dart‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -257,6 +257,8 @@ class TaskOverlayContents extends StatelessWidget {
257257
buffer.write('Running for ${ranFor.inMinutes} minutes');
258258
case TaskStatus.skipped:
259259
buffer.write('Skipped');
260+
case TaskStatus.neutral:
261+
buffer.write('Neutral');
260262
case TaskStatus.cancelled:
261263
buffer.write('Cancelled');
262264
case TaskStatus.succeeded:

‎dashboard/test/logic/task_sorting_test.dart‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ void main() {
1717
('e', TaskStatus.waitingForBackfill),
1818
('f', TaskStatus.cancelled),
1919
('g', TaskStatus.skipped),
20+
('h', TaskStatus.neutral),
2021
];
2122

2223
tasks.sort((a, b) => compareTasks(a.$1, a.$2, b.$1, b.$2));
@@ -28,6 +29,7 @@ void main() {
2829
('e', TaskStatus.waitingForBackfill),
2930
('f', TaskStatus.cancelled),
3031
('g', TaskStatus.skipped),
32+
('h', TaskStatus.neutral),
3133
('a', TaskStatus.succeeded),
3234
]);
3335
});
@@ -58,6 +60,8 @@ void main() {
5860
('w', TaskStatus.inProgress),
5961
('v', TaskStatus.failed),
6062
('u', TaskStatus.succeeded),
63+
('0', TaskStatus.neutral),
64+
('t', TaskStatus.neutral),
6165
];
6266

6367
tasks.sort((a, b) => compareTasks(a.$1, a.$2, b.$1, b.$2));
@@ -67,6 +71,8 @@ void main() {
6771
('y', TaskStatus.failed),
6872
('x', TaskStatus.infraFailure),
6973
('w', TaskStatus.inProgress),
74+
('0', TaskStatus.neutral),
75+
('t', TaskStatus.neutral),
7076
('u', TaskStatus.succeeded),
7177
('z', TaskStatus.succeeded),
7278
]);

‎dashboard/test/utils/generate_task_for_tests.dart‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,7 @@ Task generateTaskForTest({
6868
case TaskStatus.failed:
6969
case TaskStatus.infraFailure:
7070
case TaskStatus.succeeded:
71+
case TaskStatus.neutral:
7172
started = true;
7273
completed = true;
7374
case TaskStatus.inProgress:

‎packages/cocoon_common/lib/task_status.dart‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,9 @@ enum TaskStatus {
2727
/// The task ran successfully.
2828
succeeded('Succeeded'),
2929

30+
/// The task ran but the status is ignored.
31+
neutral('Neutral'),
32+
3033
/// The task was skipped instead of being executed.
3134
skipped('Skipped');
3235

0 commit comments

Comments
 (0)