Skip to content

Commit 4f80505

Browse files
ievdokdmievdokdm
committed
fix: update presubmit check handling and improve guard failure logic
1 parent d5fcbf5 commit 4f80505

File tree

6 files changed

+126
-131
lines changed

6 files changed

+126
-131
lines changed

‎app_dart/lib/src/model/firestore/presubmit_check.dart‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -233,8 +233,12 @@ final class PresubmitCheck extends AppDocument<PresubmitCheck> {
233233
fields[fieldEndTime] = endTime.toValue();
234234
}
235235

236-
set summary(String summary) {
237-
fields[fieldSummary] = summary.toValue();
236+
set summary(String? summary) {
237+
if (summary == null) {
238+
fields.remove(fieldSummary);
239+
} else {
240+
fields[fieldSummary] = summary.toValue();
241+
}
238242
}
239243

240244
void updateFromBuild(bbv2.Build build) {

‎app_dart/lib/src/service/firestore/unified_check_run.dart‎

Lines changed: 38 additions & 84 deletions
Original file line numberDiff line numberDiff line change
@@ -76,66 +76,6 @@ final class UnifiedCheckRun {
7676
}
7777
}
7878

79-
/// Initializes a new document for the given [tasks] in Firestore so that stage-tracking can succeed.
80-
///
81-
/// The list of tasks will be written as fields of a document with additional fields for tracking the creationTime
82-
/// number of tasks, remaining count. It is required to include [commitSha] as a json encoded [CheckRun] as this
83-
/// will be used to unlock any check runs blocking progress.
84-
///
85-
/// Returns the created document or throws an error.
86-
static Future<Document> initializePresubmitGuardDocument({
87-
required FirestoreService firestoreService,
88-
89-
required RepositorySlug slug,
90-
required int pullRequestId,
91-
required CheckRun checkRun,
92-
required CiStage stage,
93-
required String commitSha,
94-
required int creationTime,
95-
required String author,
96-
required List<String> tasks,
97-
}) async {
98-
final buildCount = tasks.length;
99-
final logCrumb =
100-
'initializeDocument(${slug.owner}_${slug.name}_${pullRequestId}_${checkRun.id}_$stage, $buildCount builds)';
101-
102-
final presubmitGuard = PresubmitGuard(
103-
checkRun: checkRun,
104-
commitSha: commitSha,
105-
slug: slug,
106-
pullRequestId: pullRequestId,
107-
stage: stage,
108-
creationTime: creationTime,
109-
author: author,
110-
remainingBuilds: buildCount,
111-
failedBuilds: 0,
112-
builds: {for (final task in tasks) task: TaskStatus.waitingForBackfill},
113-
);
114-
115-
final presubmitGuardId = PresubmitGuard.documentIdFor(
116-
slug: slug,
117-
pullRequestId: pullRequestId,
118-
checkRunId: checkRun.id!,
119-
stage: stage,
120-
);
121-
122-
try {
123-
// Calling createDocument multiple times for the same documentId will return a 409 - ALREADY_EXISTS error;
124-
// this is good because it means we don't have to do any transactions.
125-
// curl -X POST -H "Content-Type: application/json" -H "Authorization: Bearer <TOKEN>" "https://firestore.googleapis.com/v1beta1/projects/flutter-dashboard/databases/cocoon/documents/presubmit_guard?documentId=foo_bar_baz" -d '{"fields": {"test": {"stringValue": "baz"}}}'
126-
final newDoc = await firestoreService.createDocument(
127-
presubmitGuard,
128-
collectionId: presubmitGuard.runtimeMetadata.collectionId,
129-
documentId: presubmitGuardId.documentId,
130-
);
131-
log.info('$logCrumb: document created');
132-
return newDoc;
133-
} catch (e) {
134-
log.warn('$logCrumb: failed to create document', e);
135-
rethrow;
136-
}
137-
}
138-
13979
/// Returns _all_ checks running against the specified github [checkRunId].
14080
static Future<List<PresubmitCheck>> queryAllPresubmitChecksForGuard({
14181
required FirestoreService firestoreService,
@@ -248,9 +188,9 @@ final class UnifiedCheckRun {
248188
await firestoreService.rollback(transaction);
249189
return PresubmitGuardConclusion(
250190
result: PresubmitGuardConclusionResult.missing,
251-
remaining: presubmitGuard.remainingBuilds ?? -1,
191+
remaining: presubmitGuard.remainingBuilds!,
252192
checkRunGuard: presubmitGuard.checkRunJson,
253-
failed: presubmitGuard.failedBuilds ?? -1,
193+
failed: presubmitGuard.failedBuilds!,
254194
summary:
255195
'Check run "${state.buildName}" not present in ${guardId.stage} CI stage',
256196
details: 'Change $changeCrumb',
@@ -271,11 +211,24 @@ final class UnifiedCheckRun {
271211
remaining = presubmitGuard.remainingBuilds!;
272212
failed = presubmitGuard.failedBuilds!;
273213
final builds = presubmitGuard.builds;
214+
var status = builds?[state.buildName]!;
274215

275-
// If build is in progress, we should only update appropriate checks with
276-
// that [TaskStatus]
277-
if (state.status.isBuildInProgress) {
216+
// If build is waiting for backfill, that means its initiated by github
217+
// or re-run. So no processing needed, we should only update appropriate
218+
// checks with that [TaskStatus]
219+
if (state.status == TaskStatus.waitingForBackfill) {
220+
status = state.status;
221+
valid = true;
222+
// If build is in progress, we should update apropriate checks with start
223+
// time and their status to that [TaskStatus] only if the build is not
224+
// completed.
225+
} else if (state.status == TaskStatus.inProgress) {
278226
presubmitCheck.startTime = state.startTime!;
227+
// If the build is not completed, update the status.
228+
if (!status!.isBuildCompleted) {
229+
status = state.status;
230+
}
231+
valid = true;
279232
} else {
280233
// "remaining" should go down if buildSuccessed of any attempt
281234
// or buildFailed a first attempt.
@@ -284,35 +237,35 @@ final class UnifiedCheckRun {
284237
// attemptNumber = 1 && buildFailed: up (+1)
285238
// So if the test existed and either remaining or failed_count is changed;
286239
// the response is valid.
287-
288-
if (state.status.isBuildSuccessed ||
289-
state.attemptNumber == 1 && state.status.isBuildFailed) {
240+
status = state.status;
241+
if (status.isBuildSuccessed) {
290242
// Guard against going negative and log enough info so we can debug.
291243
if (remaining == 0) {
292244
throw '$logCrumb: field "${PresubmitGuard.fieldRemainingBuilds}" is already zero for $transaction / ${presubmitGuardDocument.fields}';
293245
}
294246
remaining = remaining - 1;
295-
valid = true;
296-
}
297247

298-
// Only rollback the "failed" counter if this is a successful test run,
299-
// i.e. the test failed, the user requested a rerun, and now it passes.
300-
if (state.attemptNumber > 1 && state.status.isBuildSuccessed) {
301-
log.info(
302-
'$logCrumb: conclusion flipped to positive - assuming test was re-run',
303-
);
304-
if (failed == 0) {
305-
throw '$logCrumb: field "${PresubmitGuard.fieldFailedBuilds}" is already zero for $transaction / ${presubmitGuardDocument.fields}';
248+
// Only rollback the "failed" counter if this is a successful test run,
249+
// i.e. the test failed, the user requested a rerun, and now it passes.
250+
if (state.attemptNumber > 1) {
251+
log.info(
252+
'$logCrumb: conclusion flipped to positive - assuming test was re-run',
253+
);
254+
if (failed == 0) {
255+
throw '$logCrumb: field "${PresubmitGuard.fieldFailedBuilds}" is already zero for $transaction / ${presubmitGuardDocument.fields}';
256+
}
257+
failed = failed - 1;
306258
}
307259
valid = true;
308-
failed = failed - 1;
309260
}
310261

311262
// Only increment the "failed" counter if the conclusion failed for the first attempt.
312-
if (state.attemptNumber == 1 && state.status.isBuildFailed) {
313-
log.info('$logCrumb: test failed');
263+
if (status.isBuildFailed) {
264+
if (state.attemptNumber == 1) {
265+
log.info('$logCrumb: test failed');
266+
failed = failed + 1;
267+
}
314268
valid = true;
315-
failed = failed + 1;
316269
}
317270

318271
// All checks pass. "valid" is only set to true if there was a change in either the remaining or failed count.
@@ -323,9 +276,10 @@ final class UnifiedCheckRun {
323276
presubmitGuard.failedBuilds = failed;
324277
presubmitCheck.endTime = state.endTime!;
325278
}
326-
builds![state.buildName] = state.status;
279+
builds![state.buildName] = status;
327280
presubmitGuard.builds = builds;
328-
presubmitCheck.status = state.status;
281+
presubmitCheck.status = status;
282+
presubmitCheck.summary = state.summary;
329283
} on DetailedApiRequestError catch (e, stack) {
330284
if (e.status == 404) {
331285
// An attempt to read a document not in firestore should not be retried.

‎app_dart/lib/src/service/scheduler.dart‎

Lines changed: 25 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -671,6 +671,7 @@ class Scheduler {
671671
// only required GitHub check.
672672
await failGuardForMergeGroup(
673673
slug,
674+
lock,
674675
headSha,
675676
'Failed to schedule checks for merge group',
676677
'''
@@ -679,7 +680,7 @@ $logCrumb
679680
ERROR: $e
680681
$s
681682
''',
682-
lock,
683+
null,
683684
);
684685
}
685686
}
@@ -854,10 +855,11 @@ $s
854855
/// corresponding pull request will have to be fixed and re-enqueued again.
855856
Future<void> failGuardForMergeGroup(
856857
RepositorySlug slug,
858+
CheckRun lock,
857859
String headSha,
858860
String summary,
859861
String details,
860-
CheckRun lock,
862+
String? detailsUrl,
861863
) async {
862864
log.info('Failing merge group guard for merge group $headSha in $slug');
863865
await _githubChecksService.githubChecksUtil.updateCheckRun(
@@ -871,6 +873,7 @@ $s
871873
summary: summary,
872874
text: details,
873875
),
876+
detailsUrl: detailsUrl,
874877
);
875878
}
876879

@@ -1018,15 +1021,32 @@ $s
10181021
final guard = checkRunFromString(stagingConclusion.checkRunGuard!);
10191022
await failGuardForMergeGroup(
10201023
check.slug,
1024+
guard,
10211025
check.sha,
10221026
stagingConclusion.summary,
10231027
stagingConclusion.details,
1024-
guard,
1028+
null,
10251029
);
10261030
}
10271031
return false;
10281032
}
10291033

1034+
// If the number of failed checks is equal to the number of remaining checks, then all remaining checks have failed.
1035+
if (check.isUnifiedCheckRun &&
1036+
stagingConclusion.failed > 0 &&
1037+
stagingConclusion.failed == stagingConclusion.remaining) {
1038+
final guard = checkRunFromString(stagingConclusion.checkRunGuard!);
1039+
await failGuardForMergeGroup(
1040+
check.slug,
1041+
guard,
1042+
check.sha,
1043+
kMergeQueueLockDescription,
1044+
'For CI stage ${check.stage} ${stagingConclusion.failed} checks failed',
1045+
'https://flutter-dashboard.appspot.com/repo/${check.slug.name}/checkrun/${check.guardId}',
1046+
);
1047+
return true;
1048+
}
1049+
10301050
// Are there tests remaining? Keep waiting.
10311051
if (stagingConclusion.isPending) {
10321052
log.info(
@@ -1047,10 +1067,11 @@ $s
10471067
final guard = checkRunFromString(stagingConclusion.checkRunGuard!);
10481068
await failGuardForMergeGroup(
10491069
check.slug,
1070+
guard,
10501071
check.sha,
10511072
stagingConclusion.summary,
10521073
stagingConclusion.details,
1053-
guard,
1074+
null,
10541075
);
10551076
}
10561077
return true;

‎app_dart/test/service/firestore/unified_check_run_test.dart‎

Lines changed: 13 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -132,31 +132,36 @@ void main() {
132132
);
133133

134134
// Initialize documents
135-
await UnifiedCheckRun.initializePresubmitGuardDocument(
136-
firestoreService: firestoreService,
135+
final guard = PresubmitGuard(
136+
checkRun: checkRun,
137+
commitSha: sha,
137138
slug: slug,
138139
pullRequestId: 1,
139-
checkRun: checkRun,
140140
stage: CiStage.fusionEngineBuild,
141-
commitSha: sha,
142141
creationTime: 1000,
143142
author: 'dash',
144-
tasks: ['linux', 'mac'],
143+
builds: {
144+
for (final task in ['linux', 'mac'])
145+
task: TaskStatus.waitingForBackfill,
146+
},
147+
remainingBuilds: 2,
148+
failedBuilds: 0,
145149
);
146150

147151
final check1 = PresubmitCheck.init(
148152
buildName: 'linux',
149-
checkRunId: 123,
153+
checkRunId: guardId.checkRunId,
150154
creationTime: 1000,
151155
);
152156
final check2 = PresubmitCheck.init(
153157
buildName: 'mac',
154-
checkRunId: 123,
158+
checkRunId: guardId.checkRunId,
155159
creationTime: 1000,
156160
);
157161

158162
await firestoreService.writeViaTransaction(
159163
documentsToWrites([
164+
Document(name: guard.name, fields: guard.fields),
160165
Document(name: check1.name, fields: check1.fields),
161166
Document(name: check2.name, fields: check2.fields),
162167
], exists: false),
@@ -262,7 +267,7 @@ void main() {
262267
);
263268

264269
expect(result.result, PresubmitGuardConclusionResult.ok);
265-
expect(result.remaining, 1);
270+
expect(result.remaining, 2);
266271
expect(result.failed, 1);
267272
});
268273

0 commit comments

Comments
 (0)