Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 32 additions & 30 deletions app_dart/lib/src/model/common/checks_extension.dart
Original file line number Diff line number Diff line change
Expand Up @@ -11,60 +11,62 @@ extension ChecksExtension on TaskStatus {
/// Converts a [TaskStatus] to a [TaskConclusion].
TaskConclusion toTaskConclusion() {
return switch (this) {
TaskStatus.succeeded => TaskConclusion.success,
TaskStatus.failed || TaskStatus.infraFailure => TaskConclusion.failure,
TaskStatus.inProgress => TaskConclusion.scheduled,
_ => TaskConclusion.unknown,
.succeeded => .success,
.neutral => .neutral,
.failed || .infraFailure => .failure,
.inProgress => .scheduled,
_ => .unknown,
};
}

/// Converts a [TaskStatus] to a GitHub check conclusion string.
String toConclusion() {
return switch (this) {
TaskStatus.succeeded => 'success',
TaskStatus.failed || TaskStatus.infraFailure => 'failure',
TaskStatus.cancelled => 'cancelled',
TaskStatus.skipped => 'skipped',
.succeeded => 'success',
.neutral => 'neutral',
.failed || .infraFailure => 'failure',
.cancelled => 'cancelled',
.skipped => 'skipped',
_ => '',
};
}

/// Converts a [TaskStatus] to a GitHub check conclusion string.
CheckRunConclusion toCheckRunConclusion() {
return switch (this) {
TaskStatus.succeeded => CheckRunConclusion.success,
TaskStatus.failed ||
TaskStatus.infraFailure => CheckRunConclusion.failure,
TaskStatus.cancelled => CheckRunConclusion.cancelled,
TaskStatus.skipped => CheckRunConclusion.skipped,
_ => CheckRunConclusion.empty,
.succeeded => .success,
.failed || .infraFailure => .failure,
.cancelled => .cancelled,
.skipped => .skipped,
.neutral => .neutral,
_ => .empty,
};
}

/// Converts a GitHub check conclusion string to a [TaskStatus].
static TaskStatus fromConclusion(String? conclusion) {
return switch (conclusion) {
'success' => TaskStatus.succeeded,
'failure' => TaskStatus.failed,
'neutral' => TaskStatus.succeeded,
'cancelled' => TaskStatus.cancelled,
'timed_out' => TaskStatus.failed,
'action_required' => TaskStatus.failed,
'skipped' => TaskStatus.skipped,
_ => TaskStatus.failed,
'success' => .succeeded,
'failure' => .failed,
'neutral' => .neutral,
'cancelled' => .cancelled,
'timed_out' => .failed,
'action_required' => .failed,
'skipped' => .skipped,
_ => .failed,
};
}

static TaskStatus fromCheckRunConclusion(CheckRunConclusion? conclusion) {
return switch (conclusion) {
CheckRunConclusion.success => TaskStatus.succeeded,
CheckRunConclusion.failure => TaskStatus.failed,
CheckRunConclusion.neutral => TaskStatus.succeeded,
CheckRunConclusion.cancelled => TaskStatus.cancelled,
CheckRunConclusion.timedOut => TaskStatus.failed,
CheckRunConclusion.actionRequired => TaskStatus.failed,
CheckRunConclusion.skipped => TaskStatus.skipped,
_ => TaskStatus.failed,
.success => .succeeded,
.failure => .failed,
.neutral => .neutral,
.cancelled => .cancelled,
.timedOut => .failed,
.actionRequired => .failed,
.skipped => .skipped,
_ => .failed,
};
}
}
19 changes: 11 additions & 8 deletions app_dart/lib/src/model/firestore/ci_staging.dart
Original file line number Diff line number Diff line change
Expand Up @@ -289,8 +289,7 @@ final class CiStaging extends AppDocument<CiStaging> {
// recordedConclusion == failure && conclusion == success: down (-1)
// recordedConclusion != failure && conclusion == failure: up (+1)
// So if the test existed and either remaining or failed_count is changed; the response is valid.
if (recordedConclusion == TaskConclusion.scheduled &&
conclusion != TaskConclusion.scheduled) {
if (recordedConclusion == .scheduled && conclusion != .scheduled) {
Comment thread
jtmcdole marked this conversation as resolved.
// Guard against going negative and log enough info so we can debug.
if (remaining == 0) {
throw '$logCrumb: field "$kRemainingField" is already zero for $transaction / ${doc.fields}';
Expand All @@ -301,8 +300,7 @@ final class CiStaging extends AppDocument<CiStaging> {

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

// Only increment the "failed" counter if the new conclusion flips from positive or neutral to failure.
if ((recordedConclusion == TaskConclusion.scheduled ||
recordedConclusion == TaskConclusion.success) &&
conclusion == TaskConclusion.failure) {
if ((recordedConclusion == .scheduled || recordedConclusion.isSuccess) &&
conclusion.isFailure) {
log.info('$logCrumb: test failed');
valid = true;
failed = failed + 1;
Expand Down Expand Up @@ -460,6 +457,9 @@ enum TaskConclusion {
/// A task was completed as a success.
success,

/// A task was completed, but status is ignored.
neutral,

/// A task was completed as a failure.
failure;

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

/// Whether the task is a success or not.
bool get isSuccess => this == success;
bool get isSuccess => this == success || this == neutral;

/// Whether the task is a failure or not.
Comment thread
jtmcdole marked this conversation as resolved.
bool get isFailure => this == failure;
}
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import 'dart:convert';

import 'package:archive/archive.dart';
import 'package:buildbucket/buildbucket_pb.dart' as bbv2;
import 'package:cocoon_common/task_status.dart';
import 'package:cocoon_server/logging.dart';
import 'package:github/github.dart';

Expand Down Expand Up @@ -168,9 +167,7 @@ final class PresubmitLuciSubscription extends SubscriptionHandler {
final check = PresubmitCompletedCheck.fromBuild(
build,
userData,
status: override == CheckRunConclusion.neutral
? TaskStatus.neutral
: null,
status: override == .neutral ? .neutral : null,
Comment thread
jtmcdole marked this conversation as resolved.
);
await _scheduler.processCheckRunCompleted(check);
}
Expand Down
12 changes: 6 additions & 6 deletions app_dart/lib/src/service/firestore/unified_check_run.dart
Original file line number Diff line number Diff line change
Expand Up @@ -213,9 +213,9 @@ final class UnifiedCheckRun {
// request re-run we have to only increment remaining builds.
// If build is still in progress re-run is not possible but if some how they
// manage to request re-run we should not touch any counters.
if (currentStatus.isBuildCompleted) {
if (currentStatus.isComplete) {
guard.remainingBuilds += 1;
if (currentStatus.isBuildFailed && guard.failedBuilds > 0) {
if (currentStatus.isFailure && guard.failedBuilds > 0) {
guard.failedBuilds -= 1;
}
}
Expand Down Expand Up @@ -584,20 +584,20 @@ final class UnifiedCheckRun {
} else if (state.status == TaskStatus.inProgress) {
presubmitCheck.startTime = state.startTime!;
// If the build is not completed, update the status.
if (!status.isBuildCompleted) {
if (!status.isComplete) {
status = state.status;
}
valid = true;
} else {
// If build already compleated remaining and failed should not updated.
if (!status.isBuildCompleted) {
if (!status.isComplete) {
// "remaining" should go down if build is succeeded or failed.
// "failed_count" can go up or down depending on:
// attemptNumber > 1 && buildSuccessed: down (-1)
// attemptNumber = 1 && buildFailed: up (+1)
// So if the test existed and either remaining or failed_count is changed;
// the response is valid.
if (state.status.isBuildCompleted) {
if (state.status.isComplete) {
// Guard against going negative and log enough info so we can debug.
if (remaining == 0) {
throw '$logCrumb: field "${PresubmitGuard.fieldRemainingBuilds}" is already zero for $transaction / ${presubmitGuardDocument.fields}';
Expand All @@ -606,7 +606,7 @@ final class UnifiedCheckRun {
valid = true;
}

if (state.status.isBuildFailed) {
if (state.status.isFailure) {
log.info('$logCrumb: test failed');
failed += 1;
valid = true;
Expand Down
2 changes: 1 addition & 1 deletion app_dart/lib/src/service/scheduler.dart
Original file line number Diff line number Diff line change
Expand Up @@ -1025,7 +1025,7 @@ detailsUrl: $detailsUrl
} else {
// for github flow check runs are processed only if the build succeeded or
// some kind of failure occurred.
if (!check.status.isBuildCompleted) {
if (!check.status.isComplete) {
return true;
}
// Check runs are fired at every stage. However, at this point it is unknown
Expand Down
2 changes: 1 addition & 1 deletion app_dart/pubspec.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ publish_to: none
resolution: workspace

environment:
sdk: ^3.9.0
sdk: ^3.10.0

dependencies:
appengine: 0.13.11
Expand Down
56 changes: 56 additions & 0 deletions app_dart/test/model/common/checks_extension_test.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
// Copyright 2026 The Flutter Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

import 'package:cocoon_common/task_status.dart';
import 'package:cocoon_server_test/test_logging.dart';
import 'package:cocoon_service/src/model/common/checks_extension.dart';
import 'package:cocoon_service/src/model/firestore/ci_staging.dart';
import 'package:github/github.dart';
import 'package:test/test.dart';

void main() {
useTestLoggerPerTest();

group('ChecksExtension', () {
test('toTaskConclusion mapping for neutral', () {
expect(TaskStatus.neutral.toTaskConclusion(), TaskConclusion.neutral);
});

test('toConclusion mapping for neutral', () {
expect(TaskStatus.neutral.toConclusion(), 'neutral');
});

test('toCheckRunConclusion mapping for neutral', () {
expect(
TaskStatus.neutral.toCheckRunConclusion(),
CheckRunConclusion.neutral,
);
});

test('fromConclusion mapping for neutral', () {
expect(ChecksExtension.fromConclusion('neutral'), TaskStatus.neutral);
});

test('fromCheckRunConclusion mapping for neutral', () {
expect(
ChecksExtension.fromCheckRunConclusion(CheckRunConclusion.neutral),
TaskStatus.neutral,
);
});
});

group('TaskConclusion', () {
test('isSuccess returns true for neutral', () {
expect(TaskConclusion.neutral.isSuccess, isTrue);
});

test('isFailure returns false for neutral', () {
expect(TaskConclusion.neutral.isFailure, isFalse);
});

test('isComplete returns true for neutral', () {
expect(TaskConclusion.neutral.isComplete, isTrue);
});
});
}
43 changes: 43 additions & 0 deletions app_dart/test/model/firestore/ci_staging_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -324,6 +324,49 @@ For CI stage engine:
);
});

test('handles a test flip-flop (neutral) after re-running', () async {
firestoreService.putDocument(
Document(
name: expectedName,
fields: {
CiStaging.kRemainingField: 1.toValue(),
CiStaging.kFailedField: 1.toValue(),
CiStaging.kTotalField: 1.toValue(),
CiStaging.kCheckRunGuardField: '{}'.toValue(),
'MacOS build_test': TaskConclusion.failure.name.toValue(),
},
),
);

final future = CiStaging.markConclusion(
firestoreService: firestoreService,
slug: slug,
sha: '1234',
stage: CiStage.fusionEngineBuild,
checkRun: 'MacOS build_test',
conclusion: TaskConclusion.neutral,
);

final result = await future;
// Remaining == 1 because our test was already concluded.
expect(
result,
const PresubmitGuardConclusion(
remaining: 1,
result: PresubmitGuardConclusionResult.ok,
failed: 0,
checkRunGuard: '{}',
summary: 'All tests passed',
details: '''
For CI stage engine:
Total check runs scheduled: 1
Pending: 1
Failed: 0
''',
),
);
});

test('ignored repeat failures', () async {
firestoreService.putDocument(
Document(
Expand Down
2 changes: 1 addition & 1 deletion auto_submit/pubspec.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ homepage: https://github.com/flutter/cocoon/blob/main/autosubmit
resolution: workspace

environment:
sdk: ^3.9.0
sdk: ^3.10.0

dependencies:
appengine: 0.13.11
Expand Down
20 changes: 10 additions & 10 deletions dashboard/lib/service/data_seeder.dart
Original file line number Diff line number Diff line change
Expand Up @@ -411,7 +411,7 @@ class DataSeeder {
.where((status) => status.isFailure)
.length;
final remainingBuilds = builds.values
.where((status) => !status.isBuildCompleted)
.where((status) => !status.isComplete)
.length;

return PresubmitGuard(
Expand Down Expand Up @@ -448,20 +448,20 @@ class DataSeeder {
creationTime: creationTime,
buildNumber: 1337 + attemptNumber,
summary: switch (status) {
TaskStatus.succeeded =>
.succeeded =>
'[INFO] Starting task $buildName...\n[SUCCESS] All tests passed (452/452)',
TaskStatus.failed =>
.failed =>
'[INFO] Starting task $buildName...\n[ERROR] Test failed: Dummy Tests',
TaskStatus.infraFailure =>
.infraFailure =>
'[INFO] Starting task $buildName...\n[ERROR] Infrastructure failure: Dummy Tests',
TaskStatus.cancelled =>
.cancelled =>
'[INFO] Starting task $buildName...\n[ERROR] Test cancelled: Dummy Tests',
TaskStatus.inProgress => '[INFO] Starting task $buildName...',
TaskStatus.waitingForBackfill => null,
TaskStatus.skipped =>
.inProgress => '[INFO] Starting task $buildName...',
.waitingForBackfill => null,
.skipped =>
'[INFO] Starting task $buildName...\n[INFO] Test skipped: Dummy Tests',
TaskStatus.neutral =>
'[INFO] Starting task $buildName...\n[INFO] Test status neutral: Dummy Tests',
.neutral =>
'[INFO] Starting task $buildName...\n[INFO] Test neutral: Dummy Tests',
},
startTime: creationTime + 30000,
endTime: creationTime + 60000,
Expand Down
Loading
Loading