Address PR comments: remove MapValueExtensions, fix activeConfigurations, and refactor tests - Remove MapValueExtensions and access Value fields directly in data.dart. - Fix activeConfigurations getter to return empty list instead of null to allow proper cleanup. - Refactor test fakes and tests to use ChangeRecord directly. - Fix approvals_test.dart patchset variables to be int. - Simplify readChangedResults in upload_results_to_database.dart. - Simplify log message in builder.dart. 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 af85dfc..beddded 100644 --- a/builder/bin/upload_results_to_database.dart +++ b/builder/bin/upload_results_to_database.dart
@@ -20,20 +20,24 @@ print('Empty input results.json file'); exit(1); } + final firstRecord = ChangeRecord.fromMap( + jsonDecode(lines[0])! as Map<String, dynamic>, + ); final changes = <ChangeRecord>[]; - final configurations = <String>{}; - ChangeRecord? firstChange; - for (final line in lines) { + final configurations = <String>{firstRecord.configuration}; + if (firstRecord.isChangedResult) { + changes.add(firstRecord); + } + for (var i = 1; i < lines.length; i++) { final change = ChangeRecord.fromMap( - jsonDecode(line)! as Map<String, dynamic>, + jsonDecode(lines[i])! as Map<String, dynamic>, ); - firstChange ??= change; configurations.add(change.configuration); if (change.isChangedResult) { changes.add(change); } } - buildInfo = BuildInfo.fromResult(firstChange!, configurations); + buildInfo = BuildInfo.fromResult(firstRecord, configurations); return changes; }
diff --git a/builder/lib/src/builder.dart b/builder/lib/src/builder.dart index 57fbd11..5345dff 100644 --- a/builder/lib/src/builder.dart +++ b/builder/lib/src/builder.dart
@@ -211,8 +211,8 @@ false)) { log( 'Unexpected active result when processing new change:\n' - 'Active result: ${untagMap(activeResult.doc.fields!)}\n\n' - 'Change: ${change.toJson()}\n\n' + 'Active result: $activeResult\n\n' + 'Change: $change\n\n' 'approved: $approved', ); }
diff --git a/builder/lib/src/data.dart b/builder/lib/src/data.dart index 6b407d9..e8d92a9 100644 --- a/builder/lib/src/data.dart +++ b/builder/lib/src/data.dart
@@ -6,57 +6,39 @@ import 'firestore_helpers.dart'; import 'result.dart'; -extension MapValueExtensions on Map<String, Value> { - int? getInt(String key) { - final val = this[key]; - if (val == null || val.nullValue != null) return null; - return getValue(val) as int?; - } - - String? getString(String key) { - final val = this[key]; - if (val == null || val.nullValue != null) return null; - return getValue(val) as String?; - } - - bool? getBool(String key) { - final val = this[key]; - if (val == null || val.nullValue != null) return null; - return getValue(val) as bool?; - } - - List<dynamic>? getList(String key) { - final val = this[key]; - if (val == null || val.nullValue != null) return null; - return getValue(val) as List<dynamic>?; - } - - bool isNull(String key) { - return !containsKey(key) || this[key]!.nullValue == 'NULL_VALUE'; - } -} - extension type ResultRecord(Document doc) { ResultRecord.fromMap(Map<String, dynamic> data) : this(Document(fields: taggedMap(data))); - String get name => doc.fields!.getString(fName)!; + String get name => doc.fields![fName]!.stringValue!; set name(String value) => doc.fields![fName] = taggedValue(value); - String get result => doc.fields!.getString(fResult)!; - String get previousResult => doc.fields!.getString(fPreviousResult)!; - String get expected => doc.fields!.getString(fExpected)!; - int get blamelistStartIndex => doc.fields!.getInt(fBlamelistStartIndex)!; - int get blamelistEndIndex => doc.fields!.getInt(fBlamelistEndIndex)!; - bool get approved => doc.fields!.getBool(fApproved) ?? false; - bool get active => doc.fields!.getBool(fActive) ?? false; - List<String> get configurations => - doc.fields!.getList(fConfigurations)!.cast<String>(); - List<String>? get activeConfigurations => - doc.fields!.getList(fActiveConfigurations)?.cast<String>(); - int? get pinnedIndex => doc.fields!.getInt(fPinnedIndex); + 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!.getString(fBlamelistStartCommit); - String? get blamelistEndCommit => doc.fields!.getString(fBlamelistEndCommit); + doc.fields![fBlamelistStartCommit]?.stringValue; + String? get blamelistEndCommit => + doc.fields![fBlamelistEndCommit]?.stringValue; set blamelistStartCommit(String? value) => doc.fields![fBlamelistStartCommit] = taggedValue(value); @@ -73,30 +55,31 @@ ChangeRecord.fromMap(Map<String, dynamic> data) : this(Document(fields: taggedMap(data))); - String get configuration => doc.fields!.getString('configuration')!; - String get builderName => doc.fields!.getString(fBuilderName)!; - int get buildNumber => int.parse(doc.fields!.getString(fBuildNumber)!); - String get commitHash => doc.fields!.getString(fCommitHash)!; + 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!.getString(fPreviousCommitHash); + String? get previousCommitHash => + doc.fields![fPreviousCommitHash]?.stringValue; - bool get changed => doc.fields!.getBool(fChanged) ?? false; - bool get flaky => doc.fields!.getBool(fFlaky) ?? false; - bool get previousFlaky => doc.fields!.getBool(fPreviousFlaky) ?? false; - bool get matches => doc.fields!.getBool(fMatches) ?? false; + 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!.getString(fPreviousResult) == null) { + if (doc.fields![fPreviousResult]?.stringValue == null) { doc.fields![fPreviousResult] = taggedValue('new test'); } - if (doc.fields!.getBool(fPreviousFlaky) == true) { + if (doc.fields![fPreviousFlaky]?.booleanValue == true) { doc.fields![fPreviousResult] = taggedValue('flaky'); } - if (doc.fields!.getBool(fFlaky) == true) { + if (doc.fields![fFlaky]?.booleanValue == true) { doc.fields![fResult] = taggedValue('flaky'); doc.fields![fMatches] = taggedValue(false); } @@ -104,60 +87,68 @@ } extension type TryResultRecord(Document doc) { - String get name => doc.fields!.getString(fName)!; - String get result => doc.fields!.getString(fResult)!; - String get previousResult => doc.fields!.getString(fPreviousResult)!; - String get expected => doc.fields!.getString(fExpected)!; - int get review => doc.fields!.getInt(fReview)!; - int get patchset => doc.fields!.getInt('patchset')!; - bool get approved => doc.fields!.getBool(fApproved) ?? false; - List<String> get configurations => - doc.fields!.getList(fConfigurations)!.cast<String>(); + String get name => doc.fields![fName]!.stringValue!; + String get result => doc.fields![fResult]!.stringValue!; + String get previousResult => doc.fields![fPreviousResult]!.stringValue!; + String get expected => doc.fields![fExpected]!.stringValue!; + int get review => int.parse(doc.fields![fReview]!.integerValue!); + int get patchset => int.parse(doc.fields!['patchset']!.integerValue!); + bool get approved => doc.fields![fApproved]?.booleanValue ?? false; + List<String> get configurations => doc + .fields![fConfigurations]! + .arrayValue! + .values! + .map((v) => v.stringValue!) + .toList(); String get testResult => [name, result, previousResult, expected].join(' '); } extension type TryBuildRecord(Document doc) { - String get builder => doc.fields!.getString('builder')!; - int get buildNumber => doc.fields!.getInt('build_number')!; - String get buildbucketId => doc.fields!.getString('buildbucket_id')!; - int get review => doc.fields!.getInt(fReview)!; - int get patchset => doc.fields!.getInt('patchset')!; - bool get success => doc.fields!.getBool('success') ?? false; - bool get completed => doc.fields!.getBool('completed') ?? false; - bool get truncated => doc.fields!.getBool('truncated') ?? false; + String get builder => doc.fields!['builder']!.stringValue!; + int get buildNumber => int.parse(doc.fields!['build_number']!.integerValue!); + String get buildbucketId => doc.fields!['buildbucket_id']!.stringValue!; + int get review => int.parse(doc.fields![fReview]!.integerValue!); + int get patchset => int.parse(doc.fields!['patchset']!.integerValue!); + bool get success => doc.fields!['success']?.booleanValue ?? false; + bool get completed => doc.fields!['completed']?.booleanValue ?? false; + bool get truncated => doc.fields!['truncated']?.booleanValue ?? false; } extension type BuildRecord(Document doc) { - String get builder => doc.fields!.getString('builder')!; - int get buildNumber => doc.fields!.getInt('build_number')!; - int get index => doc.fields!.getInt('index')!; - bool get success => doc.fields!.getBool('success') ?? false; - bool get completed => doc.fields!.getBool('completed') ?? false; + String get builder => doc.fields!['builder']!.stringValue!; + int get buildNumber => int.parse(doc.fields!['build_number']!.integerValue!); + int get index => int.parse(doc.fields!['index']!.integerValue!); + bool get success => doc.fields!['success']?.booleanValue ?? false; + bool get completed => doc.fields!['completed']?.booleanValue ?? false; } extension type ReviewRecord(Document doc) { String get review => doc.name!.split('/').last; - String get subject => doc.fields!.getString('subject')!; - int? get landedIndex => doc.fields!.getInt('landed_index'); - String? get revertOf => doc.fields!.getString('revert_of'); + String get subject => doc.fields!['subject']!.stringValue!; + int? get landedIndex => + int.tryParse(doc.fields!['landed_index']?.integerValue ?? ''); + String? get revertOf => doc.fields!['revert_of']?.stringValue; } extension type PatchsetRecord(Document doc) { - int get number => doc.fields!.getInt('number')!; - int get patchsetGroup => doc.fields!.getInt('patchset_group')!; - String get kind => doc.fields!.getString('kind')!; - String? get description => doc.fields!.getString('description'); + int get number => int.parse(doc.fields!['number']!.integerValue!); + int get patchsetGroup => + int.parse(doc.fields!['patchset_group']!.integerValue!); + String get kind => doc.fields!['kind']!.stringValue!; + String? get description => doc.fields!['description']?.stringValue; } extension type CommentRecord(Document doc) { String get id => doc.name!.split('/').last; - String get author => doc.fields!.getString('author')!; - String get comment => doc.fields!.getString('comment')!; - int get review => doc.fields!.getInt(fReview)!; - int? get blamelistStartIndex => doc.fields!.getInt(fBlamelistStartIndex); - int? get blamelistEndIndex => doc.fields!.getInt(fBlamelistEndIndex); - bool get approved => doc.fields!.getBool(fApproved) ?? false; + String get author => doc.fields!['author']!.stringValue!; + String get comment => doc.fields!['comment']!.stringValue!; + int get review => int.parse(doc.fields![fReview]!.integerValue!); + int? get blamelistStartIndex => + int.tryParse(doc.fields![fBlamelistStartIndex]?.integerValue ?? ''); + int? get blamelistEndIndex => + int.tryParse(doc.fields![fBlamelistEndIndex]?.integerValue ?? ''); + bool get approved => doc.fields![fApproved]?.booleanValue ?? false; } extension type CommitRecord(Document doc) { @@ -169,15 +160,15 @@ ), ); - int get index => doc.fields!.getInt(fIndex)!; - String? get revertOf => doc.fields!.getString(fRevertOf); + int get index => int.parse(doc.fields![fIndex]!.integerValue!); + String? get revertOf => doc.fields![fRevertOf]?.stringValue; bool get isRevert => doc.fields!.containsKey(fRevertOf); - int? get review => doc.fields!.getInt(fReview); + int? get review => int.tryParse(doc.fields![fReview]?.integerValue ?? ''); String get hash => doc.name!.split('/').last; Map<String, Object?> toJson() => untagMap(doc.fields!); } extension type ConfigurationRecord(Document doc) { - String get builder => doc.fields!.getString('builder')!; + String get builder => doc.fields!['builder']!.stringValue!; }
diff --git a/builder/lib/src/firestore.dart b/builder/lib/src/firestore.dart index 6633edd..90e54bf 100644 --- a/builder/lib/src/firestore.dart +++ b/builder/lib/src/firestore.dart
@@ -390,8 +390,7 @@ where: fieldEquals('blamelist_end_index', index), ); for (final data in unpinnedResults) { - if (data.blamelistStartIndex == index && - data.doc.fields!.isNull('pinned_index')) { + if (data.blamelistStartIndex == index && data.pinnedIndex == null) { results.add(data); } }
diff --git a/builder/lib/src/firestore_helpers.dart b/builder/lib/src/firestore_helpers.dart index 582855e..46e8f2a 100644 --- a/builder/lib/src/firestore_helpers.dart +++ b/builder/lib/src/firestore_helpers.dart
@@ -38,7 +38,7 @@ } else if (value.booleanValue != null) { return value.booleanValue; } else if (value.arrayValue != null) { - return value.arrayValue!.values?.map(getValue).toList() ?? []; + return value.arrayValue!.values!.map(getValue).toList(); } else if (value.timestampValue != null) { return DateTime.parse(value.timestampValue!); } else if (value.nullValue != null) {
diff --git a/builder/test/approvals_test.dart b/builder/test/approvals_test.dart index dda9930..9b0921b 100644 --- a/builder/test/approvals_test.dart +++ b/builder/test/approvals_test.dart
@@ -35,17 +35,17 @@ late final String index1; // Index of the final commit in the test range late final String commit1; // Hash of that commit late final String review; // CL number of that commit's Gerrit review -late final String lastPatchset; // Final patchset in that review +late final int lastPatchset; // Final patchset in that review late final String lastPatchsetRef; // 'refs/changes/[review]/[patchset]' -late final String patchsetGroup; // First patchset in the final patchset group +late final int patchsetGroup; // First patchset in the final patchset group late final String patchsetGroupRef; -late final String earlyPatchset; // Patchset not in the final patchset group +late final int earlyPatchset; // Patchset not in the final patchset group late final String earlyPatchsetRef; // Earlier commit with a review late final String index2; late final String commit2; late final String review2; -late final String patchset2; +late final int patchset2; late final String patchset2Ref; // Commits before commit2 late final String index3; @@ -140,11 +140,11 @@ parent: 'reviews/$review', ); final patchsetRecord = PatchsetRecord(patchsets.last); - lastPatchset = patchsetRecord.number.toString(); + lastPatchset = patchsetRecord.number; lastPatchsetRef = 'refs/changes/$review/$lastPatchset'; - patchsetGroup = patchsetRecord.patchsetGroup.toString(); + patchsetGroup = patchsetRecord.patchsetGroup; patchsetGroupRef = 'refs/changes/$review/$patchsetGroup'; - earlyPatchset = '1'; + earlyPatchset = 1; earlyPatchsetRef = 'refs/changes/$review/$earlyPatchset'; final patchsets2 = await firestore.query( StructuredQuery() @@ -152,7 +152,7 @@ ..orderBy = [orderBy('number', true)], parent: 'reviews/$review2', ); - patchset2 = PatchsetRecord(patchsets2.last).number.toString(); + patchset2 = PatchsetRecord(patchsets2.last).number; patchset2Ref = 'refs/changes/$review/$patchset2'; // Get commit hashes for the landed reviews, and for a commit before them
diff --git a/builder/test/fakes.dart b/builder/test/fakes.dart index 2cb660f..78b7db6 100644 --- a/builder/test/fakes.dart +++ b/builder/test/fakes.dart
@@ -18,14 +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(ChangeRecord.fromMap(firstChange), <String>{ - firstChange[fConfiguration], - }), + BuildInfo.fromResult(firstChange, <String>{firstChange.configuration}), commitsCache, firestore, ); @@ -40,14 +38,8 @@ // Test expectations } - Future<void> storeChange(Map<String, dynamic> change) async { - final record = ChangeRecord.fromMap(change); - await builder.storeChange(record); - record.toJson().forEach((key, value) { - if (change[key] != value) { - change[key] = value; - } - }); + Future<void> storeChange(ChangeRecord change) async { + await builder.storeChange(change); } } @@ -200,20 +192,14 @@ @override Future<List<ResultRecord>> findRevertedChanges(int index) async { - return results.entries - .where((entry) { - final change = entry.value; - return change[fPinnedIndex] == index || + return results.values + .where( + (change) => + change[fPinnedIndex] == index || (change[fBlamelistStartIndex] == index && - change[fBlamelistEndIndex] == index); - }) - .map( - (entry) => ResultRecord( - Document() - ..fields = taggedMap(entry.value) - ..name = entry.key, - ), + change[fBlamelistEndIndex] == index), ) + .map(ResultRecord.fromMap) .toList(); }
diff --git a/builder/test/firestore_test.dart b/builder/test/firestore_test.dart index 8cd854b..7c88dfa 100644 --- a/builder/test/firestore_test.dart +++ b/builder/test/firestore_test.dart
@@ -152,7 +152,7 @@ 2, 3, ); - final tryResult = { + final tryResult = ChangeRecord.fromMap({ 'review': testReview, 'configuration': 'test_configuration', 'name': 'test_suite/test_name', @@ -160,28 +160,21 @@ 'result': 'RuntimeError', 'expected': 'Pass', 'previous_result': 'Pass', - }; - await firestore.storeTryChange( - ChangeRecord.fromMap(tryResult), - testReview, - 1, - ); - final tryResult2 = Map<String, dynamic>.from(tryResult); - tryResult2['patchset'] = 2; - tryResult2['name'] = 'test_suite/test_name_2'; - await firestore.storeTryChange( - ChangeRecord.fromMap(tryResult2), - testReview, - 2, - ); - tryResult['patchset'] = 3; - tryResult['name'] = 'test_suite/test_name'; - tryResult['expected'] = 'CompileTimeError'; - await firestore.storeTryChange( - ChangeRecord.fromMap(tryResult), - testReview, - 3, - ); + }); + await firestore.storeTryChange(tryResult, testReview, 1); + final tryResult2 = ChangeRecord.fromMap({ + ...tryResult.toJson(), + 'patchset': 2, + 'name': 'test_suite/test_name_2', + }); + await firestore.storeTryChange(tryResult2, testReview, 2); + 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( StructuredQuery() @@ -200,12 +193,14 @@ // 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.doc.fields!); - expect(approval, tryResult2); + expect(approval, expectedApproval); }); }); }
diff --git a/builder/test/results_test.dart b/builder/test/results_test.dart index ede231f..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, ); @@ -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( - ChangeRecord.fromMap(landedCommitChange), + landedRecord, landedCommitIndex, landedCommitIndex, )], landedResult, ); final result = (await builderTest.firestore.findActiveResults( - landedCommitChange['name'], - landedCommitChange['configuration'], + landedRecord.name, + landedRecord.configuration, )).single; 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'], @@ -104,7 +109,7 @@ expect(builderTest.builder.countChanges, 1); expect( builderTest.firestore.results[await builderTest.firestore.findResult( - ChangeRecord.fromMap(flakyChange), + flakyChange, landedCommitIndex, landedCommitIndex, )],
diff --git a/builder/test/revert_test.dart b/builder/test/revert_test.dart index abc6bdd..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.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); }); }