[cloud functions] Fix logical error in active_failure build status. Add test.
Change-Id: Id822b0d9fd97157f40c3480b05847d4cc85f7602
TBR: athom@google.com
Change-Id: Id822b0d9fd97157f40c3480b05847d4cc85f7602
Reviewed-on: https://dart-review.googlesource.com/c/dart_ci/+/129880
Reviewed-by: William Hesse <whesse@google.com>
diff --git a/functions/node/firestore.dart b/functions/node/firestore.dart
index 292ccc4..b054b9e 100644
--- a/functions/node/firestore.dart
+++ b/functions/node/firestore.dart
@@ -17,6 +17,8 @@
Future<void> addCommit(String id, Map<String, dynamic> data);
+ Future<List<String>> getConfigurations(String builder);
+
Future<void> updateConfiguration(String configuration, String builder);
Future<void> updateBuildInfo(String builder, int buildNumber, int index);
diff --git a/functions/node/firestore_impl.dart b/functions/node/firestore_impl.dart
index 70f352e..8fed82a 100644
--- a/functions/node/firestore_impl.dart
+++ b/functions/node/firestore_impl.dart
@@ -59,6 +59,14 @@
await docRef.setData(DocumentData.fromMap(data));
}
+ Future<List<String>> getConfigurations(String builder) async {
+ QuerySnapshot snapshot = await firestore
+ .collection('configurations')
+ .where('builder', isEqualTo: builder)
+ .get();
+ return [for (final document in snapshot.documents) document.documentID];
+ }
+
Future<void> updateConfiguration(String configuration, String builder) async {
final record =
await firestore.document('configurations/$configuration').get();
@@ -304,6 +312,25 @@
Future<void> storeChunkStatus(String builder, int index, bool success) async {
final document = firestore.document('builds/$builder:$index');
+ // Compute activeFailures outside transaction, because it runs queries.
+ // Because "completed" might be true inside transaction, but not now,
+ // we must compute activeFailures always, not just on last chunk.
+ var activeFailures = false;
+ final configurations = await getConfigurations(builder);
+ for (final configuration in configurations) {
+ // Find out if there are any unapproved unfixed failures,
+ // which we call "active" failures, to give sticky redness.
+ final snapshot = await firestore
+ .collection('results')
+ .where('active_configurations', arrayContains: configuration)
+ .where('approved', isEqualTo: false)
+ .limit(1)
+ .get();
+ if (snapshot.isNotEmpty) {
+ activeFailures = true;
+ break;
+ }
+ }
Future<void> updateStatus(Transaction transaction) async {
final snapshot = await transaction.get(document);
@@ -311,19 +338,6 @@
final int chunks = data['num_chunks'];
final int processedChunks = data['processed_chunks'] ?? 0;
final bool completed = chunks == processedChunks + 1;
- bool activeFailures = false;
- if (completed) {
- // Find out if there are any unapproved unfixed failures,
- // which we call "active" failures, to give sticky redness.
- final snapshot = await firestore
- .collection('results')
- .where('active_configurations', arrayContains: builder)
- .where('approved', isEqualTo: false)
- .limit(1)
- .get();
- activeFailures = snapshot.isNotEmpty;
- }
-
final update = UpdateData.fromMap({
'processed_chunks': processedChunks + 1,
'success': (data['success'] ?? true) && success,
diff --git a/functions/node/test/firestore_test_nodejs.dart b/functions/node/test/firestore_test_nodejs.dart
index a422287..1ff458b 100644
--- a/functions/node/test/firestore_test_nodejs.dart
+++ b/functions/node/test/firestore_test_nodejs.dart
@@ -3,6 +3,7 @@
// BSD-style license that can be found in the LICENSE file.
import 'package:firebase_admin_interop/firebase_admin_interop.dart';
+import 'package:firebase_functions_interop/firebase_functions_interop.dart';
import 'package:test/test.dart';
import '../firestore_impl.dart' as fs;
@@ -21,8 +22,15 @@
final firestore = fs.FirestoreServiceImpl();
test('Test chunk storing', () async {
- final builder = 'test_builder';
+ final builder = testBuilder;
+ final configuration = testConfiguration;
final index = 123;
+
+ await firestore.updateConfiguration(configuration, builder);
+ final failingResultReference = await fs.firestore
+ .collection('results')
+ .add(DocumentData.fromMap(activeFailureResult));
+
final document = fs.firestore.document('builds/$builder:$index');
await document.delete();
@@ -47,6 +55,10 @@
expect(data['num_chunks'], 4);
expect(data['processed_chunks'], 4);
expect(data['completed'], isTrue);
+ expect(data['active_failures'], isTrue);
+ await document.delete();
+ await failingResultReference.delete();
+ await fs.firestore.document('configurations/$testConfiguration').delete();
});
group('Try results', () {
diff --git a/functions/node/test/test_data.dart b/functions/node/test/test_data.dart
index eebb69f..9ab4e31 100644
--- a/functions/node/test/test_data.dart
+++ b/functions/node/test/test_data.dart
@@ -85,6 +85,20 @@
"changed": true
};
+const Map<String, dynamic> activeFailureResult = {
+ "name": "test_suite/active_failing_test",
+ "configurations": [testConfiguration, 'configuration 2', 'configuration 3'],
+ "active": true,
+ "active_configurations": [testConfiguration, 'configuration 2'],
+ "approved": false,
+ "suite": "test_suite",
+ "result": "RuntimeError",
+ "expected": "Pass",
+ "previous_result": "Pass",
+ "blamelist_start_index": 67195,
+ "blamelist_end_index": 67195
+};
+
const List<Map<String, dynamic>> tryjobResults = [
{
"review": 44445,
@@ -114,6 +128,7 @@
const testBuilder = 'test_builder';
const testBuildNumber = "308";
+const testConfiguration = 'test_configuration';
const testReview = 123;
const testPatchset = 3;
const testPreviousPatchset = 1;
