Skip to content

Commit e6b3661

Browse files
authored
Remove MockFirestoreService, add transactions to FakeFiretoreService (#4511)
Closes flutter/flutter#167016.
1 parent 3076c73 commit e6b3661

File tree

17 files changed

+1214
-1678
lines changed

17 files changed

+1214
-1678
lines changed

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

Lines changed: 33 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -168,6 +168,25 @@ final class CiStaging extends AppDocument<CiStaging> {
168168
/// The check_run to complete when this stage is closed.
169169
String get checkRunGuard => fields[kCheckRunGuardField]!.stringValue!;
170170

171+
static const keysOfImport = [
172+
kRemainingField,
173+
kTotalField,
174+
kFailedField,
175+
kCheckRunGuardField,
176+
fieldRepoFullPath,
177+
fieldCommitSha,
178+
fieldStage,
179+
];
180+
181+
/// The recorded check-runs, a map of "test_name": "check_run id".
182+
Map<String, TaskConclusion> get checkRuns {
183+
return {
184+
for (final MapEntry(:key, :value) in fields.entries)
185+
if (!keysOfImport.contains(key))
186+
key: TaskConclusion.fromName(value.stringValue),
187+
};
188+
}
189+
171190
/// Mark a [checkRun] for a given [stage] with [conclusion].
172191
///
173192
/// Returns a [StagingConclusion] record or throws. If the check_run was
@@ -190,17 +209,7 @@ final class CiStaging extends AppDocument<CiStaging> {
190209
// updated correctly. For that to happen correctly; we need to perform a
191210
// read of the document in the transaction as well. So start the transaction
192211
// first thing.
193-
final docRes = await firestoreService.documentResource();
194-
final transactionResponse = await docRes.beginTransaction(
195-
BeginTransactionRequest(
196-
options: TransactionOptions(readWrite: ReadWrite()),
197-
),
198-
kDatabase,
199-
);
200-
final transaction = transactionResponse.transaction;
201-
if (transaction == null) {
202-
throw '$logCrumb: transaction was null when updating $conclusion';
203-
}
212+
final transaction = await firestoreService.beginTransaction();
204213

205214
var remaining = -1;
206215
var failed = -1;
@@ -215,7 +224,10 @@ final class CiStaging extends AppDocument<CiStaging> {
215224
try {
216225
// First: read the fields we want to change.
217226
final documentName = documentNameFor(slug: slug, stage: stage, sha: sha);
218-
doc = await docRes.get(documentName, transaction: transaction);
227+
doc = await firestoreService.getDocument(
228+
documentName,
229+
transaction: transaction,
230+
);
219231

220232
final fields = doc.fields;
221233
if (fields == null) {
@@ -254,10 +266,7 @@ final class CiStaging extends AppDocument<CiStaging> {
254266
log.info(
255267
'$logCrumb: $checkRun not present in doc for $transaction / $doc',
256268
);
257-
await docRes.rollback(
258-
RollbackRequest(transaction: transaction),
259-
kDatabase,
260-
);
269+
await firestoreService.rollback(transaction);
261270
return StagingConclusion(
262271
result: StagingConclusionResult.missing,
263272
remaining: remaining,
@@ -326,10 +335,7 @@ final class CiStaging extends AppDocument<CiStaging> {
326335
if (e.status == 404) {
327336
// An attempt to read a document not in firestore should not be retried.
328337
log.info('$logCrumb: staging document not found for $transaction');
329-
await docRes.rollback(
330-
RollbackRequest(transaction: transaction),
331-
kDatabase,
332-
);
338+
await firestoreService.rollback(transaction);
333339
return StagingConclusion(
334340
result: StagingConclusionResult.internalError,
335341
remaining: -1,
@@ -347,28 +353,21 @@ $stack
347353
);
348354
}
349355
// All other errors should bubble up and be retried.
350-
await docRes.rollback(
351-
RollbackRequest(transaction: transaction),
352-
kDatabase,
353-
);
356+
await firestoreService.rollback(transaction);
354357
rethrow;
355358
} catch (e) {
356359
// All other errors should bubble up and be retried.
357-
await docRes.rollback(
358-
RollbackRequest(transaction: transaction),
359-
kDatabase,
360-
);
360+
await firestoreService.rollback(transaction);
361361
rethrow;
362362
}
363363

364364
// Commit this write firebase and if no one else was writing at the same time, return success.
365365
// If this commit fails, that means someone else modified firestore and the caller should try again.
366366
// We do not need to rollback the transaction; firebase documentation says a failed commit takes care of that.
367-
final commitRequest = CommitRequest(
368-
transaction: transaction,
369-
writes: documentsToWrites([doc], exists: true),
367+
final response = await firestoreService.commit(
368+
transaction,
369+
documentsToWrites([doc], exists: true),
370370
);
371-
final response = await docRes.commit(commitRequest, kDatabase);
372371
log.info(
373372
'$logCrumb: results = ${response.writeResults?.map((e) => e.toJson())}',
374373
);
@@ -433,12 +432,9 @@ For CI stage $stage:
433432
// Calling createDocument multiple times for the same documentId will return a 409 - ALREADY_EXISTS error;
434433
// this is good because it means we don't have to do any transactions.
435434
// curl -X POST -H "Content-Type: application/json" -H "Authorization: Bearer <TOKEN>" "https://firestore.googleapis.com/v1beta1/projects/flutter-dashboard/databases/cocoon/documents/ciStaging?documentId=foo_bar_baz" -d '{"fields": {"test": {"stringValue": "baz"}}}'
436-
final databasesDocumentsResource =
437-
await firestoreService.documentResource();
438-
final newDoc = await databasesDocumentsResource.createDocument(
435+
final newDoc = await firestoreService.createDocument(
439436
document,
440-
kDocumentParent,
441-
_collectionId,
437+
collectionId: _collectionId,
442438
documentId:
443439
documentIdFor(
444440
slug: slug,

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

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -25,8 +25,7 @@ const String kGithubBuildStatusUpdatesField = 'updates';
2525
/// /projects/flutter-dashboard/databases/cocoon/commits/
2626
/// document: <this.head>
2727
/// ```
28-
/*final - TODO(matanlurey): Can't add because of MockFirestoreService. */
29-
class GithubBuildStatus extends AppDocument<GithubBuildStatus> {
28+
final class GithubBuildStatus extends AppDocument<GithubBuildStatus> {
3029
static AppDocumentId<GithubBuildStatus> documentIdFor({
3130
required String headSha,
3231
}) {

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

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -25,8 +25,7 @@ const String kGithubGoldStatusRepositoryField = 'repository';
2525
/// /projects/flutter-dashboard/databases/cocoon/commits/
2626
/// document: <this.slug.owner>_<this.slug.name>_<this.prNumber>
2727
/// ```
28-
/*final - TODO(matanlurey): Can't add because of MockFirestoreService. */
29-
class GithubGoldStatus extends AppDocument<GithubGoldStatus> {
28+
final class GithubGoldStatus extends AppDocument<GithubGoldStatus> {
3029
static AppDocumentId<GithubGoldStatus> documentIdFor({
3130
required String owner,
3231
required String repo,

app_dart/lib/src/request_handlers/rerun_prod_task.dart

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ import '../model/firestore/task.dart' as fs;
1313
import '../request_handling/api_request_handler.dart';
1414
import '../request_handling/body.dart';
1515
import '../request_handling/exceptions.dart';
16-
import '../service/firestore.dart';
16+
import '../service/firestore/commit_and_tasks.dart';
1717
import '../service/luci_build_service.dart';
1818
import '../service/luci_build_service/build_tags.dart';
1919
import '../service/luci_build_service/opaque_commit.dart';

app_dart/lib/src/request_handlers/scheduler/vacuum_stale_tasks.dart

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ import '../../model/appengine/task.dart' as ds;
1717
import '../../model/firestore/commit.dart' as fs;
1818
import '../../model/firestore/task.dart' as fs;
1919
import '../../service/datastore.dart';
20+
import '../../service/firestore/commit_and_tasks.dart';
2021

2122
/// Vacuum stale tasks.
2223
///

app_dart/lib/src/service/build_status_provider.dart

Lines changed: 14 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44

55
import 'dart:async';
66

7+
import 'package:cocoon_server/logging.dart';
78
import 'package:github/github.dart';
89
import 'package:meta/meta.dart';
910

@@ -49,44 +50,47 @@ interface class BuildStatusService {
4950
///
5051
/// Tree status is only for [defaultBranches].
5152
Future<BuildStatus?> calculateCumulativeStatus(RepositorySlug slug) async {
52-
final statuses = await retrieveCommitStatusFirestore(
53+
final commits = await retrieveCommitStatusFirestore(
5354
limit: numberOfCommitsToReferenceForTreeStatus,
5455
slug: slug,
5556
);
56-
if (statuses.isEmpty) {
57+
if (commits.isEmpty) {
58+
log.info('Tree status of failure for $slug: no commits found');
5759
return BuildStatus.failure();
5860
}
5961

60-
final tasksInProgress = _findTasksRelevantToLatestStatus(statuses);
61-
if (tasksInProgress.isEmpty) {
62+
final mostRecentTasks = _findTasksRelevantToLatestStatus(commits);
63+
if (mostRecentTasks.isEmpty) {
64+
log.info('Tree status of failure for $slug: no recent tasks found');
6265
return BuildStatus.failure();
6366
}
6467

6568
final failedTasks = <String>[];
66-
for (var status in statuses) {
69+
for (var status in commits) {
6770
for (var task in status.tasks) {
6871
/// If a task [isRelevantToLatestStatus] but has not run yet, we look
6972
/// for a previous run of the task from the previous commit.
70-
final isRelevantToLatestStatus = tasksInProgress.containsKey(
73+
final isRelevantToLatestStatus = mostRecentTasks.containsKey(
7174
task.taskName,
7275
);
7376

7477
/// Tasks that are not relevant to the latest status will have a
7578
/// null value in the map.
76-
final taskInProgress = tasksInProgress[task.taskName] ?? true;
79+
final taskInProgress = mostRecentTasks[task.taskName] ?? true;
7780

7881
if (isRelevantToLatestStatus && taskInProgress) {
7982
if (task.bringup || _isSuccessful(task)) {
8083
/// This task no longer needs to be checked to see if it causing
8184
/// the build status to fail.
82-
tasksInProgress[task.taskName] = false;
85+
mostRecentTasks[task.taskName] = false;
8386
} else if (_isFailed(task) || _isRerunning(task)) {
87+
log.debug('${task.taskName} (${task.commitSha}) is failing');
8488
failedTasks.add(task.taskName);
8589

8690
/// This task no longer needs to be checked to see if its causing
8791
/// the build status to fail since its been
8892
/// added to the failedTasks list.
89-
tasksInProgress[task.taskName] = false;
93+
mostRecentTasks[task.taskName] = false;
9094
}
9195
}
9296
}
@@ -129,6 +133,7 @@ interface class BuildStatusService {
129133
);
130134
return [
131135
for (final commit in commits)
136+
// It's not obvious, but this is ordered by task creation time, descending.
132137
CommitTasksStatus(commit, await firestore.queryCommitTasks(commit.sha)),
133138
];
134139
}

0 commit comments

Comments
 (0)