[builder] Fetch unapproved failures from all builder configurations
Change-Id: Ia36f93572362420fccedb33190cb7e679e8d051b
Bug: b/210111991
Reviewed-on: https://dart-review.googlesource.com/c/dart_ci/+/234640
Reviewed-by: Jonas Termansen <sortie@google.com>
Commit-Queue: William Hesse <whesse@google.com>
diff --git a/builder/bin/update_results_database.dart b/builder/bin/update_results_database.dart
index 4c78b3f..62565c5 100644
--- a/builder/bin/update_results_database.dart
+++ b/builder/bin/update_results_database.dart
@@ -21,7 +21,8 @@
print('Empty input results.json file');
exit(1);
}
- buildInfo = BuildInfo.fromResult(lines.first);
+ buildInfo = BuildInfo.fromResult(
+ lines.first, {for (final line in lines) line[fConfiguration]});
return lines.where(isChangedResult).toList();
}
diff --git a/builder/lib/src/builder.dart b/builder/lib/src/builder.dart
index 24812f8..0d110f6 100644
--- a/builder/lib/src/builder.dart
+++ b/builder/lib/src/builder.dart
@@ -52,9 +52,7 @@
// TODO(karlklose): add a flag to overwrite builder results.
exit(1);
}
- final configurations =
- changes.map((change) => change['configuration'] as String).toSet();
- await update(configurations);
+ await update(info.configurations);
log('storing ${changes.length} change(s)');
await Pool(30).forEach(changes, guardedStoreChange).drain();
log('complete builder record');
@@ -62,7 +60,7 @@
final status = BuildStatus()..success = success;
try {
status.unapprovedFailures = {
- for (final configuration in configurations)
+ for (final configuration in info.configurations)
configuration: await unapprovedFailuresForConfiguration(configuration)
}..removeWhere((key, value) => value.isEmpty);
} catch (e) {
diff --git a/builder/lib/src/result.dart b/builder/lib/src/result.dart
index 6eef883..8b65946 100644
--- a/builder/lib/src/result.dart
+++ b/builder/lib/src/result.dart
@@ -84,20 +84,23 @@
final int buildNumber;
final String commitRef;
final String? previousCommitHash;
+ final Set<String> configurations;
- BuildInfo(Map<String, dynamic> result)
+ BuildInfo(Map<String, dynamic> result, this.configurations)
: builderName = result[fBuilderName],
buildNumber = int.parse(result[fBuildNumber]),
commitRef = result[fCommitHash],
previousCommitHash = result[fPreviousCommitHash];
- factory BuildInfo.fromResult(Map<String, dynamic> result) {
+ factory BuildInfo.fromResult(
+ Map<String, dynamic> result, Set<String> configurations) {
final commitRef = result[fCommitHash];
final match = commitRefRegExp.matchAsPrefix(commitRef);
if (match == null) {
- return BuildInfo(result);
+ return BuildInfo(result, configurations);
} else {
- return TryBuildInfo(result, int.parse(match[1]!), int.parse(match[2]!));
+ return TryBuildInfo(
+ result, configurations, int.parse(match[1]!), int.parse(match[2]!));
}
}
}
@@ -106,7 +109,9 @@
final int review;
final int patchset;
- TryBuildInfo(result, this.review, this.patchset) : super(result);
+ TryBuildInfo(Map<String, dynamic> result, Set<String> configurations,
+ this.review, this.patchset)
+ : super(result, configurations);
}
class TestNameLock {
diff --git a/builder/test/approvals_test.dart b/builder/test/approvals_test.dart
index 9ac97e8..0989867 100644
--- a/builder/test/approvals_test.dart
+++ b/builder/test/approvals_test.dart
@@ -143,8 +143,14 @@
Tryjob makeTryjob(String name, Map<String, dynamic> firstChange,
{String? baseCommit}) =>
- Tryjob(BuildInfo.fromResult(firstChange) as TryBuildInfo, 'bbID_$name',
- baseCommit ?? commit4, commitsCache, firestore, client);
+ Tryjob(
+ BuildInfo.fromResult(firstChange, <String>{firstChange[fConfiguration]})
+ as TryBuildInfo,
+ 'bbID_$name',
+ baseCommit ?? commit4,
+ commitsCache,
+ firestore,
+ client);
const newFailure = 'Pass/RuntimeError/Pass';
Map<String, dynamic> makeTryChange(
@@ -192,7 +198,8 @@
}
Build makeBuild(String commit, Map<String, dynamic> change) {
- return Build(BuildInfo.fromResult(change), commitsCache, firestore);
+ return Build(BuildInfo.fromResult(change, <String>{change[fConfiguration]}),
+ commitsCache, firestore);
}
void main() async {
diff --git a/builder/test/builder_test.dart b/builder/test/builder_test.dart
index 428458e..800fc7f 100644
--- a/builder/test/builder_test.dart
+++ b/builder/test/builder_test.dart
@@ -72,8 +72,10 @@
await commitsCache.getCommitByIndex(previousBuildPreviousIndex);
}
-Build makeBuild(Map<String, dynamic> firstChange) =>
- Build(BuildInfo.fromResult(firstChange), commitsCache, firestore);
+Build makeBuild(Map<String, dynamic> firstChange) => Build(
+ BuildInfo.fromResult(firstChange, <String>{firstChange[fConfiguration]}),
+ commitsCache,
+ firestore);
Map<String, dynamic> makeChange(String name, String result,
{bool flaky = false}) {
@@ -120,7 +122,7 @@
baseClient: baseClient);
final api = FirestoreApi(client);
firestore = FirestoreService(api, client);
- if (!await firestore.isStaging()) {
+ if (!await firestore.isStaging) {
throw (TestFailure('Error: test is being run on production'));
}
commitsCache = CommitsCache(firestore, client);
@@ -138,6 +140,9 @@
registerChangeForDeletion(failingPreviousChange); // Name changed.
final previousBuild = makeBuild(failingPreviousChange);
final previousStatus = await previousBuild.process([failingPreviousChange]);
+ expect(previousStatus.success, isFalse);
+ expect(previousStatus.unapprovedFailures.values.first, hasLength(1));
+
final failingChange = makeChange('failure', 'Pass/RuntimeError/Pass');
final build = makeBuild(failingChange);
final status = await build.process([failingChange]);
@@ -158,7 +163,6 @@
previousBlamelistStartCommit.hash);
expect(failure.getStringOrNull(fBlamelistEndCommit), commit.hash);
expect(failure.getStringOrNull(fBlamelistStartCommit), commit.hash);
-
final message = status.toJson();
expect(message, matches(r"There are unapproved failures\\n"));
expect(
@@ -169,8 +173,18 @@
message,
matches(
r"failure_test \(Pass -> RuntimeError , expected Pass \) at 2368c2"));
- expect(previousStatus.success, isFalse);
- expect(previousStatus.unapprovedFailures.values.first, hasLength(1));
+
+ // Check a build with no changed results.
+ // Another build with the same blamelist and configuration is allowed,
+ // although it should not happen in practice. This build has no changes.
+ final unchangedChange =
+ makeChange('failure', 'RuntimeError/RuntimeError/Pass');
+ final unchangedBuild = makeBuild(unchangedChange);
+ final unchangedStatus = await unchangedBuild.process([]);
+ expect(unchangedStatus.success, isTrue);
+ expect(unchangedStatus.unapprovedFailures, isNotEmpty);
+ expect(unchangedStatus.unapprovedFailures['failure_configuration']!,
+ isNotEmpty);
});
test('existing approved failure', () async {
@@ -182,6 +196,10 @@
final otherConfigurationBuild = makeBuild(failingOtherConfigurationChange);
final otherStatus = await otherConfigurationBuild
.process([failingOtherConfigurationChange]);
+ expect(otherStatus.success, isFalse);
+ expect(otherStatus.unapprovedFailures, isNotEmpty);
+ expect(
+ otherStatus.unapprovedFailures.keys, contains('other_configuration'));
final result = (await firestore.findActiveResults(
'approved_failure_test', 'other_configuration'))
.single;
@@ -195,10 +213,6 @@
expect(status.success, isTrue);
expect(status.truncatedResults, isFalse);
expect(status.unapprovedFailures, isEmpty);
- expect(otherStatus.success, isFalse);
- expect(otherStatus.unapprovedFailures, isNotEmpty);
- expect(
- otherStatus.unapprovedFailures.keys, contains('other_configuration'));
final changedResult = (await firestore.findActiveResults(
'approved_failure_test', 'other_configuration'))
.single;
diff --git a/builder/test/fakes.dart b/builder/test/fakes.dart
index b0910fd..a1a37e1 100644
--- a/builder/test/fakes.dart
+++ b/builder/test/fakes.dart
@@ -22,7 +22,11 @@
BuilderTest(this.firstChange) {
commitsCache = CommitsCache(firestore, client);
- builder = Build(BuildInfo.fromResult(firstChange), commitsCache, firestore);
+ builder = Build(
+ BuildInfo.fromResult(
+ firstChange, <String>{firstChange[fConfiguration]}),
+ commitsCache,
+ firestore);
}
Future<void> update() async {
diff --git a/builder/test/tryjob_test.dart b/builder/test/tryjob_test.dart
index b54a932..4061a71 100644
--- a/builder/test/tryjob_test.dart
+++ b/builder/test/tryjob_test.dart
@@ -113,7 +113,8 @@
}
Tryjob makeTryjob(String name, Map<String, dynamic> firstChange) => Tryjob(
- BuildInfo.fromResult(firstChange) as TryBuildInfo,
+ BuildInfo.fromResult(firstChange, <String>{firstChange[fConfiguration]})
+ as TryBuildInfo,
'bbID_$name',
data['landedCommit']!,
commitsCache,
@@ -121,8 +122,14 @@
client);
Tryjob makeLandedTryjob(String name, Map<String, dynamic> firstChange) =>
- Tryjob(BuildInfo.fromResult(firstChange) as TryBuildInfo, 'bbID_$name',
- data['baseCommit']!, commitsCache, firestore, client);
+ Tryjob(
+ BuildInfo.fromResult(firstChange, <String>{firstChange[fConfiguration]})
+ as TryBuildInfo,
+ 'bbID_$name',
+ data['baseCommit']!,
+ commitsCache,
+ firestore,
+ client);
Map<String, dynamic> makeChange(String name, String result,
{bool flaky = false}) {
@@ -246,6 +253,16 @@
expect(tryjob.counter.unapprovedFailures, 0);
});
+ test('empty', () async {
+ final emptyChange = makeChange('empty', 'Pass/Pass/Pass');
+ final tryjob = makeTryjob('empty', emptyChange);
+ final status = await tryjob.process([]);
+ await checkTryBuild('empty', success: true);
+ expect(status.success, isTrue);
+ expect(tryjob.success, isTrue);
+ expect(tryjob.counter.unapprovedFailures, 0);
+ });
+
test('truncatedPass', () async {
final passingChange = makeChange('truncatedPass', 'RuntimeError/Pass/Pass');
final tryjob = makeTryjob('truncatedPass', passingChange);
