Refactor CI Results to use extension types (Part 1) (#211)
Introduce ResultRecord, ChangeRecord, and ConfigurationRecord extension types to wrap Firestore documents. Refactor builder.dart, upload_results_to_database.dart, reverted_changes.dart, status.dart, and corresponding tests to use these extension types instead of raw maps and SafeDocument. Keep tryjobs, reviews, commits, and comments using SafeDocument/Commit for now, to be refactored in subsequent PRs.
TAG=agy
CONV=a2878fa6-b79f-4b1d-9635-654148d5ceb2
diff --git a/builder/bin/upload_results_to_database.dart b/builder/bin/upload_results_to_database.dart
index 3faea3c..1e8adc7 100644
--- a/builder/bin/upload_results_to_database.dart
+++ b/builder/bin/upload_results_to_database.dart
@@ -14,18 +14,29 @@
late BuildInfo buildInfo;
-Future<List<Map<String, dynamic>>> readChangedResults(File resultsFile) async {
- final lines = (await resultsFile.readAsLines()).map(
- (line) => jsonDecode(line)! as Map<String, dynamic>,
- );
+Future<List<ChangeRecord>> readChangedResults(File resultsFile) async {
+ final lines = await resultsFile.readAsLines();
if (lines.isEmpty) {
print('Empty input results.json file');
exit(1);
}
- buildInfo = BuildInfo.fromResult(lines.first, {
- for (final line in lines) line[fConfiguration],
- });
- return lines.where(isChangedResult).toList();
+
+ ChangeRecord? firstChange;
+ final changes = <ChangeRecord>[];
+ final configurations = <String>{};
+ for (final change
+ in lines
+ .map(jsonDecode)
+ .cast<Map<String, dynamic>>()
+ .map(ChangeRecord.fromMap)) {
+ firstChange ??= change;
+ configurations.add(change.configuration);
+ if (change.isChangedResult) {
+ changes.add(change);
+ }
+ }
+ buildInfo = BuildInfo.fromResult(firstChange!, configurations);
+ return changes;
}
File fileOption(ArgResults options, String name) {
diff --git a/builder/lib/src/builder.dart b/builder/lib/src/builder.dart
index 16b1145..f41fa45 100644
--- a/builder/lib/src/builder.dart
+++ b/builder/lib/src/builder.dart
@@ -41,7 +41,7 @@
void log(String string) => firestore.log(string);
- Future<BuildStatus> process(List<Map<String, dynamic>> changes) async {
+ Future<BuildStatus> process(List<ChangeRecord> changes) async {
log('store build commits info');
await storeBuildCommitsInfo();
log('update build info');
@@ -70,7 +70,7 @@
}..removeWhere((key, value) => value.isEmpty);
} catch (e) {
log('Failed to fetch unapproved failures: $e');
- status.unapprovedFailures = {'failed': []};
+ status.unapprovedFailures = {'failed': <ResultRecord>[]};
status.success = false;
}
final report = [
@@ -153,23 +153,23 @@
}
}
- Future<void> guardedStoreChange(Map<String, dynamic> change) =>
+ Future<void> guardedStoreChange(ChangeRecord change) =>
testNameLock.guardedCall(storeChange, change);
- Future<void> storeChange(Map<String, dynamic> change) async {
+ Future<void> storeChange(ChangeRecord change) async {
countChanges++;
await reviewsFetched;
- transformChange(change);
- final failure = isFailure(change);
+ change.transform();
+ final failure = change.isFailure;
bool approved;
var result = await firestore.findResult(change, startIndex, endIndex);
var activeResults = await firestore.findActiveResults(
- change['name'],
- change['configuration'],
+ change.name,
+ change.configuration,
);
if (result == null) {
final approvingIndex =
- tryApprovals[testResult(change)] ??
+ tryApprovals[change.testResult] ??
allRevertedChanges
.firstWhereOrNull(
(revertedChange) => revertedChange.approveRevert(change),
@@ -189,14 +189,14 @@
countApprovalsCopied++;
if (countApprovalsCopied <= 10) {
approvalMessages.add(
- 'Copied approval of result ${testResult(change)}',
+ 'Copied approval of result ${change.testResult}',
);
}
}
} else {
approved = await firestore.updateResult(
result,
- change['configuration'],
+ change.configuration,
startIndex,
endIndex,
failure: failure,
@@ -206,14 +206,12 @@
for (final activeResult in activeResults) {
// Log error message if any expected invariants are violated
- if (activeResult.getInt(fBlamelistEndIndex)! >= startIndex ||
- !(activeResult
- .getList(fActiveConfigurations)
- ?.contains(change['configuration']) ??
+ if (activeResult.blamelistEndIndex >= startIndex ||
+ !(activeResult.activeConfigurations?.contains(change.configuration) ??
false)) {
log(
'Unexpected active result when processing new change:\n'
- 'Active result: ${untagMap(activeResult.fields)}\n\n'
+ 'Active result: $activeResult\n\n'
'Change: $change\n\n'
'approved: $approved',
);
@@ -221,12 +219,12 @@
// Removes the configuration from the list of active configurations.
await firestore.removeActiveConfiguration(
activeResult,
- change['configuration'],
+ change.configuration,
);
}
}
- Future<List<SafeDocument>> unapprovedFailuresForConfiguration(
+ Future<List<ResultRecord>> unapprovedFailuresForConfiguration(
String configuration,
) async {
final failures = await firestore.findUnapprovedFailures(
@@ -237,37 +235,37 @@
return failures;
}
- Future<void> addBlamelistCommits(SafeDocument result) async {
+ Future<void> addBlamelistCommits(ResultRecord result) async {
final startCommit = await commitsCache.getCommitByIndex(
- result.getInt(fBlamelistStartIndex)!,
+ result.blamelistStartIndex,
);
- result.fields[fBlamelistStartCommit] = taggedValue(startCommit.hash);
+ result.blamelistStartCommit = startCommit.hash;
final endCommit = await commitsCache.getCommitByIndex(
- result.getInt(fBlamelistEndIndex)!,
+ result.blamelistEndIndex,
);
- result.fields[fBlamelistEndCommit] = taggedValue(endCommit.hash);
+ result.blamelistEndCommit = endCommit.hash;
}
}
-Map<String, dynamic> constructResult(
- Map<String, dynamic> change,
+ResultRecord constructResult(
+ ChangeRecord change,
int startIndex,
int endIndex, {
required bool approved,
int? landedReviewIndex,
required bool failure,
}) {
- return {
- fName: change[fName],
- fResult: change[fResult],
- fPreviousResult: change[fPreviousResult],
- fExpected: change[fExpected],
+ return ResultRecord.fromMap({
+ fName: change.name,
+ fResult: change.result,
+ fPreviousResult: change.previousResult,
+ fExpected: change.expected,
fBlamelistStartIndex: startIndex,
fBlamelistEndIndex: endIndex,
if (startIndex != endIndex && approved) fPinnedIndex: landedReviewIndex,
- fConfigurations: <String>[change['configuration']],
+ fConfigurations: <String>[change.configuration],
fApproved: approved,
if (failure) fActive: true,
- if (failure) fActiveConfigurations: <String>[change['configuration']],
- };
+ if (failure) fActiveConfigurations: <String>[change.configuration],
+ });
}
diff --git a/builder/lib/src/data.dart b/builder/lib/src/data.dart
new file mode 100644
index 0000000..b1baaa9
--- /dev/null
+++ b/builder/lib/src/data.dart
@@ -0,0 +1,91 @@
+// Copyright (c) 2026, the Dart project authors. Please see the AUTHORS file
+// for details. 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:googleapis/firestore/v1.dart';
+import 'firestore_helpers.dart';
+import 'result.dart';
+
+extension type ResultRecord(Document doc) {
+ ResultRecord.fromMap(Map<String, dynamic> data)
+ : this(Document(fields: taggedMap(data)));
+
+ String get name => doc.fields![fName]!.stringValue!;
+ set name(String value) => doc.fields![fName] = taggedValue(value);
+ String get result => doc.fields![fResult]!.stringValue!;
+ String get previousResult => doc.fields![fPreviousResult]!.stringValue!;
+ String get expected => doc.fields![fExpected]!.stringValue!;
+ int get blamelistStartIndex =>
+ int.parse(doc.fields![fBlamelistStartIndex]!.integerValue!);
+ int get blamelistEndIndex =>
+ int.parse(doc.fields![fBlamelistEndIndex]!.integerValue!);
+ bool get approved => doc.fields![fApproved]?.booleanValue ?? false;
+ bool get active => doc.fields![fActive]?.booleanValue ?? false;
+ List<String> get configurations => doc
+ .fields![fConfigurations]!
+ .arrayValue!
+ .values!
+ .map((v) => v.stringValue!)
+ .toList();
+ List<String>? get activeConfigurations {
+ final val = doc.fields![fActiveConfigurations];
+ if (val == null || val.nullValue != null) return null;
+ return val.arrayValue?.values?.map((v) => v.stringValue!).toList() ?? [];
+ }
+
+ int? get pinnedIndex =>
+ int.tryParse(doc.fields![fPinnedIndex]?.integerValue ?? '');
+ String? get blamelistStartCommit =>
+ doc.fields![fBlamelistStartCommit]?.stringValue;
+ String? get blamelistEndCommit =>
+ doc.fields![fBlamelistEndCommit]?.stringValue;
+
+ set blamelistStartCommit(String? value) =>
+ doc.fields![fBlamelistStartCommit] = taggedValue(value);
+ set blamelistEndCommit(String? value) =>
+ doc.fields![fBlamelistEndCommit] = taggedValue(value);
+
+ String get testResult => [name, result, previousResult, expected].join(' ');
+
+ Map<String, dynamic> toJson() =>
+ untagMap(doc.fields!).cast<String, dynamic>();
+}
+
+extension type ChangeRecord(Document doc) implements ResultRecord {
+ ChangeRecord.fromMap(Map<String, dynamic> data)
+ : this(Document(fields: taggedMap(data)));
+
+ String get configuration => doc.fields!['configuration']!.stringValue!;
+ String get builderName => doc.fields![fBuilderName]!.stringValue!;
+ int get buildNumber => int.parse(doc.fields![fBuildNumber]!.stringValue!);
+ String get commitHash => doc.fields![fCommitHash]!.stringValue!;
+ set commitHash(String value) => doc.fields![fCommitHash] = taggedValue(value);
+ String? get previousCommitHash =>
+ doc.fields![fPreviousCommitHash]?.stringValue;
+
+ bool get changed => doc.fields![fChanged]?.booleanValue ?? false;
+ bool get flaky => doc.fields![fFlaky]?.booleanValue ?? false;
+ bool get previousFlaky => doc.fields![fPreviousFlaky]?.booleanValue ?? false;
+ bool get matches => doc.fields![fMatches]?.booleanValue ?? false;
+
+ bool get isChangedResult => changed && (!flaky || !previousFlaky);
+
+ bool get isFailure => !matches && result != 'flaky';
+
+ void transform() {
+ if (doc.fields![fPreviousResult]?.stringValue == null) {
+ doc.fields![fPreviousResult] = taggedValue('new test');
+ }
+ if (doc.fields![fPreviousFlaky]?.booleanValue == true) {
+ doc.fields![fPreviousResult] = taggedValue('flaky');
+ }
+ if (doc.fields![fFlaky]?.booleanValue == true) {
+ doc.fields![fResult] = taggedValue('flaky');
+ doc.fields![fMatches] = taggedValue(false);
+ }
+ }
+}
+
+extension type ConfigurationRecord(Document doc) {
+ String get builder => doc.fields!['builder']!.stringValue!;
+}
diff --git a/builder/lib/src/firestore.dart b/builder/lib/src/firestore.dart
index 5139a18..e3a8713 100644
--- a/builder/lib/src/firestore.dart
+++ b/builder/lib/src/firestore.dart
@@ -64,7 +64,24 @@
..where = where
..orderBy = orderBy != null ? [orderBy] : null
..limit = limit;
- return runQuery(query, parent: parent);
+ final results = await runQuery(query, parent: parent);
+ return results.map((d) => SafeDocument(d)).toList();
+ }
+
+ Future<List<T>> _query<T>({
+ required String from,
+ Filter? where,
+ Order? orderBy,
+ int? limit,
+ String? parent,
+ }) async {
+ final query = StructuredQuery()
+ ..from = inCollection(from)
+ ..where = where
+ ..orderBy = orderBy != null ? [orderBy] : null
+ ..limit = limit;
+ final results = await runQuery(query, parent: parent);
+ return results.cast<T>();
}
Future<Document> getDocument(String path) async {
@@ -100,7 +117,7 @@
String get database => 'projects/$project/databases/(default)';
String get documents => '$database/documents';
- Future<List<SafeDocument>> runQuery(
+ Future<List<Document>> runQuery(
StructuredQuery query, {
String? parent,
}) async {
@@ -113,8 +130,7 @@
if (queryResponse.first.document == null) return [];
documentsFetched += queryResponse.length;
return [
- for (final responseElement in queryResponse)
- SafeDocument(responseElement.document!),
+ for (final responseElement in queryResponse) responseElement.document!,
];
}
@@ -273,15 +289,15 @@
}
Future<String?> findResult(
- Map<String, dynamic> change,
+ ResultRecord change,
int startIndex,
int endIndex,
) async {
- final name = change['name'] as String;
- final result = change['result'] as String;
- final previousResult = change['previous_result'] as String;
- final expected = change['expected'] as String;
- final snapshot = await query(
+ final name = change.name;
+ final result = change.result;
+ final previousResult = change.previousResult;
+ final expected = change.expected;
+ final snapshot = await _query<ResultRecord>(
from: 'results',
orderBy: orderBy('blamelist_end_index', false),
where: compositeFilter([
@@ -293,19 +309,19 @@
limit: 5,
);
- bool blamelistIncludesChange(SafeDocument document) {
- final before = endIndex < document.getInt('blamelist_start_index')!;
- final after = startIndex > document.getInt('blamelist_end_index')!;
+ bool blamelistIncludesChange(ResultRecord document) {
+ final before = endIndex < document.blamelistStartIndex;
+ final after = startIndex > document.blamelistEndIndex;
return !before && !after;
}
- return snapshot.firstWhereOrNull(blamelistIncludesChange)?.name;
+ return snapshot.firstWhereOrNull(blamelistIncludesChange)?.doc.name;
}
- Future<Document> storeResult(Map<String, dynamic> result) async {
+ Future<Document> storeResult(ResultRecord result) async {
for (var retries = 0; true; retries++) {
try {
- final document = Document()..fields = taggedMap(result);
+ final document = result.doc;
final createdDocument = await firestore.projects.databases.documents
.createDocument(document, documents, 'results');
documentsWritten++;
@@ -313,7 +329,7 @@
} catch (e) {
log('Failed creating document at path $documents/results.');
log('Retrying in 1000 ms.');
- log('Document contents: ${jsonEncode(result)}\n');
+ log('Document contents: ${jsonEncode(result.toJson())}\n');
log('$e');
if (retries > 2) {
rethrow;
@@ -325,7 +341,7 @@
Future<bool> updateResult(
String result,
- String? configuration,
+ String configuration,
int startIndex,
int endIndex, {
required bool failure,
@@ -333,12 +349,11 @@
late bool approved;
await retryCommit(() async {
final document = await getDocument(result);
- final data = SafeDocument(document);
- // Allow missing 'approved' field during transition period.
- approved = data.getBool('approved') ?? false;
+ final data = ResultRecord(document);
+ approved = data.approved;
// Add the new configuration and narrow the blamelist.
- final newStart = max(startIndex, data.getInt('blamelist_start_index')!);
- final newEnd = min(endIndex, data.getInt('blamelist_end_index')!);
+ final newStart = max(startIndex, data.blamelistStartIndex);
+ final newEnd = min(endIndex, data.blamelistEndIndex);
// TODO(karlklose): check for pinned, and remove the pin if the new range
// doesn't include it?
final updates = [
@@ -394,35 +409,33 @@
/// Returns all results which are either pinned to or have a range that is
/// this single index. // TODO: rename this function
- Future<List<Map<String, Value>>> findRevertedChanges(int index) async {
- final pinnedResults = await query(
+ Future<List<ResultRecord>> findRevertedChanges(int index) async {
+ final results = await _query<ResultRecord>(
from: 'results',
where: fieldEquals('pinned_index', index),
);
- final results = pinnedResults.map((response) => response.fields).toList();
- final unpinnedResults = await query(
+ final unpinnedResults = await _query<ResultRecord>(
from: 'results',
where: fieldEquals('blamelist_end_index', index),
);
for (final data in unpinnedResults) {
- if (data.getInt('blamelist_start_index') == index &&
- data.isNull('pinned_index')) {
- results.add(data.fields);
+ if (data.blamelistStartIndex == index && data.pinnedIndex == null) {
+ results.add(data);
}
}
return results;
}
Future<bool> storeTryChange(
- Map<String, dynamic> change,
+ ChangeRecord change,
int review,
int patchset,
) async {
- final name = change['name'] as String;
- final result = change['result'] as String;
- final expected = change['expected'] as String;
- final previousResult = change['previous_result'] as String;
- final configuration = change['configuration'] as String;
+ final name = change.name;
+ final result = change.result;
+ final expected = change.expected;
+ final previousResult = change.previousResult;
+ final configuration = change.configuration;
// Find an existing result record for this test on this patchset.
final responses = await query(
@@ -517,23 +530,23 @@
/// Removes [configuration] from the active configurations and marks the
/// active result inactive when we remove the last active config.
Future<void> removeActiveConfiguration(
- SafeDocument activeResult,
- String? configuration,
+ ResultRecord activeResult,
+ String configuration,
) async {
- final configurations = activeResult.getList('active_configurations')!;
+ final configurations = activeResult.activeConfigurations!;
assert(configurations.contains(configuration));
await removeArrayEntry(
- activeResult,
+ activeResult.doc,
'active_configurations',
taggedValue(configuration),
);
- final document = await getDocument(activeResult.name);
- activeResult = SafeDocument(document);
- if (activeResult.getList('active_configurations')?.isEmpty == true) {
- activeResult.fields.remove('active_configurations');
- activeResult.fields.remove('active');
+ final document = await getDocument(activeResult.doc.name!);
+ activeResult = ResultRecord(document);
+ if (activeResult.activeConfigurations?.isEmpty == true) {
+ activeResult.doc.fields!.remove('active_configurations');
+ activeResult.doc.fields!.remove('active');
final write = Write()
- ..update = activeResult.toDocument()
+ ..update = activeResult.doc
..updateMask = (DocumentMask()
..fieldPaths = ['active', 'active_configurations']);
await _executeWrite([write]);
@@ -541,7 +554,7 @@
}
Future<void> removeArrayEntry(
- SafeDocument document,
+ Document document,
String fieldName,
Value entry,
) async {
@@ -557,11 +570,11 @@
]);
}
- Future<List<SafeDocument>> findActiveResults(
+ Future<List<ResultRecord>> findActiveResults(
String name,
String configuration,
) async {
- final results = await query(
+ final results = await _query<ResultRecord>(
from: 'results',
where: compositeFilter([
arrayContains('active_configurations', configuration),
@@ -580,11 +593,11 @@
return results;
}
- Future<List<SafeDocument>> findUnapprovedFailures(
+ Future<List<ResultRecord>> findUnapprovedFailures(
String configuration,
int limit,
) async {
- final results = await query(
+ final results = await _query<ResultRecord>(
from: 'results',
where: compositeFilter([
arrayContains('active_configurations', configuration),
diff --git a/builder/lib/src/firestore_helpers.dart b/builder/lib/src/firestore_helpers.dart
index 1ea01fb..9e1903f 100644
--- a/builder/lib/src/firestore_helpers.dart
+++ b/builder/lib/src/firestore_helpers.dart
@@ -4,6 +4,8 @@
import 'package:googleapis/firestore/v1.dart';
+export 'data.dart';
+
class SafeDocument {
final String name;
final Map<String, Value> fields;
diff --git a/builder/lib/src/result.dart b/builder/lib/src/result.dart
index 526c61f..edcca29 100644
--- a/builder/lib/src/result.dart
+++ b/builder/lib/src/result.dart
@@ -7,6 +7,8 @@
import 'package:googleapis/firestore/v1.dart' show Value;
+import 'firestore_helpers.dart';
+
// Field names of Result document fields
const fName = 'name';
const fResult = 'result';
@@ -44,26 +46,6 @@
const fRevertOf = 'revert_of';
const fRelandOf = 'reland_of';
-bool isChangedResult(Map<String, dynamic> change) =>
- change[fChanged] && (!change[fFlaky] || !change[fPreviousFlaky]);
-
-/// Whether the change will be marked as an active failure.
-/// New flaky tests will not be marked active, so they will appear in the
-/// results feed "all", but not turn the builder red
-bool isFailure(Map<String, dynamic> change) =>
- !change[fMatches] && change[fResult] != 'flaky';
-
-void transformChange(Map<String, dynamic> change) {
- change[fPreviousResult] ??= 'new test';
- if (change[fPreviousFlaky]) {
- change[fPreviousResult] = 'flaky';
- }
- if (change[fFlaky]) {
- change[fResult] = 'flaky';
- change[fMatches] = false;
- }
-}
-
String? fromStringOrValue(dynamic value) {
return value is Value ? value.stringValue : value;
}
@@ -86,17 +68,17 @@
final String? previousCommitHash;
final Set<String> configurations;
- BuildInfo(Map<String, dynamic> result, this.configurations)
- : builderName = result[fBuilderName],
- buildNumber = int.parse(result[fBuildNumber]),
- commitRef = result[fCommitHash],
- previousCommitHash = result[fPreviousCommitHash];
+ BuildInfo(ChangeRecord result, this.configurations)
+ : builderName = result.builderName,
+ buildNumber = result.buildNumber,
+ commitRef = result.commitHash,
+ previousCommitHash = result.previousCommitHash;
factory BuildInfo.fromResult(
- Map<String, dynamic> result,
+ ChangeRecord result,
Set<String> configurations,
) {
- final commitRef = result[fCommitHash];
+ final commitRef = result.commitHash;
final match = commitRefRegExp.matchAsPrefix(commitRef);
if (match == null) {
return BuildInfo(result, configurations);
@@ -122,10 +104,10 @@
final locks = <String, Future<void>>{};
Future<void> guardedCall(
- Future<void> Function(Map<String, dynamic> change) f,
- Map<String, dynamic> change,
+ Future<void> Function(ChangeRecord change) f,
+ ChangeRecord change,
) async {
- final name = change[fName]!;
+ final name = change.name;
while (locks.containsKey(name)) {
await locks[name];
}
diff --git a/builder/lib/src/reverted_changes.dart b/builder/lib/src/reverted_changes.dart
index 5085ebd..870cd17 100644
--- a/builder/lib/src/reverted_changes.dart
+++ b/builder/lib/src/reverted_changes.dart
@@ -3,10 +3,8 @@
// BSD-style license that can be found in the LICENSE file.
import 'package:collection/collection.dart';
-import 'package:googleapis/firestore/v1.dart';
import 'firestore.dart';
-import 'result.dart';
Future<RevertedChanges> getRevertedChanges(
String reverted,
@@ -23,15 +21,15 @@
index,
revertIndex,
changes,
- groupBy(changes, (change) => getValue(change[fName]!)),
+ groupBy(changes, (change) => change.name),
);
}
class RevertedChanges {
final int index;
final int revertIndex;
- final List<Map<String, Value>> changes;
- final Map<String, List<Map<String, Value>>> changesForTest;
+ final List<ResultRecord> changes;
+ final Map<String, List<ResultRecord>> changesForTest;
RevertedChanges(
this.index,
@@ -40,12 +38,10 @@
this.changesForTest,
);
- bool approveRevert(Map<String, dynamic> revert) {
- final reverted = changesForTest[revert[fName]];
- return isFailure(revert) &&
+ bool approveRevert(ChangeRecord revert) {
+ final reverted = changesForTest[revert.name];
+ return revert.isFailure &&
reverted != null &&
- reverted.any(
- (change) => revert[fResult] == getValue(change[fPreviousResult]!),
- );
+ reverted.any((change) => revert.result == change.previousResult);
}
}
diff --git a/builder/lib/src/status.dart b/builder/lib/src/status.dart
index 5acc843..d93e7c3 100644
--- a/builder/lib/src/status.dart
+++ b/builder/lib/src/status.dart
@@ -5,13 +5,12 @@
import 'dart:convert' show jsonEncode;
import 'firestore.dart';
-import 'result.dart';
class BuildStatus {
static const unapprovedFailuresLimit = 10;
bool success = true;
bool truncatedResults = false;
- Map<String, List<SafeDocument>> unapprovedFailures = {};
+ Map<String, List<ResultRecord>> unapprovedFailures = {};
String toJson() {
return jsonEncode({
@@ -39,13 +38,13 @@
}
}
-String resultLine(SafeDocument result) {
- final name = result.getString(fName);
- final previous = result.getString(fPreviousResult);
- final current = result.getString(fResult);
- final expected = result.getString(fExpected);
- final start = result.getString(fBlamelistStartCommit);
- final end = result.getString(fBlamelistEndCommit);
+String resultLine(ResultRecord result) {
+ final name = result.name;
+ final previous = result.previousResult;
+ final current = result.result;
+ final expected = result.expected;
+ final start = result.blamelistStartCommit!;
+ final end = result.blamelistEndCommit!;
final range = start == end
? start.substring(0, 6)
: '${start.substring(0, 6)}..${end.substring(0, 6)}';
diff --git a/builder/lib/src/tryjob.dart b/builder/lib/src/tryjob.dart
index 682f182..15fefc9 100644
--- a/builder/lib/src/tryjob.dart
+++ b/builder/lib/src/tryjob.dart
@@ -27,15 +27,15 @@
bool get hasTooManyPassingChanges => passes > maxReportedSuccesses;
bool get hasTooManyFailingChanges => failures > maxReportedFailures;
- void count(Map<String, dynamic> change) {
+ void count(ChangeRecord change) {
++changes;
- change[fMatches] ? ++passes : ++failures;
- if (change[fFlaky] && !change[fPreviousFlaky]) ++newFlakes;
+ change.matches ? ++passes : ++failures;
+ if (change.flaky && !change.previousFlaky) ++newFlakes;
}
- bool isNotReported(Map<String, dynamic> change) {
- if (change[fMatches] && hasTooManyPassingChanges ||
- !change[fMatches] && hasTooManyFailingChanges) {
+ bool isNotReported(ChangeRecord change) {
+ if (change.matches && hasTooManyPassingChanges ||
+ !change.matches && hasTooManyFailingChanges) {
hasTruncatedChanges = true;
return true;
}
@@ -90,17 +90,17 @@
).update();
}
- bool isNotLandedResult(Map<String, dynamic> change) {
- return change[fResult] !=
- lastLandedResultByName[change[fName]]?.getString(fResult);
+ bool isNotLandedResult(ChangeRecord change) {
+ return change.result !=
+ lastLandedResultByName[change.name]?.getString(fResult);
}
- Future<BuildStatus> process(List<Map<String, dynamic>> results) async {
+ Future<BuildStatus> process(List<ChangeRecord> results) async {
await update();
log('storing ${results.length} change(s)');
- final resultsByConfiguration = groupBy<Map<String, dynamic>, String>(
+ final resultsByConfiguration = groupBy<ChangeRecord, String>(
results,
- (result) => result['configuration'],
+ (result) => result.configuration,
);
for (final configuration in resultsByConfiguration.keys) {
@@ -141,11 +141,11 @@
return status;
}
- Future<void> guardedStoreChange(Map<String, dynamic> change) =>
+ Future<void> guardedStoreChange(ChangeRecord change) =>
testNameLock.guardedCall(storeChange, change);
- Future<void> storeChange(Map<String, dynamic> change) async {
- transformChange(change);
+ Future<void> storeChange(ChangeRecord change) async {
+ change.transform();
counter.count(change);
if (counter.isNotReported(change)) return;
final approved = await firestore.storeTryChange(
@@ -153,7 +153,7 @@
info.review,
info.patchset,
);
- if (!approved && isFailure(change)) {
+ if (!approved && change.isFailure) {
counter.unapprovedFailures++;
success = false;
}
diff --git a/builder/test/approvals_test.dart b/builder/test/approvals_test.dart
index e008a75..2c6b518 100644
--- a/builder/test/approvals_test.dart
+++ b/builder/test/approvals_test.dart
@@ -56,9 +56,9 @@
final buildersToRemove = <String>{};
final testsToRemove = <String>{};
-void registerChangeForDeletion(Map<String, dynamic> change) {
- buildersToRemove.add(change['builder_name'] as String);
- testsToRemove.add(change['name'] as String);
+void registerChangeForDeletion(ChangeRecord change) {
+ buildersToRemove.add(change.builderName);
+ testsToRemove.add(change.name);
}
Future<void> removeBuildersAndResults() async {
@@ -161,10 +161,10 @@
Tryjob makeTryjob(
String name,
- Map<String, dynamic> firstChange, {
+ ChangeRecord firstChange, {
String? baseCommit,
}) => Tryjob(
- BuildInfo.fromResult(firstChange, <String>{firstChange[fConfiguration]})
+ BuildInfo.fromResult(firstChange, <String>{firstChange.configuration})
as TryBuildInfo,
'bbID_$name',
baseCommit ?? commit4,
@@ -174,7 +174,7 @@
);
const newFailure = 'Pass/RuntimeError/Pass';
-Map<String, dynamic> makeTryChange(
+ChangeRecord makeTryChange(
String name,
String result,
String patchsetRef, {
@@ -206,11 +206,12 @@
'bot_name': 'fake_bot_name',
'previous_build_number': '306',
};
- registerChangeForDeletion(change);
- return change;
+ final record = ChangeRecord.fromMap(change);
+ registerChangeForDeletion(record);
+ return record;
}
-Map<String, dynamic> makeChange(
+ChangeRecord makeChange(
String name,
String result,
String commit,
@@ -218,16 +219,16 @@
String? testName,
}) {
final change = {
- ...makeTryChange(name, result, '', testName: testName),
+ ...makeTryChange(name, result, '', testName: testName).toJson(),
'commit_hash': commit,
'previous_commit_hash': previousCommit,
};
- return change;
+ return ChangeRecord.fromMap(change);
}
-Build makeBuild(String commit, Map<String, dynamic> change) {
+Build makeBuild(String commit, ChangeRecord change) {
return Build(
- BuildInfo.fromResult(change, <String>{change[fConfiguration]}),
+ BuildInfo.fromResult(change, <String>{change.configuration}),
commitsCache,
firestore,
);
@@ -290,8 +291,10 @@
await firestore.approveResult(documents.single.toDocument());
final change3 = makeChange('approvals', newFailure, commit1, commit4);
- final change3a = makeChange('approvals', newFailure, commit1, commit4)
- ..['configuration'] = 'second_approvals_configuration';
+ final change3a = ChangeRecord.fromMap({
+ ...change3.toJson(),
+ 'configuration': 'second_approvals_configuration',
+ });
final change4 = makeChange(
'approvals',
newFailure,
@@ -303,7 +306,7 @@
commit1,
change3,
).process([change3, change3a, change4]);
- await checkBuild(change3['builder_name'], index1, success: true);
+ await checkBuild(change3.builderName, index1, success: true);
expect(status.success, isTrue);
expect(status.truncatedResults, isFalse);
await checkResult(change3, index3, index1, {'approved': true});
@@ -317,14 +320,14 @@
testName: 'approvals',
);
final status2 = await makeBuild(commit1, change5).process([change5]);
- await checkBuild(change5['builder_name'], index1, success: true);
+ await checkBuild(change5.builderName, index1, success: true);
expect(status2.success, isTrue);
await checkResult(change5, index2, index1, {
'approved': true,
'configurations': [
- change3['configuration'],
- change3a['configuration'],
- change5['configuration'],
+ change3.configuration,
+ change3a.configuration,
+ change5.configuration,
],
});
});
@@ -417,7 +420,7 @@
}
Future<void> checkResult(
- Map<String, dynamic> change,
+ ChangeRecord change,
String startIndex,
String endIndex,
Map<String, dynamic> expected,
@@ -431,7 +434,7 @@
expect(resultName, isNotNull);
final resultDocument = await firestore.getDocument(resultName!);
final data = untagMap(resultDocument.fields!);
- expect(data[fName], change[fName]);
+ expect(data[fName], change.name);
expect(data[fBlamelistStartIndex], int.parse(startIndex));
expect(data[fBlamelistEndIndex], int.parse(endIndex));
expect(
diff --git a/builder/test/builder_test.dart b/builder/test/builder_test.dart
index f9a9cbc..f670b6b 100644
--- a/builder/test/builder_test.dart
+++ b/builder/test/builder_test.dart
@@ -43,28 +43,33 @@
final buildersToRemove = <String>{};
final testsToRemove = <String>{};
-void registerChangeForDeletion(Map<String, dynamic> change) {
- buildersToRemove.add(change['builder_name']!);
- testsToRemove.add(change['name']!);
+void registerChangeForDeletion(ChangeRecord change) {
+ buildersToRemove.add(change.builderName);
+ testsToRemove.add(change.name);
}
Future<void> removeBuildersAndResults() async {
- Future<void> deleteDocuments(List<SafeDocument> documents) async {
+ Future<void> deleteDocuments(List<Document> documents) async {
for (final document in documents) {
- await firestore.deleteDocument(document.name);
+ await firestore.deleteDocument(document.name!);
}
}
for (final test in testsToRemove) {
await deleteDocuments(
- await firestore.query(from: 'results', where: fieldEquals(fName, test)),
+ await firestore.runQuery(
+ StructuredQuery()
+ ..from = inCollection('results')
+ ..where = fieldEquals(fName, test),
+ ),
);
}
for (final builder in buildersToRemove) {
await deleteDocuments(
- await firestore.query(
- from: 'builds',
- where: fieldEquals('builder', builder),
+ await firestore.runQuery(
+ StructuredQuery()
+ ..from = inCollection('builds')
+ ..where = fieldEquals('builder', builder),
),
);
}
@@ -84,23 +89,26 @@
);
}
-Build makeBuild(Map<String, dynamic> firstChange) => Build(
- BuildInfo.fromResult(firstChange, <String>{firstChange[fConfiguration]}),
+Build makeBuild(ChangeRecord firstChange) => Build(
+ BuildInfo.fromResult(firstChange, <String>{firstChange.configuration}),
commitsCache,
firestore,
);
-Map<String, dynamic> makeChange(
+ChangeRecord makeChange(
String name,
String result, {
bool flaky = false,
+ String? testName,
+ String? commitHash,
+ String? previousCommitHash,
}) {
final results = result.split('/');
final previous = results[0];
final current = results[1];
final expected = results[2];
final change = {
- fName: '${name}_test',
+ fName: testName ?? '${name}_test',
fConfiguration: '${name}_configuration',
'suite': 'unused_field',
'test_name': 'unused_field',
@@ -110,25 +118,34 @@
fExpected: expected,
fMatches: current == expected,
fChanged: current != previous,
- fCommitHash: commit.hash,
+ fCommitHash: commitHash ?? commit.hash,
'commit_time': 1583906489,
fBuildNumber: '99997',
fBuilderName: 'builder_$name',
fFlaky: flaky,
fPreviousFlaky: false,
- fPreviousCommitHash: previousCommit.hash,
+ fPreviousCommitHash: previousCommitHash ?? previousCommit.hash,
'previous_commit_time': 1583906489,
'bot_name': 'fake_bot_name',
'previous_build_number': '306',
};
- registerChangeForDeletion(change);
- return change;
+ final record = ChangeRecord.fromMap(change);
+ registerChangeForDeletion(record);
+ return record;
}
-Map<String, dynamic> makePreviousChange(String name, String result) {
- return makeChange(name, result)
- ..[fCommitHash] = previousBlamelistEndCommit.hash
- ..[fPreviousCommitHash] = previousBuildPreviousCommit.hash;
+ChangeRecord makePreviousChange(
+ String name,
+ String result, {
+ String? testName,
+}) {
+ return makeChange(
+ name,
+ result,
+ testName: testName,
+ commitHash: previousBlamelistEndCommit.hash,
+ previousCommitHash: previousBuildPreviousCommit.hash,
+ );
}
void main() async {
@@ -154,8 +171,8 @@
final failingPreviousChange = makePreviousChange(
'failure',
'Pass/RuntimeError/Pass',
- )..[fName] = 'previous_failure_test';
- registerChangeForDeletion(failingPreviousChange); // Name changed.
+ testName: 'previous_failure_test',
+ );
final previousBuild = makeBuild(failingPreviousChange);
final previousStatus = await previousBuild.process([failingPreviousChange]);
expect(previousStatus.success, isFalse);
@@ -170,21 +187,18 @@
expect(status.unapprovedFailures.keys, contains('failure_configuration'));
final failures = status.unapprovedFailures['failure_configuration']!;
final previousFailure = failures
- .where((failure) => failure.getString(fName) == 'previous_failure_test')
+ .where((failure) => failure.name == 'previous_failure_test')
.single;
final failure = failures
- .where((failure) => failure.getString(fName) == 'failure_test')
+ .where((failure) => failure.name == 'failure_test')
.single;
+ expect(previousFailure.blamelistEndCommit, previousBlamelistEndCommit.hash);
expect(
- previousFailure.getStringOrNull(fBlamelistEndCommit),
- previousBlamelistEndCommit.hash,
- );
- expect(
- previousFailure.getStringOrNull(fBlamelistStartCommit),
+ previousFailure.blamelistStartCommit,
previousBlamelistStartCommit.hash,
);
- expect(failure.getStringOrNull(fBlamelistEndCommit), commit.hash);
- expect(failure.getStringOrNull(fBlamelistStartCommit), commit.hash);
+ expect(failure.blamelistEndCommit, commit.hash);
+ expect(failure.blamelistStartCommit, commit.hash);
final message = status.toJson();
expect(message, matches(r'There are unapproved failures\\n'));
expect(
@@ -208,7 +222,7 @@
'RuntimeError/RuntimeError/Pass',
);
final unchangedBuild = makeBuild(unchangedChange);
- final unchangedStatus = await unchangedBuild.process([]);
+ final unchangedStatus = await unchangedBuild.process(<ChangeRecord>[]);
expect(unchangedStatus.success, isTrue);
expect(unchangedStatus.unapprovedFailures, isNotEmpty);
expect(
@@ -218,11 +232,12 @@
});
test('existing approved failure', () async {
- final failingOtherConfigurationChange =
- makeChange('other', 'Pass/RuntimeError/Pass')
- ..[fName] = 'approved_failure_test'
- ..[fPreviousCommitHash] = previousBuildPreviousCommit.hash;
- registerChangeForDeletion(failingOtherConfigurationChange);
+ final failingOtherConfigurationChange = makeChange(
+ 'other',
+ 'Pass/RuntimeError/Pass',
+ testName: 'approved_failure_test',
+ previousCommitHash: previousBuildPreviousCommit.hash,
+ );
final otherConfigurationBuild = makeBuild(failingOtherConfigurationChange);
final otherStatus = await otherConfigurationBuild.process([
failingOtherConfigurationChange,
@@ -237,9 +252,9 @@
'approved_failure_test',
'other_configuration',
)).single;
- expect(result.getInt(fBlamelistEndIndex), index);
- expect(result.getInt(fBlamelistStartIndex), previousBlamelistStart);
- await firestore.approveResult(result.toDocument());
+ expect(result.blamelistEndIndex, index);
+ expect(result.blamelistStartIndex, previousBlamelistStart);
+ await firestore.approveResult(result.doc);
final failingChange = makeChange(
'approved_failure',
'Pass/RuntimeError/Pass',
@@ -255,7 +270,7 @@
)).single;
expect(result.name, changedResult.name);
// Check blamelist narrowing.
- expect(changedResult.getInt(fBlamelistEndIndex), index);
- expect(changedResult.getInt(fBlamelistStartIndex), index);
+ expect(changedResult.blamelistEndIndex, index);
+ expect(changedResult.blamelistStartIndex, index);
});
}
diff --git a/builder/test/fakes.dart b/builder/test/fakes.dart
index c06d2fc..47a3630 100644
--- a/builder/test/fakes.dart
+++ b/builder/test/fakes.dart
@@ -18,12 +18,12 @@
final firestore = FirestoreServiceFake();
late CommitsCache commitsCache;
late Build builder;
- Map<String, dynamic> firstChange;
+ ChangeRecord firstChange;
BuilderTest(this.firstChange) {
commitsCache = CommitsCache(firestore, client);
builder = Build(
- BuildInfo.fromResult(firstChange, <String>{firstChange[fConfiguration]}),
+ BuildInfo.fromResult(firstChange, <String>{firstChange.configuration}),
commitsCache,
firestore,
);
@@ -38,8 +38,8 @@
// Test expectations
}
- Future<void> storeChange(Map<String, dynamic> change) async {
- return builder.storeChange(change);
+ Future<void> storeChange(ChangeRecord change) async {
+ await builder.storeChange(change);
}
}
@@ -89,7 +89,7 @@
@override
Future<String?> findResult(
- Map<String, dynamic> change,
+ ResultRecord change,
int startIndex,
int endIndex,
) {
@@ -97,10 +97,10 @@
int? resultEndIndex;
for (final entry in results.entries) {
final result = entry.value;
- if (result[fName] == change[fName] &&
- result[fResult] == change[fResult] &&
- result[fExpected] == change[fExpected] &&
- result[fPreviousResult] == change[fPreviousResult] &&
+ if (result[fName] == change.name &&
+ result[fResult] == change.result &&
+ result[fExpected] == change.expected &&
+ result[fPreviousResult] == change.previousResult &&
result[fBlamelistEndIndex] >= startIndex &&
result[fBlamelistStartIndex] <= endIndex) {
if (resultEndIndex == null ||
@@ -114,7 +114,7 @@
}
@override
- Future<List<SafeDocument>> findActiveResults(
+ Future<List<ResultRecord>> findActiveResults(
String? name,
String? configuration,
) async {
@@ -123,7 +123,7 @@
if (results[id]![fName] == name &&
results[id]![fActiveConfigurations] != null &&
results[id]![fActiveConfigurations].contains(configuration))
- SafeDocument(
+ ResultRecord(
Document()
..fields = taggedMap(Map.from(results[id]!))
..name = id,
@@ -132,19 +132,19 @@
}
@override
- Future<Document> storeResult(Map<String, dynamic> result) async {
+ Future<Document> storeResult(ResultRecord result) async {
final id = 'resultDocumentID$addedResultIdCounter';
addedResultIdCounter++;
- results[id] = result;
+ results[id] = result.toJson();
return Document()
- ..fields = taggedMap(result)
+ ..fields = result.doc.fields
..name = id;
}
@override
Future<bool> updateResult(
String resultId,
- String? configuration,
+ String configuration,
int startIndex,
int endIndex, {
required bool failure,
@@ -158,7 +158,7 @@
result[fBlamelistEndIndex] = min<int>(endIndex, result[fBlamelistEndIndex]);
if (!result[fConfigurations].contains(configuration)) {
- result[fConfigurations] = List<String?>.from(result[fConfigurations])
+ result[fConfigurations] = List<String>.from(result[fConfigurations])
..add(configuration)
..sort();
}
@@ -166,7 +166,7 @@
result[fActive] = true;
if (!result[fActiveConfigurations].contains(configuration)) {
result[fActiveConfigurations] =
- List<String?>.from(result[fActiveConfigurations])
+ List<String>.from(result[fActiveConfigurations])
..add(configuration)
..sort();
}
@@ -177,21 +177,21 @@
@override
Future<void> removeActiveConfiguration(
- SafeDocument activeResult,
+ ResultRecord activeResult,
String? configuration,
) async {
- final result = Map<String, dynamic>.from(results[activeResult.name]!);
+ final result = Map<String, dynamic>.from(results[activeResult.doc.name]!);
result[fActiveConfigurations] = List.from(result[fActiveConfigurations])
..remove(configuration);
if (result[fActiveConfigurations].isEmpty) {
result.remove(fActiveConfigurations);
result.remove(fActive);
}
- results[activeResult.name] = result;
+ results[activeResult.doc.name!] = result;
}
@override
- Future<List<Map<String, Value>>> findRevertedChanges(int index) async {
+ Future<List<ResultRecord>> findRevertedChanges(int index) async {
return results.values
.where(
(change) =>
@@ -199,7 +199,7 @@
(change[fBlamelistStartIndex] == index &&
change[fBlamelistEndIndex] == index),
)
- .map(taggedMap)
+ .map(ResultRecord.fromMap)
.toList();
}
diff --git a/builder/test/firestore_test.dart b/builder/test/firestore_test.dart
index 22d79b7..dd073b8 100644
--- a/builder/test/firestore_test.dart
+++ b/builder/test/firestore_test.dart
@@ -55,27 +55,29 @@
tearDown(() async {
// Delete database records created by the tests.
- var snapshot = await firestore.query(
- from: 'try_builds',
- where: fieldEquals('review', testReview),
+ var snapshot = await firestore.runQuery(
+ StructuredQuery()
+ ..from = inCollection('try_builds')
+ ..where = fieldEquals('review', testReview),
);
for (final doc in snapshot) {
- await firestore.deleteDocument(doc.name);
+ await firestore.deleteDocument(doc.name!);
}
- snapshot = await firestore.query(
- from: 'patchsets',
+ snapshot = await firestore.runQuery(
+ StructuredQuery()..from = inCollection('patchsets'),
parent: 'reviews/$testReview/',
);
for (final doc in snapshot) {
- await firestore.deleteDocument(doc.name);
+ await firestore.deleteDocument(doc.name!);
}
- snapshot = await firestore.query(
- from: 'results',
- where: fieldEquals('name', removeActiveConfigurationTestName),
+ snapshot = await firestore.runQuery(
+ StructuredQuery()
+ ..from = inCollection('results')
+ ..where = fieldEquals('name', removeActiveConfigurationTestName),
);
for (final doc in snapshot) {
- await firestore.deleteDocument(doc.name);
+ await firestore.deleteDocument(doc.name!);
}
await firestore.deleteDocument(testReviewDocument);
});
@@ -83,7 +85,9 @@
test('Remove active configuration', () async {
// Remove the two active configurations from createdResultDocument,
// checking that the document is updated correctly at each stage.
- final createdResultDocument = await firestore.storeResult(createdResult);
+ final createdResultDocument = await firestore.storeResult(
+ ResultRecord.fromMap(createdResult),
+ );
final name = removeActiveConfigurationTestName;
var foundActiveResults = await firestore.findActiveResults(
@@ -91,7 +95,7 @@
testConfiguration,
);
var activeResult = foundActiveResults.single;
- expect(createdResultDocument.name, activeResult.name);
+ expect(createdResultDocument.name, activeResult.doc.name);
await firestore.removeActiveConfiguration(
activeResult,
@@ -108,7 +112,7 @@
);
activeResult = foundActiveResults.single;
- expect(activeResult.fields, contains('active'));
+ expect(activeResult.doc.fields!, contains('active'));
await firestore.removeActiveConfiguration(
activeResult,
'configuration 2',
@@ -148,7 +152,7 @@
2,
3,
);
- final tryResult = {
+ final tryResult = ChangeRecord.fromMap({
'review': testReview,
'configuration': 'test_configuration',
'name': 'test_suite/test_name',
@@ -156,39 +160,47 @@
'result': 'RuntimeError',
'expected': 'Pass',
'previous_result': 'Pass',
- };
+ });
await firestore.storeTryChange(tryResult, testReview, 1);
- final tryResult2 = Map<String, dynamic>.from(tryResult);
- tryResult2['patchset'] = 2;
- tryResult2['name'] = 'test_suite/test_name_2';
+ final tryResult2 = ChangeRecord.fromMap({
+ ...tryResult.toJson(),
+ 'patchset': 2,
+ 'name': 'test_suite/test_name_2',
+ });
await firestore.storeTryChange(tryResult2, testReview, 2);
- tryResult['patchset'] = 3;
- tryResult['name'] = 'test_suite/test_name';
- tryResult['expected'] = 'CompileTimeError';
- await firestore.storeTryChange(tryResult, testReview, 3);
+ final tryResult3 = ChangeRecord.fromMap({
+ ...tryResult.toJson(),
+ 'patchset': 3,
+ 'name': 'test_suite/test_name',
+ 'expected': 'CompileTimeError',
+ });
+ await firestore.storeTryChange(tryResult3, testReview, 3);
// Set the results on patchsets 1 and 2 to approved.
- final snapshot = await firestore.query(
- from: 'try_results',
- where: compositeFilter([
- fieldEquals('approved', false),
- fieldEquals('review', testReview),
- fieldLessThanOrEqual('patchset', 2),
- ]),
+ final snapshot = await firestore.runQuery(
+ StructuredQuery()
+ ..from = inCollection('try_results')
+ ..where = compositeFilter([
+ fieldEquals('approved', false),
+ fieldEquals('review', testReview),
+ fieldLessThanOrEqual('patchset', 2),
+ ]),
);
for (final response in snapshot) {
- await firestore.approveResult(response.toDocument());
+ await firestore.approveResult(response);
//await firestore.updateDocument(response.document.name, {'approved': taggedValue(true)});
}
// Should return only the approved change on patchset 2,
// not the one on patchset 1 or the unapproved change on patchset 3.
final approvals = await firestore.tryApprovals(testReview);
- tryResult2['configurations'] = [tryResult2['configuration']];
- tryResult2['approved'] = true;
- tryResult2.remove('configuration');
+ final expectedApproval = {
+ ...tryResult2.toJson(),
+ 'configurations': [tryResult2.configuration],
+ 'approved': true,
+ }..remove('configuration');
expect(1, approvals.length);
final approval = untagMap(approvals.single.fields);
- expect(approval, tryResult2);
+ expect(approval, expectedApproval);
});
});
}
diff --git a/builder/test/results_test.dart b/builder/test/results_test.dart
index 6c385d3..9d9708e 100644
--- a/builder/test/results_test.dart
+++ b/builder/test/results_test.dart
@@ -11,19 +11,19 @@
void main() async {
test('Base builder test', () async {
- final builderTest = BuilderTest(landedCommitChange);
+ final builderTest = BuilderTest(ChangeRecord.fromMap(landedCommitChange));
await builderTest.update();
});
test('Get info for already saved commit', () async {
- final builderTest = BuilderTest(existingCommitChange);
+ final builderTest = BuilderTest(ChangeRecord.fromMap(existingCommitChange));
await builderTest.storeBuildCommitsInfo();
expect(builderTest.builder.endIndex, existingCommitIndex);
expect(builderTest.builder.startIndex, previousCommitIndex + 1);
});
test('Link landed commit to review', () async {
- final builderTest = BuilderTest(landedCommitChange);
+ final builderTest = BuilderTest(ChangeRecord.fromMap(landedCommitChange));
builderTest.firestore.commits.removeWhere(
(key, value) => value[fIndex] > existingCommitIndex,
);
@@ -33,8 +33,8 @@
expect(builderTest.builder.endIndex, landedCommitIndex);
expect(builderTest.builder.startIndex, existingCommitIndex + 1);
expect(builderTest.builder.tryApprovals, {
- testResult(review44445Result): 54,
- testResult(review77779Result): 53,
+ ResultRecord.fromMap(review44445Result).testResult: 54,
+ ResultRecord.fromMap(review77779Result).testResult: 53,
});
expect(
(await builderTest.firestore.getCommit(commit53Hash))!.toJson(),
@@ -47,9 +47,10 @@
});
test('update previous active result', () async {
- final builderTest = BuilderTest(landedCommitChange);
+ final landedRecord = ChangeRecord.fromMap(landedCommitChange);
+ final builderTest = BuilderTest(landedRecord);
await builderTest.storeBuildCommitsInfo();
- await builderTest.storeChange(landedCommitChange);
+ await builderTest.storeChange(landedRecord);
expect(builderTest.builder.success, true);
expect(
builderTest.firestore.results['activeResultID'],
@@ -57,9 +58,10 @@
..[fActiveConfigurations] = ['another configuration'],
);
- final changeAnotherConfiguration = Map<String, dynamic>.from(
- landedCommitChange,
- )..['configuration'] = 'another configuration';
+ final changeAnotherConfiguration = ChangeRecord.fromMap(
+ Map<String, dynamic>.from(landedCommitChange)
+ ..['configuration'] = 'another configuration',
+ );
await builderTest.storeChange(changeAnotherConfiguration);
expect(builderTest.builder.success, true);
expect(
@@ -72,28 +74,31 @@
expect(builderTest.builder.countChanges, 2);
expect(
builderTest.firestore.results[await builderTest.firestore.findResult(
- landedCommitChange,
+ landedRecord,
landedCommitIndex,
landedCommitIndex,
)],
landedResult,
);
final result = (await builderTest.firestore.findActiveResults(
- landedCommitChange['name'],
- landedCommitChange['configuration'],
+ landedRecord.name,
+ landedRecord.configuration,
)).single;
- expect(untagMap(result.fields), landedResult);
+ expect(untagMap(result.doc.fields!), landedResult);
});
test('mark active result flaky', () async {
- final builderTest = BuilderTest(landedCommitChange);
+ final landedRecord = ChangeRecord.fromMap(landedCommitChange);
+ final builderTest = BuilderTest(landedRecord);
await builderTest.storeBuildCommitsInfo();
- final flakyChange = Map<String, dynamic>.from(landedCommitChange)
- ..[fPreviousResult] = 'RuntimeError'
- ..[fFlaky] = true;
- expect(flakyChange[fResult], 'RuntimeError');
+ final flakyChange = ChangeRecord.fromMap(
+ Map<String, dynamic>.from(landedCommitChange)
+ ..[fPreviousResult] = 'RuntimeError'
+ ..[fFlaky] = true,
+ );
+ expect(flakyChange.result, 'RuntimeError');
await builderTest.storeChange(flakyChange);
- expect(flakyChange[fResult], 'flaky');
+ expect(flakyChange.result, 'flaky');
expect(builderTest.builder.success, true);
expect(
builderTest.firestore.results['activeResultID'],
diff --git a/builder/test/revert_test.dart b/builder/test/revert_test.dart
index d9480a7..fe56140 100644
--- a/builder/test/revert_test.dart
+++ b/builder/test/revert_test.dart
@@ -7,12 +7,15 @@
import 'package:test/test.dart';
import 'package:builder/src/result.dart';
+import 'package:builder/src/firestore_helpers.dart';
import 'fakes.dart';
import 'test_data.dart';
void main() async {
test('fetch commit that is a revert', () async {
- final builderTest = BuilderTest(revertUnchangedChange);
+ final builderTest = BuilderTest(
+ ChangeRecord.fromMap(revertUnchangedChange),
+ );
builderTest.firestore.commits[revertedCommitHash] = revertedCommit;
builderTest.client.addDefaultResponse(revertGitilesLog);
@@ -28,7 +31,9 @@
});
test('fetch commit that is a reland (as a reland)', () async {
- final builderTest = BuilderTest(relandUnchangedChange);
+ final builderTest = BuilderTest(
+ ChangeRecord.fromMap(relandUnchangedChange),
+ );
builderTest.firestore.commits[revertedCommitHash] = revertedCommit;
builderTest.client.addDefaultResponse(revertAndRelandGitilesLog);
await builderTest.storeBuildCommitsInfo();
@@ -53,7 +58,9 @@
});
test('fetch commit that is a reland (as a revert)', () async {
- final builderTest = RevertBuilderTest(relandUnchangedChange);
+ final builderTest = RevertBuilderTest(
+ ChangeRecord.fromMap(relandUnchangedChange),
+ );
builderTest.client.addDefaultResponse(relandGitilesLog);
await builderTest.storeBuildCommitsInfo();
expect(builderTest.builder.endIndex, relandCommit['index']);
@@ -67,9 +74,10 @@
});
test('Automatically approve expected failure on revert', () async {
- final builderTest = RevertBuilderTest(revertChange);
+ final record = ChangeRecord.fromMap(revertChange);
+ final builderTest = RevertBuilderTest(record);
await builderTest.update();
- await builderTest.storeChange(revertChange);
+ await builderTest.storeChange(record);
expect(
builderTest.firestore.results.values
.where((result) => result[fBlamelistEndIndex] == 55)
@@ -79,23 +87,29 @@
});
test('Revert in blamelist, doesn\'t match new failure', () async {
- final builderTest = RevertBuilderTest(commit56UnmatchingChange);
- await builderTest.update();
- await builderTest.storeChange(commit56UnmatchingChange);
- await builderTest.storeChange(commit56DifferentNameChange);
- await builderTest.storeChange(commit56Change);
+ final unmatchingRecord = ChangeRecord.fromMap(commit56UnmatchingChange);
+ final differentNameRecord = ChangeRecord.fromMap(
+ commit56DifferentNameChange,
+ );
+ final record = ChangeRecord.fromMap(commit56Change);
- Future<bool> findApproval(Map<String, dynamic> change) async {
+ final builderTest = RevertBuilderTest(unmatchingRecord);
+ await builderTest.update();
+ await builderTest.storeChange(unmatchingRecord);
+ await builderTest.storeChange(differentNameRecord);
+ await builderTest.storeChange(record);
+
+ Future<bool> findApproval(ChangeRecord change) async {
final result = await builderTest.firestore.findActiveResults(
- change[fName],
- change[fConfiguration],
+ change.name,
+ change.configuration,
);
- return result.single.getBool(fApproved)!;
+ return result.single.approved;
}
- expect(await findApproval(commit56UnmatchingChange), false);
- expect(await findApproval(commit56DifferentNameChange), false);
- expect(await findApproval(commit56Change), true);
+ expect(await findApproval(unmatchingRecord), false);
+ expect(await findApproval(differentNameRecord), false);
+ expect(await findApproval(record), true);
});
}
diff --git a/builder/test/tryjob_test.dart b/builder/test/tryjob_test.dart
index aa78de8..e6b5f50 100644
--- a/builder/test/tryjob_test.dart
+++ b/builder/test/tryjob_test.dart
@@ -34,9 +34,9 @@
final buildersToRemove = <String?>{};
final testsToRemove = <String?>{};
-void registerChangeForDeletion(Map<String, dynamic> change) {
- buildersToRemove.add(change['builder_name']);
- testsToRemove.add(change['name']);
+void registerChangeForDeletion(ChangeRecord change) {
+ testsToRemove.add(change.name);
+ buildersToRemove.add(change.builderName);
}
Future<void> removeTryBuildersAndResults() async {
@@ -121,8 +121,8 @@
};
}
-Tryjob makeTryjob(String name, Map<String, dynamic> firstChange) => Tryjob(
- BuildInfo.fromResult(firstChange, <String>{firstChange[fConfiguration]})
+Tryjob makeTryjob(String name, ChangeRecord firstChange) => Tryjob(
+ BuildInfo.fromResult(firstChange, <String>{firstChange.configuration})
as TryBuildInfo,
'bbID_$name',
data['landedCommit']!,
@@ -131,22 +131,17 @@
client,
);
-Tryjob makeLandedTryjob(String name, Map<String, dynamic> firstChange) =>
- Tryjob(
- BuildInfo.fromResult(firstChange, <String>{firstChange[fConfiguration]})
- as TryBuildInfo,
- 'bbID_$name',
- data['baseCommit']!,
- commitsCache,
- firestore,
- client,
- );
+Tryjob makeLandedTryjob(String name, ChangeRecord firstChange) => Tryjob(
+ BuildInfo.fromResult(firstChange, <String>{firstChange.configuration})
+ as TryBuildInfo,
+ 'bbID_$name',
+ data['baseCommit']!,
+ commitsCache,
+ firestore,
+ client,
+);
-Map<String, dynamic> makeChange(
- String name,
- String result, {
- bool flaky = false,
-}) {
+ChangeRecord makeChange(String name, String result, {bool flaky = false}) {
final results = result.split('/');
final previous = results[0];
final current = results[1];
@@ -173,12 +168,13 @@
'bot_name': 'fake_bot_name',
'previous_build_number': '306',
};
- registerChangeForDeletion(change);
- return change;
+ final record = ChangeRecord.fromMap(change);
+ registerChangeForDeletion(record);
+ return record;
}
-Map<String, dynamic> makeLandedChange(String name, String result) {
- return makeChange(name, result)..['commit_hash'] = data['landedPatchsetRef'];
+ChangeRecord makeLandedChange(String name, String result) {
+ return makeChange(name, result)..commitHash = data['landedPatchsetRef']!;
}
Future<void> checkTryBuild(
@@ -227,11 +223,11 @@
expect(failedStatus.success, isFalse);
expect(failedStatus.truncatedResults, isFalse);
// Add a second failing configuration for the test.
- final otherConfigurationChange = {
- ...failingChange,
+ final otherConfigurationChange = ChangeRecord.fromMap({
+ ...failingChange.toJson(),
'configuration': 'other_configuration',
'builder': 'other_builder',
- };
+ });
registerChangeForDeletion(otherConfigurationChange);
final otherTryjob = makeTryjob(
'other_configuration',
@@ -297,7 +293,7 @@
final passingChange = makeChange('truncatedPass', 'RuntimeError/Pass/Pass');
final tryjob = makeTryjob('truncatedPass', passingChange);
final failingChange = makeChange('truncatedPass', 'Pass/RuntimeError/Pass')
- ..['name'] = 'truncated_pass_2_test';
+ ..name = 'truncated_pass_2_test';
registerChangeForDeletion(failingChange);
tryjob.counter.passes = ChangeCounter.maxReportedSuccesses;
final truncatedStatus = await tryjob.process([
@@ -328,7 +324,10 @@
test('truncated', () async {
final failingChange = makeChange('truncated', 'Pass/RuntimeError/Pass');
final tryjob = makeTryjob('truncated', failingChange);
- final truncatedChange = {...failingChange, 'name': 'truncated_2_test'};
+ final truncatedChange = ChangeRecord.fromMap({
+ ...failingChange.toJson(),
+ 'name': 'truncated_2_test',
+ });
registerChangeForDeletion(truncatedChange);
tryjob.counter.failures = ChangeCounter.maxReportedFailures - 1;
final truncatedStatus = await tryjob.process([
