Using protobuf instead of ciyaml directly. (#2206)
diff --git a/app_dart/lib/src/foundation/utils.dart b/app_dart/lib/src/foundation/utils.dart
index 4183e09..40dcfd6 100644
--- a/app_dart/lib/src/foundation/utils.dart
+++ b/app_dart/lib/src/foundation/utils.dart
@@ -165,14 +165,21 @@
final List<String> noOwnerBuilders = <String>[];
final YamlMap? ciYaml = loadYaml(ciYamlContent) as YamlMap?;
final pb.SchedulerConfig unCheckedSchedulerConfig = pb.SchedulerConfig()..mergeFromProto3Json(ciYaml);
- final pb.SchedulerConfig schedulerConfig = CiYaml(
+ final CiYaml ciYamlFromProto = CiYaml(
slug: Config.flutterSlug,
branch: Config.defaultBranch(Config.flutterSlug),
config: unCheckedSchedulerConfig,
- ).config;
+ );
+
+ final pb.SchedulerConfig schedulerConfig = ciYamlFromProto.config;
+
for (pb.Target target in schedulerConfig.targets) {
final String builder = target.name;
- final String? owner = getTestOwnership(builder, getTypeForBuilder(builder, ciYaml!), testOwnersContent).owner;
+ final String? owner = getTestOwnership(
+ builder,
+ getTypeForBuilder(builder, ciYamlFromProto),
+ testOwnersContent,
+ ).owner;
print('$builder: $owner');
if (owner == null) {
noOwnerBuilders.add(builder);
diff --git a/app_dart/lib/src/model/ci_yaml/target.dart b/app_dart/lib/src/model/ci_yaml/target.dart
index 3a70770..316e898 100644
--- a/app_dart/lib/src/model/ci_yaml/target.dart
+++ b/app_dart/lib/src/model/ci_yaml/target.dart
@@ -89,6 +89,14 @@
return BatchPolicy();
}
+ /// Get the tags from the defined properties in the ci.
+ ///
+ /// Return an empty list if no tags are found.
+ List<String> get getTags {
+ Map<String, Object> properties = getProperties();
+ return (properties.containsKey('tags')) ? (properties['tags'] as List).map((e) => e as String).toList() : [];
+ }
+
/// Gets the assembled properties for this [pb.Target].
///
/// Target properties are prioritized in:
diff --git a/app_dart/lib/src/request_handlers/check_flaky_builders.dart b/app_dart/lib/src/request_handlers/check_flaky_builders.dart
index 5b40997..82e7a10 100644
--- a/app_dart/lib/src/request_handlers/check_flaky_builders.dart
+++ b/app_dart/lib/src/request_handlers/check_flaky_builders.dart
@@ -74,8 +74,9 @@
slug,
kTestOwnerPath,
);
+
for (final _BuilderInfo info in eligibleBuilders) {
- final BuilderType type = getTypeForBuilder(info.name, loadYaml(ciContent) as YamlMap);
+ final BuilderType type = getTypeForBuilder(info.name, ciYaml);
final TestOwnership testOwnership = getTestOwnership(info.name!, type, testOwnerContent);
final List<BuilderRecord> builderRecords =
await bigquery.listRecentBuildRecordsForBuilder(kBigQueryProjectId, builder: info.name, limit: kRecordNumber);
@@ -154,6 +155,8 @@
if (nameToExistingPRs.containsKey(builder)) {
continue;
}
+
+ //TODO (ricardoamador): Refactor this so we don't need to parse the entire yaml looking for commented issues, https://github.com/flutter/flutter/issues/113232
int builderLineNumber = lines.indexWhere((String line) => line.contains('name: $builder')) + 1;
while (builderLineNumber < lines.length && !lines[builderLineNumber].contains('name:')) {
if (lines[builderLineNumber].contains('$kCiYamlTargetIsFlakyKey:')) {
diff --git a/app_dart/lib/src/request_handlers/file_flaky_issue_and_pr.dart b/app_dart/lib/src/request_handlers/file_flaky_issue_and_pr.dart
index 65e727b..1decf11 100644
--- a/app_dart/lib/src/request_handlers/file_flaky_issue_and_pr.dart
+++ b/app_dart/lib/src/request_handlers/file_flaky_issue_and_pr.dart
@@ -57,7 +57,7 @@
if (statistic.flakyRate < _threshold) {
continue;
}
- final BuilderType type = getTypeForBuilder(statistic.name, ci!);
+ final BuilderType type = getTypeForBuilder(statistic.name, ciYaml);
await _fileIssueAndPR(
gitHub,
slug,
@@ -65,7 +65,7 @@
statistic: statistic,
existingIssue: nameToExistingIssue[statistic.name],
existingPullRequest: nameToExistingPR[statistic.name],
- isMarkedFlaky: _getIsMarkedFlaky(statistic.name, ci),
+ isMarkedFlaky: _getIsMarkedFlaky(statistic.name, ci!),
type: type,
ownership: getTestOwnership(statistic.name, type, testOwnerContent)),
);
diff --git a/app_dart/lib/src/request_handlers/flaky_handler_utils.dart b/app_dart/lib/src/request_handlers/flaky_handler_utils.dart
index 61ee372..0863779 100644
--- a/app_dart/lib/src/request_handlers/flaky_handler_utils.dart
+++ b/app_dart/lib/src/request_handlers/flaky_handler_utils.dart
@@ -5,8 +5,9 @@
import 'dart:convert';
import 'dart:core';
+import 'package:cocoon_service/ci_yaml.dart';
+import 'package:collection/collection.dart';
import 'package:github/github.dart';
-import 'package:yaml/yaml.dart';
import '../service/bigquery.dart';
import '../service/github_service.dart';
@@ -295,8 +296,8 @@
}
/// Looks up the owner of a builder in TESTOWNERS file.
-TestOwnership getTestOwnership(String builderName, BuilderType type, String testOwnersContent) {
- final String testName = _getTestNameFromBuilderName(builderName);
+TestOwnership getTestOwnership(String targetName, BuilderType type, String testOwnersContent) {
+ final String testName = _getTestNameFromTargetName(targetName);
String? owner;
Team? team;
switch (type) {
@@ -418,11 +419,12 @@
/// Gets the [BuilderType] of the builder by looking up the information in the
/// ci.yaml.
-BuilderType getTypeForBuilder(String? builderName, YamlMap ci) {
- final List<dynamic>? tags = _getTags(builderName, ci);
- if (tags == null) {
+BuilderType getTypeForBuilder(String? targetName, CiYaml ciYaml) {
+ final List<String>? tags = _getTags(targetName, ciYaml);
+ if (tags == null || tags.isEmpty) {
return BuilderType.unknown;
}
+
bool hasFrameworkTag = false;
bool hasHostOnlyTag = false;
// If tags contain 'shard', it must be a shard test.
@@ -430,7 +432,7 @@
// If tags contain 'firebaselab`, it must be a firebase tests.
// Otherwise, it is framework host only test if its tags contain both
// 'framework' and 'hostonly'.
- for (dynamic tag in tags) {
+ for (String tag in tags) {
if (tag == kCiYamlTargetTagsFirebaselab) {
return BuilderType.firebaselab;
} else if (tag == kCiYamlTargetTagsShard) {
@@ -446,21 +448,19 @@
return hasFrameworkTag && hasHostOnlyTag ? BuilderType.frameworkHostOnly : BuilderType.unknown;
}
-List<dynamic>? _getTags(String? builderName, YamlMap ci) {
- final YamlList targets = ci[kCiYamlTargetsKey] as YamlList;
- final YamlMap? target = targets.firstWhere(
- (dynamic element) => element[kCiYamlTargetNameKey] == builderName,
- orElse: () => null,
- ) as YamlMap?;
+List<String>? _getTags(String? targetName, CiYaml ciYaml) {
+ List<Target> allTargets = ciYaml.presubmitTargets;
+ allTargets.addAll(ciYaml.postsubmitTargets);
+ Target? target = allTargets.firstWhereOrNull((element) => element.value.name == targetName);
if (target == null) {
return null;
}
- return jsonDecode(target[kCiYamlPropertiesKey][kCiYamlTargetTagsKey] as String) as List<dynamic>?;
+ return target.getTags;
}
-String _getTestNameFromBuilderName(String builderName) {
+String _getTestNameFromTargetName(String targetName) {
// The builder names is in the format '<platform> <test name>'.
- final List<String> words = builderName.split(' ');
+ final List<String> words = targetName.split(' ');
return words.length < 2 ? words[0] : words[1];
}
diff --git a/app_dart/lib/src/request_handlers/update_existing_flaky_issues.dart b/app_dart/lib/src/request_handlers/update_existing_flaky_issues.dart
index 2c1fe42..a25caf0 100644
--- a/app_dart/lib/src/request_handlers/update_existing_flaky_issues.dart
+++ b/app_dart/lib/src/request_handlers/update_existing_flaky_issues.dart
@@ -28,26 +28,34 @@
const UpdateExistingFlakyIssue({
required super.config,
required super.authenticationProvider,
+ @visibleForTesting this.ciYaml,
});
static const String kThresholdKey = 'threshold';
static const int kFreshPeriodForOpenFlake = 7; // days
+ final CiYaml? ciYaml;
+
@override
Future<Body> get() async {
final RepositorySlug slug = Config.flutterSlug;
final GithubService gitHub = config.createGithubServiceWithToken(await config.githubOAuthToken);
final BigqueryService bigquery = await config.createBigQueryService();
- final YamlMap? ci = loadYaml(await gitHub.getFileContent(
- slug,
- kCiYamlPath,
- )) as YamlMap?;
- final pb.SchedulerConfig unCheckedSchedulerConfig = pb.SchedulerConfig()..mergeFromProto3Json(ci);
- final CiYaml ciYaml = CiYaml(
- slug: slug,
- branch: Config.defaultBranch(slug),
- config: unCheckedSchedulerConfig,
- );
+
+ CiYaml? localCiYaml = ciYaml;
+ if (localCiYaml == null) {
+ final YamlMap? ci = loadYaml(await gitHub.getFileContent(
+ slug,
+ kCiYamlPath,
+ )) as YamlMap?;
+ final pb.SchedulerConfig unCheckedSchedulerConfig = pb.SchedulerConfig()..mergeFromProto3Json(ci);
+ localCiYaml = CiYaml(
+ slug: slug,
+ branch: Config.defaultBranch(slug),
+ config: unCheckedSchedulerConfig,
+ );
+ }
+
final List<BuilderStatistic> prodBuilderStatisticList =
await bigquery.listBuilderStatistic(kBigQueryProjectId, bucket: 'prod');
final List<BuilderStatistic> stagingBuilderStatisticList =
@@ -56,7 +64,7 @@
await _updateExistingFlakyIssue(
gitHub,
slug,
- ciYaml,
+ localCiYaml,
prodBuilderStatisticList: prodBuilderStatisticList,
stagingBuilderStatisticList: stagingBuilderStatisticList,
nameToExistingIssue: nameToExistingIssue,
@@ -79,6 +87,7 @@
required Bucket bucket,
required BuilderStatistic statistic,
required Issue existingIssue,
+ required CiYaml ciYaml,
}) async {
if (DateTime.now().difference(existingIssue.createdAt!) < const Duration(days: kFreshPeriodForOpenFlake)) {
return;
@@ -88,17 +97,12 @@
await gitHub.createComment(slug, issueNumber: existingIssue.number, body: updateBuilder.issueUpdateComment);
await gitHub.replaceLabelsForIssue(slug, issueNumber: existingIssue.number, labels: updateBuilder.issueLabels);
if (existingIssue.assignee == null && !updateBuilder.isBelow) {
- final String ciContent = await gitHub.getFileContent(
- slug,
- kCiYamlPath,
- );
final String testOwnerContent = await gitHub.getFileContent(
slug,
kTestOwnerPath,
);
- final String? testOwner = getTestOwnership(
- statistic.name, getTypeForBuilder(statistic.name, loadYaml(ciContent) as YamlMap), testOwnerContent)
- .owner;
+ final String? testOwner =
+ getTestOwnership(statistic.name, getTypeForBuilder(statistic.name, ciYaml), testOwnerContent).owner;
if (testOwner != null) {
await gitHub.assignIssue(slug, issueNumber: existingIssue.number, assignee: testOwner);
}
@@ -133,8 +137,14 @@
builderFlakyMap.containsKey(statistic.name) &&
// ignore: iterable_contains_unrelated_type
!ignoreFlakyMap.containsKey(statistic.name)) {
- await _addCommentToExistingIssue(gitHub, slug,
- bucket: Bucket.prod, statistic: statistic, existingIssue: nameToExistingIssue[statistic.name]!);
+ await _addCommentToExistingIssue(
+ gitHub,
+ slug,
+ bucket: Bucket.prod,
+ statistic: statistic,
+ existingIssue: nameToExistingIssue[statistic.name]!,
+ ciYaml: ciYaml,
+ );
}
}
// For all staging builder stats, updates any existing flaky bug.
@@ -143,8 +153,14 @@
builderFlakyMap[statistic.name] == true &&
// ignore: iterable_contains_unrelated_type
!ignoreFlakyMap.containsKey(statistic.name)) {
- await _addCommentToExistingIssue(gitHub, slug,
- bucket: Bucket.staging, statistic: statistic, existingIssue: nameToExistingIssue[statistic.name]!);
+ await _addCommentToExistingIssue(
+ gitHub,
+ slug,
+ bucket: Bucket.staging,
+ statistic: statistic,
+ existingIssue: nameToExistingIssue[statistic.name]!,
+ ciYaml: ciYaml,
+ );
}
}
}
diff --git a/app_dart/test/model/ci_yaml/target_test.dart b/app_dart/test/model/ci_yaml/target_test.dart
index cd2b4ca..189385b 100644
--- a/app_dart/test/model/ci_yaml/target_test.dart
+++ b/app_dart/test/model/ci_yaml/target_test.dart
@@ -105,6 +105,34 @@
});
});
+ test('tags are parsed from within properties', () {
+ final Target target = generateTarget(
+ 1,
+ platform: 'Linux_build_test',
+ platformProperties: <String, String>{
+ // This should be overrided by the target specific property
+ 'android_sdk': 'abc',
+ },
+ properties: <String, String>{'xcode': '12abc', 'tags': '["devicelab", "android", "linux"]'},
+ );
+ expect(target.getTags, ['devicelab', 'android', 'linux']);
+ });
+
+ test('we do not blow up if tags are not present', () {
+ final Target target = generateTarget(
+ 1,
+ platform: 'Linux_build_test',
+ platformProperties: <String, String>{
+ // This should be overrided by the target specific property
+ 'android_sdk': 'abc',
+ },
+ properties: <String, String>{
+ 'xcode': '12abc',
+ },
+ );
+ expect(target.getTags, []);
+ });
+
test('platform properties with xcode', () {
final Target target = generateTarget(
1,
diff --git a/app_dart/test/request_handlers/update_existing_flaky_issues_test.dart b/app_dart/test/request_handlers/update_existing_flaky_issues_test.dart
index b2b27c7..d8267e7 100644
--- a/app_dart/test/request_handlers/update_existing_flaky_issues_test.dart
+++ b/app_dart/test/request_handlers/update_existing_flaky_issues_test.dart
@@ -56,14 +56,6 @@
return const Stream<Issue>.empty();
});
- // when gets the content of .ci.yaml
- when(mockRepositoriesService.getContents(
- captureAny,
- kCiYamlPath,
- )).thenAnswer((Invocation invocation) {
- return Future<RepositoryContents>.value(
- RepositoryContents(file: GitHubFile(content: gitHubEncode(ciYamlContent))));
- });
// when gets the content of TESTOWNERS
when(mockRepositoriesService.getContents(
captureAny,
@@ -87,6 +79,7 @@
handler = UpdateExistingFlakyIssue(
config: config,
authenticationProvider: auth,
+ ciYaml: testCiYaml,
);
});
@@ -326,6 +319,7 @@
)).thenAnswer((Invocation invocation) {
return Future<Response>.value(Response('[]', 200));
});
+
final Map<String, dynamic> result = await utf8.decoder
.bind((await tester.get<Body>(handler)).serialize() as Stream<List<int>>)
.transform(json.decoder)
diff --git a/app_dart/test/request_handlers/update_existing_flaky_issues_test_data.dart b/app_dart/test/request_handlers/update_existing_flaky_issues_test_data.dart
index f05638b..5b3c821 100644
--- a/app_dart/test/request_handlers/update_existing_flaky_issues_test_data.dart
+++ b/app_dart/test/request_handlers/update_existing_flaky_issues_test_data.dart
@@ -4,7 +4,11 @@
import 'dart:convert';
+import 'package:cocoon_service/src/model/ci_yaml/ci_yaml.dart';
import 'package:cocoon_service/src/service/bigquery.dart';
+import 'package:cocoon_service/src/service/config.dart';
+
+import 'package:cocoon_service/src/model/proto/protos.dart' as pb;
const String expectedSemanticsIntegrationTestIssueComment = '''
[prod pool] current flaky ratio for the past (up to) 100 commits is 50.00%. Flaky number: 3; total number: 10.
@@ -203,66 +207,82 @@
Please follow https://github.com/flutter/flutter/wiki/Reducing-Test-Flakiness#fixing-flaky-tests to fix the flakiness and enable the test back after validating the fix (internal dashboard to validate: go/flutter_test_flakiness).
''';
-const String ciYamlContent = '''
-# Describes the targets run in continuous integration environment.
-#
-# Flutter infra uses this file to generate a checklist of tasks to be performed
-# for every commit.
-#
-# More information at:
-# * https://github.com/flutter/cocoon/blob/master/scheduler/README.md
-enabled_branches:
- - master
-
-targets:
- - name: Mac_android android_semantics_integration_test
- builder: Mac_android android_semantics_integration_test
- presubmit: false
- scheduler: luci
- properties:
- tags: >
- ["devicelab"]
-
- - name: Mac_android ignore_myflakiness
- builder: Mac_android android_semantics_integration_test
- presubmit: false
- scheduler: luci
- properties:
- ignore_flakiness: "true"
- tags: >
- ["devicelab"]
-
- - name: Linux ci_yaml flutter roller
- recipe: infra/ci_yaml
- bringup: true # TODO(chillers): https://github.com/flutter/flutter/issues/93225
- timeout: 30
- properties:
- tags: >
- ["framework","hostonly","shard"]
- scheduler: luci
- runIf:
- - .ci.yaml
-
- - name: Mac build_tests_1_4
- recipe: flutter/flutter_drone
- timeout: 60
- properties:
- add_recipes_cq: "true"
- dependencies: >-
- [
- {"dependency": "android_sdk", "version": "version:29.0"},
- {"dependency": "chrome_and_driver", "version": "version:84"},
- {"dependency": "open_jdk", "version": "11"},
- {"dependency": "xcode", "version": "13a233"},
- {"dependency": "gems", "version": "v3.3.14"},
- {"dependency": "goldctl", "version": "git_revision:3a77d0b12c697a840ca0c7705208e8622dc94603"}
- ]
- shard: build_tests
- subshard: "1_4"
- tags: >
- ["framework","hostonly","shard"]
- scheduler: luci
-''';
+final CiYaml testCiYaml = CiYaml(
+ slug: Config.flutterSlug,
+ branch: Config.defaultBranch(Config.flutterSlug),
+ config: pb.SchedulerConfig(
+ enabledBranches: <String>[
+ Config.defaultBranch(Config.flutterSlug),
+ ],
+ targets: <pb.Target>[
+ pb.Target(
+ name: 'Mac_android android_semantics_integration_test',
+ scheduler: pb.SchedulerSystem.luci,
+ presubmit: false,
+ properties: <String, String>{
+ 'tags': jsonEncode(['devicelab'])
+ },
+ ),
+ pb.Target(
+ name: 'Mac_android ignore_myflakiness',
+ scheduler: pb.SchedulerSystem.luci,
+ presubmit: false,
+ properties: <String, String>{
+ 'ignore_flakiness': 'true',
+ 'tags': jsonEncode(['devicelab']),
+ }),
+ pb.Target(
+ name: 'Linux ci_yaml flutter roller',
+ scheduler: pb.SchedulerSystem.luci,
+ bringup: true,
+ timeout: 30,
+ runIf: ['.ci.yaml'],
+ recipe: 'infra/ci_yaml',
+ properties: <String, String>{
+ 'tags': jsonEncode(["framework", "hostonly", "shard"]),
+ },
+ ),
+ pb.Target(
+ name: 'Mac build_tests_1_4',
+ scheduler: pb.SchedulerSystem.luci,
+ recipe: 'flutter/flutter_drone',
+ timeout: 60,
+ properties: <String, String>{
+ 'add_recipes_cq': 'true',
+ 'shard': 'build_tests',
+ 'subshard': '1_4',
+ 'tags': jsonEncode(["framework", "hostonly", "shard"]),
+ 'dependencies': jsonEncode([
+ {
+ 'dependency': 'android_sdk',
+ 'version': 'version:29.0',
+ },
+ {
+ 'dependency': 'chrome_and_driver',
+ 'version': 'version:84',
+ },
+ {
+ 'dependency': 'xcode',
+ 'version': '13a233',
+ },
+ {
+ 'dependency': 'open_jdk',
+ 'version': '11',
+ },
+ {
+ 'dependency': 'gems',
+ 'version': 'v3.3.14',
+ },
+ {
+ 'dependency': 'goldctl',
+ 'version': 'git_revision:3a77d0b12c697a840ca0c7705208e8622dc94603',
+ },
+ ]),
+ },
+ ),
+ ],
+ ),
+);
const String testOwnersContent = '''
diff --git a/app_dart/test/src/utilities/entity_generators.dart b/app_dart/test/src/utilities/entity_generators.dart
index 786a1c7..197b210 100644
--- a/app_dart/test/src/utilities/entity_generators.dart
+++ b/app_dart/test/src/utilities/entity_generators.dart
@@ -2,6 +2,7 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
+import 'package:cocoon_service/ci_yaml.dart';
import 'package:cocoon_service/src/model/appengine/commit.dart';
import 'package:cocoon_service/src/model/appengine/task.dart';
import 'package:cocoon_service/src/model/ci_yaml/target.dart';
diff --git a/dashboard/pubspec.lock b/dashboard/pubspec.lock
index e23aa55..9dc64c6 100644
--- a/dashboard/pubspec.lock
+++ b/dashboard/pubspec.lock
@@ -323,7 +323,7 @@
name: material_color_utilities
url: "https://pub.dartlang.org"
source: hosted
- version: "0.1.5"
+ version: "0.2.0"
meta:
dependency: transitive
description:
@@ -447,7 +447,7 @@
name: source_span
url: "https://pub.dartlang.org"
source: hosted
- version: "1.9.0"
+ version: "1.9.1"
stack_trace:
dependency: transitive
description:
@@ -461,7 +461,7 @@
name: stream_channel
url: "https://pub.dartlang.org"
source: hosted
- version: "2.1.0"
+ version: "2.1.1"
stream_transform:
dependency: transitive
description:
@@ -489,7 +489,7 @@
name: test_api
url: "https://pub.dartlang.org"
source: hosted
- version: "0.4.12"
+ version: "0.4.14"
timing:
dependency: transitive
description:
@@ -573,7 +573,7 @@
name: vector_math
url: "https://pub.dartlang.org"
source: hosted
- version: "2.1.2"
+ version: "2.1.4"
watcher:
dependency: transitive
description:
@@ -596,5 +596,5 @@
source: hosted
version: "3.1.1"
sdks:
- dart: ">=2.17.0 <3.0.0"
+ dart: ">=2.18.0 <3.0.0"
flutter: ">=2.10.0"
