Skip to content

Commit ca0c5ec

Browse files
authored
cleanup: dart 3.10, dot shorthands, refactor isSuccess (#4991)
A number of things: 1. Update to dart 3.10 to allow for dart dot-shorthands 2. Refactor "isBuildSuccessed" and "isBuildFailed" - there is already an isSuccess, isCompleted, and isFailure.
1 parent cf5db89 commit ca0c5ec

20 files changed

Lines changed: 219 additions & 128 deletions

File tree

app_dart/lib/src/model/common/checks_extension.dart

Lines changed: 32 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -11,60 +11,62 @@ extension ChecksExtension on TaskStatus {
1111
/// Converts a [TaskStatus] to a [TaskConclusion].
1212
TaskConclusion toTaskConclusion() {
1313
return switch (this) {
14-
TaskStatus.succeeded => TaskConclusion.success,
15-
TaskStatus.failed || TaskStatus.infraFailure => TaskConclusion.failure,
16-
TaskStatus.inProgress => TaskConclusion.scheduled,
17-
_ => TaskConclusion.unknown,
14+
.succeeded => .success,
15+
.neutral => .neutral,
16+
.failed || .infraFailure => .failure,
17+
.inProgress => .scheduled,
18+
_ => .unknown,
1819
};
1920
}
2021

2122
/// Converts a [TaskStatus] to a GitHub check conclusion string.
2223
String toConclusion() {
2324
return switch (this) {
24-
TaskStatus.succeeded => 'success',
25-
TaskStatus.failed || TaskStatus.infraFailure => 'failure',
26-
TaskStatus.cancelled => 'cancelled',
27-
TaskStatus.skipped => 'skipped',
25+
.succeeded => 'success',
26+
.neutral => 'neutral',
27+
.failed || .infraFailure => 'failure',
28+
.cancelled => 'cancelled',
29+
.skipped => 'skipped',
2830
_ => '',
2931
};
3032
}
3133

3234
/// Converts a [TaskStatus] to a GitHub check conclusion string.
3335
CheckRunConclusion toCheckRunConclusion() {
3436
return switch (this) {
35-
TaskStatus.succeeded => CheckRunConclusion.success,
36-
TaskStatus.failed ||
37-
TaskStatus.infraFailure => CheckRunConclusion.failure,
38-
TaskStatus.cancelled => CheckRunConclusion.cancelled,
39-
TaskStatus.skipped => CheckRunConclusion.skipped,
40-
_ => CheckRunConclusion.empty,
37+
.succeeded => .success,
38+
.failed || .infraFailure => .failure,
39+
.cancelled => .cancelled,
40+
.skipped => .skipped,
41+
.neutral => .neutral,
42+
_ => .empty,
4143
};
4244
}
4345

4446
/// Converts a GitHub check conclusion string to a [TaskStatus].
4547
static TaskStatus fromConclusion(String? conclusion) {
4648
return switch (conclusion) {
47-
'success' => TaskStatus.succeeded,
48-
'failure' => TaskStatus.failed,
49-
'neutral' => TaskStatus.succeeded,
50-
'cancelled' => TaskStatus.cancelled,
51-
'timed_out' => TaskStatus.failed,
52-
'action_required' => TaskStatus.failed,
53-
'skipped' => TaskStatus.skipped,
54-
_ => TaskStatus.failed,
49+
'success' => .succeeded,
50+
'failure' => .failed,
51+
'neutral' => .neutral,
52+
'cancelled' => .cancelled,
53+
'timed_out' => .failed,
54+
'action_required' => .failed,
55+
'skipped' => .skipped,
56+
_ => .failed,
5557
};
5658
}
5759

5860
static TaskStatus fromCheckRunConclusion(CheckRunConclusion? conclusion) {
5961
return switch (conclusion) {
60-
CheckRunConclusion.success => TaskStatus.succeeded,
61-
CheckRunConclusion.failure => TaskStatus.failed,
62-
CheckRunConclusion.neutral => TaskStatus.succeeded,
63-
CheckRunConclusion.cancelled => TaskStatus.cancelled,
64-
CheckRunConclusion.timedOut => TaskStatus.failed,
65-
CheckRunConclusion.actionRequired => TaskStatus.failed,
66-
CheckRunConclusion.skipped => TaskStatus.skipped,
67-
_ => TaskStatus.failed,
62+
.success => .succeeded,
63+
.failure => .failed,
64+
.neutral => .neutral,
65+
.cancelled => .cancelled,
66+
.timedOut => .failed,
67+
.actionRequired => .failed,
68+
.skipped => .skipped,
69+
_ => .failed,
6870
};
6971
}
7072
}

app_dart/lib/src/model/firestore/ci_staging.dart

Lines changed: 11 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -289,8 +289,7 @@ final class CiStaging extends AppDocument<CiStaging> {
289289
// recordedConclusion == failure && conclusion == success: down (-1)
290290
// recordedConclusion != failure && conclusion == failure: up (+1)
291291
// So if the test existed and either remaining or failed_count is changed; the response is valid.
292-
if (recordedConclusion == TaskConclusion.scheduled &&
293-
conclusion != TaskConclusion.scheduled) {
292+
if (recordedConclusion == .scheduled && conclusion != .scheduled) {
294293
// Guard against going negative and log enough info so we can debug.
295294
if (remaining == 0) {
296295
throw '$logCrumb: field "$kRemainingField" is already zero for $transaction / ${doc.fields}';
@@ -301,8 +300,7 @@ final class CiStaging extends AppDocument<CiStaging> {
301300

302301
// Only rollback the "failed" counter if this is a successful test run,
303302
// i.e. the test failed, the user requested a rerun, and now it passes.
304-
if (recordedConclusion == TaskConclusion.failure &&
305-
conclusion == TaskConclusion.success) {
303+
if (recordedConclusion.isFailure && conclusion.isSuccess) {
306304
log.info(
307305
'$logCrumb: conclusion flipped to positive - assuming test was re-run',
308306
);
@@ -314,9 +312,8 @@ final class CiStaging extends AppDocument<CiStaging> {
314312
}
315313

316314
// Only increment the "failed" counter if the new conclusion flips from positive or neutral to failure.
317-
if ((recordedConclusion == TaskConclusion.scheduled ||
318-
recordedConclusion == TaskConclusion.success) &&
319-
conclusion == TaskConclusion.failure) {
315+
if ((recordedConclusion == .scheduled || recordedConclusion.isSuccess) &&
316+
conclusion.isFailure) {
320317
log.info('$logCrumb: test failed');
321318
valid = true;
322319
failed = failed + 1;
@@ -460,6 +457,9 @@ enum TaskConclusion {
460457
/// A task was completed as a success.
461458
success,
462459

460+
/// A task was completed, but status is ignored.
461+
neutral,
462+
463463
/// A task was completed as a failure.
464464
failure;
465465

@@ -477,5 +477,8 @@ enum TaskConclusion {
477477
bool get isComplete => this != scheduled;
478478

479479
/// Whether the task is a success or not.
480-
bool get isSuccess => this == success;
480+
bool get isSuccess => this == success || this == neutral;
481+
482+
/// Whether the task is a failure or not.
483+
bool get isFailure => this == failure;
481484
}

app_dart/lib/src/request_handlers/presubmit_luci_subscription.dart

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@ 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';
109
import 'package:cocoon_server/logging.dart';
1110
import 'package:github/github.dart';
1211

@@ -168,9 +167,7 @@ final class PresubmitLuciSubscription extends SubscriptionHandler {
168167
final check = PresubmitCompletedCheck.fromBuild(
169168
build,
170169
userData,
171-
status: override == CheckRunConclusion.neutral
172-
? TaskStatus.neutral
173-
: null,
170+
status: override == .neutral ? .neutral : null,
174171
);
175172
await _scheduler.processCheckRunCompleted(check);
176173
}

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

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -213,9 +213,9 @@ final class UnifiedCheckRun {
213213
// request re-run we have to only increment remaining builds.
214214
// If build is still in progress re-run is not possible but if some how they
215215
// manage to request re-run we should not touch any counters.
216-
if (currentStatus.isBuildCompleted) {
216+
if (currentStatus.isComplete) {
217217
guard.remainingBuilds += 1;
218-
if (currentStatus.isBuildFailed && guard.failedBuilds > 0) {
218+
if (currentStatus.isFailure && guard.failedBuilds > 0) {
219219
guard.failedBuilds -= 1;
220220
}
221221
}
@@ -584,20 +584,20 @@ final class UnifiedCheckRun {
584584
} else if (state.status == TaskStatus.inProgress) {
585585
presubmitCheck.startTime = state.startTime!;
586586
// If the build is not completed, update the status.
587-
if (!status.isBuildCompleted) {
587+
if (!status.isComplete) {
588588
status = state.status;
589589
}
590590
valid = true;
591591
} else {
592592
// If build already compleated remaining and failed should not updated.
593-
if (!status.isBuildCompleted) {
593+
if (!status.isComplete) {
594594
// "remaining" should go down if build is succeeded or failed.
595595
// "failed_count" can go up or down depending on:
596596
// attemptNumber > 1 && buildSuccessed: down (-1)
597597
// attemptNumber = 1 && buildFailed: up (+1)
598598
// So if the test existed and either remaining or failed_count is changed;
599599
// the response is valid.
600-
if (state.status.isBuildCompleted) {
600+
if (state.status.isComplete) {
601601
// Guard against going negative and log enough info so we can debug.
602602
if (remaining == 0) {
603603
throw '$logCrumb: field "${PresubmitGuard.fieldRemainingBuilds}" is already zero for $transaction / ${presubmitGuardDocument.fields}';
@@ -606,7 +606,7 @@ final class UnifiedCheckRun {
606606
valid = true;
607607
}
608608

609-
if (state.status.isBuildFailed) {
609+
if (state.status.isFailure) {
610610
log.info('$logCrumb: test failed');
611611
failed += 1;
612612
valid = true;

app_dart/lib/src/service/scheduler.dart

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1025,7 +1025,7 @@ detailsUrl: $detailsUrl
10251025
} else {
10261026
// for github flow check runs are processed only if the build succeeded or
10271027
// some kind of failure occurred.
1028-
if (!check.status.isBuildCompleted) {
1028+
if (!check.status.isComplete) {
10291029
return true;
10301030
}
10311031
// Check runs are fired at every stage. However, at this point it is unknown

app_dart/pubspec.yaml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ publish_to: none
1010
resolution: workspace
1111

1212
environment:
13-
sdk: ^3.9.0
13+
sdk: ^3.10.0
1414

1515
dependencies:
1616
appengine: 0.13.11
Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
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:cocoon_common/task_status.dart';
6+
import 'package:cocoon_server_test/test_logging.dart';
7+
import 'package:cocoon_service/src/model/common/checks_extension.dart';
8+
import 'package:cocoon_service/src/model/firestore/ci_staging.dart';
9+
import 'package:github/github.dart';
10+
import 'package:test/test.dart';
11+
12+
void main() {
13+
useTestLoggerPerTest();
14+
15+
group('ChecksExtension', () {
16+
test('toTaskConclusion mapping for neutral', () {
17+
expect(TaskStatus.neutral.toTaskConclusion(), TaskConclusion.neutral);
18+
});
19+
20+
test('toConclusion mapping for neutral', () {
21+
expect(TaskStatus.neutral.toConclusion(), 'neutral');
22+
});
23+
24+
test('toCheckRunConclusion mapping for neutral', () {
25+
expect(
26+
TaskStatus.neutral.toCheckRunConclusion(),
27+
CheckRunConclusion.neutral,
28+
);
29+
});
30+
31+
test('fromConclusion mapping for neutral', () {
32+
expect(ChecksExtension.fromConclusion('neutral'), TaskStatus.neutral);
33+
});
34+
35+
test('fromCheckRunConclusion mapping for neutral', () {
36+
expect(
37+
ChecksExtension.fromCheckRunConclusion(CheckRunConclusion.neutral),
38+
TaskStatus.neutral,
39+
);
40+
});
41+
});
42+
43+
group('TaskConclusion', () {
44+
test('isSuccess returns true for neutral', () {
45+
expect(TaskConclusion.neutral.isSuccess, isTrue);
46+
});
47+
48+
test('isFailure returns false for neutral', () {
49+
expect(TaskConclusion.neutral.isFailure, isFalse);
50+
});
51+
52+
test('isComplete returns true for neutral', () {
53+
expect(TaskConclusion.neutral.isComplete, isTrue);
54+
});
55+
});
56+
}

app_dart/test/model/firestore/ci_staging_test.dart

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -324,6 +324,49 @@ For CI stage engine:
324324
);
325325
});
326326

327+
test('handles a test flip-flop (neutral) after re-running', () async {
328+
firestoreService.putDocument(
329+
Document(
330+
name: expectedName,
331+
fields: {
332+
CiStaging.kRemainingField: 1.toValue(),
333+
CiStaging.kFailedField: 1.toValue(),
334+
CiStaging.kTotalField: 1.toValue(),
335+
CiStaging.kCheckRunGuardField: '{}'.toValue(),
336+
'MacOS build_test': TaskConclusion.failure.name.toValue(),
337+
},
338+
),
339+
);
340+
341+
final future = CiStaging.markConclusion(
342+
firestoreService: firestoreService,
343+
slug: slug,
344+
sha: '1234',
345+
stage: CiStage.fusionEngineBuild,
346+
checkRun: 'MacOS build_test',
347+
conclusion: TaskConclusion.neutral,
348+
);
349+
350+
final result = await future;
351+
// Remaining == 1 because our test was already concluded.
352+
expect(
353+
result,
354+
const PresubmitGuardConclusion(
355+
remaining: 1,
356+
result: PresubmitGuardConclusionResult.ok,
357+
failed: 0,
358+
checkRunGuard: '{}',
359+
summary: 'All tests passed',
360+
details: '''
361+
For CI stage engine:
362+
Total check runs scheduled: 1
363+
Pending: 1
364+
Failed: 0
365+
''',
366+
),
367+
);
368+
});
369+
327370
test('ignored repeat failures', () async {
328371
firestoreService.putDocument(
329372
Document(

auto_submit/pubspec.yaml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ homepage: https://github.com/flutter/cocoon/blob/main/autosubmit
1010
resolution: workspace
1111

1212
environment:
13-
sdk: ^3.9.0
13+
sdk: ^3.10.0
1414

1515
dependencies:
1616
appengine: 0.13.11

dashboard/lib/service/data_seeder.dart

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -411,7 +411,7 @@ class DataSeeder {
411411
.where((status) => status.isFailure)
412412
.length;
413413
final remainingBuilds = builds.values
414-
.where((status) => !status.isBuildCompleted)
414+
.where((status) => !status.isComplete)
415415
.length;
416416

417417
return PresubmitGuard(
@@ -448,20 +448,20 @@ class DataSeeder {
448448
creationTime: creationTime,
449449
buildNumber: 1337 + attemptNumber,
450450
summary: switch (status) {
451-
TaskStatus.succeeded =>
451+
.succeeded =>
452452
'[INFO] Starting task $buildName...\n[SUCCESS] All tests passed (452/452)',
453-
TaskStatus.failed =>
453+
.failed =>
454454
'[INFO] Starting task $buildName...\n[ERROR] Test failed: Dummy Tests',
455-
TaskStatus.infraFailure =>
455+
.infraFailure =>
456456
'[INFO] Starting task $buildName...\n[ERROR] Infrastructure failure: Dummy Tests',
457-
TaskStatus.cancelled =>
457+
.cancelled =>
458458
'[INFO] Starting task $buildName...\n[ERROR] Test cancelled: Dummy Tests',
459-
TaskStatus.inProgress => '[INFO] Starting task $buildName...',
460-
TaskStatus.waitingForBackfill => null,
461-
TaskStatus.skipped =>
459+
.inProgress => '[INFO] Starting task $buildName...',
460+
.waitingForBackfill => null,
461+
.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',
463+
.neutral =>
464+
'[INFO] Starting task $buildName...\n[INFO] Test neutral: Dummy Tests',
465465
},
466466
startTime: creationTime + 30000,
467467
endTime: creationTime + 60000,

0 commit comments

Comments
 (0)