From 89fc4480ed97cfe0b2a6398fa7bb4186f9bb8edd Mon Sep 17 00:00:00 2001 From: keyonghan <54558023+keyonghan@users.noreply.github.com> Date: Thu, 22 Feb 2024 14:39:58 -0800 Subject: [PATCH 1/4] Update CI_YAML doc about properties/dimensions (#3507) Part of https://github.com/flutter/flutter/issues/143671 This PR lists/consolidates all supported properties in .ci.yaml. --- CI_YAML.md | 358 +++++++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 318 insertions(+), 40 deletions(-) diff --git a/CI_YAML.md b/CI_YAML.md index da6a0b298..8da88ff4b 100644 --- a/CI_YAML.md +++ b/CI_YAML.md @@ -52,6 +52,7 @@ targets: # properties: A map of string, string. Values are parsed to their closest data model. # postsubmit_properties: Properties that are only run on postsubmit. # timeout: Integer defining whole build execution time limit for all steps in minutes. +# dimensions: A list of testbed dimensions which the CI determines what testbed to assign a target to. # # Minimal example: # Linux analyze will run on all presubmit and in postsubmit. @@ -124,33 +125,144 @@ following are a list of keys that are reserved for special use. **Properties** is a Map and any special values must be JSON encoded (i.e. no trailing commas). Additionally, these strings must be compatible with YAML multiline strings -**$flutter/osx_sdk**: xcode configs including sdk and runtime. **Note**: support on legacy `xcode`/`runtime` -properties and `xcode` dependency has been deprecated. + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + +
+ Property Name + + Description + + Default Value + + Type + + Example +
add_recipes_cqWhether to add this target to flutter/recipes CQ. This ensures +changes to flutter/recipes pass on this target before landing. + "false"string bool -Example: ``` yaml -$flutter/osx_sdk : >- - { - "sdk_version": "14e222b", - "runtime_versions": - [ - "ios-16-4_14e222b", - "ios-16-2_14c18" - ] - } +add_recipes_cq: "true" ``` +
cache_nameThe name identifier of the second layer Engine source cache. This is maintained by flutter/infra + team via cache.py recipe + and is separate from LUCI side default caches. + N/Astring -**add_recipes_cq**: String boolean whether to add this target to flutter/recipes CQ. This ensures -changes to flutter/recipes pass on this target before landing. +``` yaml +cache_name: "builder" +``` +
cache_pathThe paths of Engine checkout source that will be auto saved to CAS for boosting source checkout when +caches no longer exist from the bots. + N/Alist + +``` yaml +cache_paths: >- + [ + "builder", + "git" + ] +``` +
clobberWhether to clean the Engine source code cache. + "false"string bool + +``` yaml +clobber: "true" +``` +
config_nameThe config name of the targets. It is used for `Engine V2 recipes`, +and is a one-on-one map to the config files located under `ci/builders`. This is +not needed for targets using none `Engine V2 recipes`. + N/Astring + +``` yaml +config_name: linux_benchmarks +``` +
contextsThe list of contexts that will guide recipes to add to the ExitStack. This will initialize and prepare the virtual device used for tests. Other supported contexts include: `osx_sdk`, `depot_tools_on_path`, etc. + N/Alist + +```yaml +contexts: >- + [ + "android_virtual_device" + ] +``` +
coresThe machine cores a target will be running against. A higher number of cores may be needed for extensive targets. +
+ Note: This property will be auto populated to CI builder dimensions, which CI uses to determine the + testbed to run this target. +		N/Astring int -**dependencies**: JSON list of objects with "dependency" and optionally "version". -The list of supported deps is in [flutter_deps recipe_module](https://cs.opensource.google/flutter/recipes/+/master:recipe_modules/flutter_deps/api.py). +``` yaml +cores: "8" +``` +
dependenciesJSON list of objects with "dependency" and optionally "version". +The list of supported deps is in flutter_deps recipe_module. Dependencies generate a corresponding swarming cache that can be used in the recipe code. The path of the cache will be the name of the dependency. +
+Versions can be located in CIPD + 		N/Alist -Versions can be located in [CIPD](https://chrome-infra-packages.appspot.com/) - -Example ``` yaml dependencies: >- [ @@ -160,44 +272,210 @@ dependencies: >- {"dependency": "goldctl"} ] ``` +
device_typeThe phone device type a target will be running against. For host only targets that do not need + a phone, a value of `none` should be used. +
+ Note: This property will be auto populated to CI builder dimensions, which CI uses to determine the + testbed to run this target. +		N/Astring -**tags**: JSON list of strings. These are currently only used in flutter/flutter to help -with TESTOWNERSHIP and test flakiness. +``` yaml +device_type: "msm8952" +``` +
drone_dimensionsA list of testbed dimensions which the CI determines what testbed to assign a subbuild drone of a target to. This + property will be auto populated to CI dimensions of a subbuild triggered from the orchestrator target. +N/Astring -Example -```yaml -tags: > - ["devicelab","hostonly"] +``` yaml +drone_dimensions: > + ["device_type=none", "os=Linux"] ``` +
$flutter/osx_sdkXcode configs including sdk and runtimeN/Amap -**test_timeout_secs** String determining seconds before timeout for an individual test step. -Note that this is the timeout for a single test step rather than the entire build execution -timeout. +``` yaml +$flutter/osx_sdk : >- + { + "sdk_version": "14e222b", + "runtime_versions": + [ + "ios-16-4_14e222b", + "ios-16-2_14c18" + ] + } +``` +
gclient_variablesThe gclient variables populated to recipes when checking out sources via gclient sync. + N/Amap -Example ``` yaml -test_timeout_secs: "2700" +gclient_variables: >- + { + "download_android_deps": "true" + } +``` +
ignore_cache_pathsThe paths of Engine checkout source that will be skipped when saved to CAS. Please reference to `cache_path`. + N/Alist + +``` yaml +ignore_cache_paths: >- + [ + "buibuilder/src/flutter/prebuilts/SDKs", + "builder/src/flutter/prebuilts/Library"lder" + ] ``` +
no_gomaWhether to use goma when building artifacts. + "false"string bool -**presubmit_max_attempts** The max attempts the target will be auto executed in presubmit. If it is +``` yaml +no_goma: "true" +``` +
osThe machine os a target will be running against, such as `Linux`, `Mac-13`, etc. +
+ Note: This property will be auto populated to CI builder dimensions, which CI uses to determine the + testbed to run this target. +		N/Astring + +``` yaml +os: Linux +``` +
presubmit_max_attemptsThe max attempts the target will be auto executed in presubmit. If it is not specified, the default value is `1` and it means no auto rerun will happen. If explicitly defined, it controls the max number of attempts. For example: `3` means it will be auto rescheduled two more times. - -Example + "1"string int + ``` yaml presubmit_max_attempts: "3" ``` +
release_buildWhether is required to run to release Engine. Will be triggered via + release_builder.py + "false"string bool + +``` yaml +release_build: "true" +``` +
shardThe shard name of the sharding target, used in the test.dart test runner. + N/Astring -**contexts** The list of contexts that will guide [recipes](https://flutter.googlesource.com/recipes/+/refs/heads/main/recipe_modules/flutter_deps/api.py#665) to add to [ExitStack](https://docs.python.org/3/library/contextlib.html#contextlib.ExitStack). +``` yaml +shard: web_tests +``` +
subshardsThe sub shards of the sharding target, used in the test.dart test runner. +If omitted with `shard` defined, it will run all unit tests in a single shard. + N/Alist + +``` yaml +subshards: >- + ["0", "1", "2", "3", "4", "5", "6", "7_last"] +``` +
tagsJSON list of strings. These are currently only used in flutter/flutter to help +with TESTOWNERSHIP and test flakiness. + N/Alist -Example ```yaml -contexts: >- - [ - "android_virtual_device" - ] +tags: > + ["devicelab","hostonly"] +``` +
test_timeout_secsString determining seconds before timeout for an individual test step. +Note that this is the timeout for a single test step rather than the entire build execution +timeout. + "1800"string int + +``` yaml +test_timeout_secs: "2700" ``` -This will initialize and prepare the virtual device used for tests. Other supported contexts include: `osx_sdk`, `depot_tools_on_path`, etc. +
### Updating targets From e75d18702c5c7c165a67ffe91e141afe3ef68e9d Mon Sep 17 00:00:00 2001 From: keyonghan <54558023+keyonghan@users.noreply.github.com> Date: Thu, 22 Feb 2024 17:15:23 -0800 Subject: [PATCH 2/4] Update task documents in Firestore when vacuuming stale tasks (#3511) Part of https://github.com/flutter/flutter/issues/142951 --- .ci.yaml | 1 + CI_YAML.md | 8 +++---- .../integration_test/data/cocoon_config.json | 2 +- .../scheduler/vacuum_stale_tasks.dart | 14 +++++++++++ .../scheduler/vacuum_stale_tasks_test.dart | 24 ++++++++++++++++++- 5 files changed, 43 insertions(+), 6 deletions(-) diff --git a/.ci.yaml b/.ci.yaml index ca2ee9fa2..2dfe0d32c 100644 --- a/.ci.yaml +++ b/.ci.yaml @@ -44,6 +44,7 @@ targets: - packages/** - test_utilities/** - tooling/** + - CI_YAML.md - name: Linux device_doctor recipe: cocoon/cipd diff --git a/CI_YAML.md b/CI_YAML.md index 8da88ff4b..6035add36 100644 --- a/CI_YAML.md +++ b/CI_YAML.md @@ -225,7 +225,7 @@ config_name: linux_benchmarks N/A list - + ```yaml contexts: >- [ @@ -397,7 +397,7 @@ it controls the max number of attempts. For example: `3` means it will be auto r "1" string int - + ``` yaml presubmit_max_attempts: "3" ``` @@ -405,7 +405,7 @@ presubmit_max_attempts: "3" release_build - Whether is required to run to release Engine. Will be triggered via + Whether is required to run to release Engine. Will be triggered via release_builder.py "false" @@ -469,7 +469,7 @@ timeout. "1800" string int - + ``` yaml test_timeout_secs: "2700" ``` diff --git a/app_dart/integration_test/data/cocoon_config.json b/app_dart/integration_test/data/cocoon_config.json index 81860112e..4d4d4787d 100644 --- a/app_dart/integration_test/data/cocoon_config.json +++ b/app_dart/integration_test/data/cocoon_config.json @@ -1 +1 @@ -{"targets":[{"name":"Linux Cocoon","properties":{"add_recipes_cq":"true"},"runIf":[".ci.yaml","analyze/**","app_dart/**","auto_submit/**","cipd_packages/**","cloud_build/**","dashboard/**","dev/**","licenses/**","packages/**","test_utilities/**","tooling/**"],"recipe":"cocoon/cocoon"},{"name":"Linux device_doctor","properties":{"script":"cipd_packages/device_doctor/tool/build.sh","cipd_name":"flutter/device_doctor/linux-amd64"},"runIf":["cipd_packages/device_doctor/**",".ci.yaml"],"recipe":"cocoon/cipd"},{"name":"Mac device_doctor","properties":{"script":"cipd_packages/device_doctor/tool/build.sh","cipd_name":"flutter/device_doctor/mac-amd64","device_type":"none"},"runIf":["cipd_packages/device_doctor/**",".ci.yaml"],"recipe":"cocoon/cipd"},{"name":"Mac_arm64 device_doctor","properties":{"script":"cipd_packages/device_doctor/tool/build.sh","cipd_name":"flutter/device_doctor/mac-arm64","device_type":"none"},"runIf":["cipd_packages/device_doctor/**",".ci.yaml"],"recipe":"cocoon/cipd"},{"name":"Windows device_doctor","properties":{"script":"cipd_packages\\device_doctor\\tool\\build.bat","cipd_name":"flutter/device_doctor/windows-amd64"},"runIf":["cipd_packages/device_doctor/**",".ci.yaml"],"recipe":"cocoon/cipd"},{"name":"Linux doxygen","properties":{"script":"cipd_packages/doxygen/tool/build.sh","cipd_name":"flutter/doxygen/linux-amd64","dependencies":"[\n {\"dependency\": \"cmake\", \"version\": \"build_id:8787856497187628321\"}\n]"},"runIf":["cipd_packages/doxygen/**",".ci.yaml"],"recipe":"cocoon/cipd"},{"name":"Mac codesign","properties":{"script":"cipd_packages/codesign/tool/build.sh","cipd_name":"flutter/codesign/mac-amd64","device_type":"none"},"runIf":["cipd_packages/codesign/**",".ci.yaml"],"recipe":"cocoon/cipd"},{"name":"Mac_arm64 codesign","properties":{"script":"cipd_packages/codesign/tool/build.sh","cipd_name":"flutter/codesign/mac-arm64","device_type":"none"},"runIf":["cipd_packages/codesign/**",".ci.yaml"],"recipe":"cocoon/cipd"},{"name":"Mac ruby","timeout":60,"properties":{"script":"cipd_packages/ruby/tools/build.sh","cipd_name":"flutter/ruby/mac-amd64","device_os":"iOS","contexts":"[\n \"osx_sdk_devicelab\"\n]","$flutter/osx_sdk":"{\n \"sdk_version\": \"14e300c\"\n}"},"runIf":["cipd_packages/ruby/**",".ci.yaml"],"recipe":"cocoon/cipd"},{"name":"Mac_arm64 ruby","timeout":60,"properties":{"script":"cipd_packages/ruby/tools/build.sh","cipd_name":"flutter/ruby/mac-arm64","device_os":"iOS","contexts":"[\n \"osx_sdk_devicelab\"\n]","$flutter/osx_sdk":"{\n \"sdk_version\": \"14e300c\"\n}"},"runIf":["cipd_packages/ruby/**",".ci.yaml"],"recipe":"cocoon/cipd"},{"name":"Linux ci_yaml roller","properties":{"backfill":"false"},"runIf":[".ci.yaml"],"recipe":"infra/ci_yaml"}],"enabledBranches":["main"],"platformProperties":{"linux":{"properties":{"os":"Linux","device_type":"none"}},"mac":{"properties":{"os":"Mac-12|Mac-13","cpu":"x86"}},"mac_arm64":{"properties":{"os":"Mac-12|Mac-13","cpu":"arm64"}},"windows":{"properties":{"os":"Windows","device_type":"none"}}}} +{"targets":[{"name":"Linux Cocoon","properties":{"add_recipes_cq":"true"},"runIf":[".ci.yaml","analyze/**","app_dart/**","auto_submit/**","cipd_packages/**","cloud_build/**","dashboard/**","dev/**","licenses/**","packages/**","test_utilities/**","tooling/**","CI_YAML.md"],"recipe":"cocoon/cocoon"},{"name":"Linux device_doctor","properties":{"script":"cipd_packages/device_doctor/tool/build.sh","cipd_name":"flutter/device_doctor/linux-amd64"},"runIf":["cipd_packages/device_doctor/**",".ci.yaml"],"recipe":"cocoon/cipd"},{"name":"Mac device_doctor","properties":{"script":"cipd_packages/device_doctor/tool/build.sh","cipd_name":"flutter/device_doctor/mac-amd64","device_type":"none"},"runIf":["cipd_packages/device_doctor/**",".ci.yaml"],"recipe":"cocoon/cipd"},{"name":"Mac_arm64 device_doctor","properties":{"script":"cipd_packages/device_doctor/tool/build.sh","cipd_name":"flutter/device_doctor/mac-arm64","device_type":"none"},"runIf":["cipd_packages/device_doctor/**",".ci.yaml"],"recipe":"cocoon/cipd"},{"name":"Windows device_doctor","properties":{"script":"cipd_packages\\device_doctor\\tool\\build.bat","cipd_name":"flutter/device_doctor/windows-amd64"},"runIf":["cipd_packages/device_doctor/**",".ci.yaml"],"recipe":"cocoon/cipd"},{"name":"Linux doxygen","properties":{"script":"cipd_packages/doxygen/tool/build.sh","cipd_name":"flutter/doxygen/linux-amd64","dependencies":"[\n {\"dependency\": \"cmake\", \"version\": \"build_id:8787856497187628321\"}\n]"},"runIf":["cipd_packages/doxygen/**",".ci.yaml"],"recipe":"cocoon/cipd"},{"name":"Mac codesign","properties":{"script":"cipd_packages/codesign/tool/build.sh","cipd_name":"flutter/codesign/mac-amd64","device_type":"none"},"runIf":["cipd_packages/codesign/**",".ci.yaml"],"recipe":"cocoon/cipd"},{"name":"Mac_arm64 codesign","properties":{"script":"cipd_packages/codesign/tool/build.sh","cipd_name":"flutter/codesign/mac-arm64","device_type":"none"},"runIf":["cipd_packages/codesign/**",".ci.yaml"],"recipe":"cocoon/cipd"},{"name":"Mac ruby","timeout":60,"properties":{"script":"cipd_packages/ruby/tools/build.sh","cipd_name":"flutter/ruby/mac-amd64","device_os":"iOS","contexts":"[\n \"osx_sdk_devicelab\"\n]","$flutter/osx_sdk":"{\n \"sdk_version\": \"14e300c\"\n}"},"runIf":["cipd_packages/ruby/**",".ci.yaml"],"recipe":"cocoon/cipd"},{"name":"Mac_arm64 ruby","timeout":60,"properties":{"script":"cipd_packages/ruby/tools/build.sh","cipd_name":"flutter/ruby/mac-arm64","device_os":"iOS","contexts":"[\n \"osx_sdk_devicelab\"\n]","$flutter/osx_sdk":"{\n \"sdk_version\": \"14e300c\"\n}"},"runIf":["cipd_packages/ruby/**",".ci.yaml"],"recipe":"cocoon/cipd"},{"name":"Linux ci_yaml roller","properties":{"backfill":"false"},"runIf":[".ci.yaml"],"recipe":"infra/ci_yaml"}],"enabledBranches":["main"],"platformProperties":{"linux":{"properties":{"os":"Linux","device_type":"none"}},"mac":{"properties":{"os":"Mac-12|Mac-13","cpu":"x86"}},"mac_arm64":{"properties":{"os":"Mac-12|Mac-13","cpu":"arm64"}},"windows":{"properties":{"os":"Windows","device_type":"none"}}}} diff --git a/app_dart/lib/src/request_handlers/scheduler/vacuum_stale_tasks.dart b/app_dart/lib/src/request_handlers/scheduler/vacuum_stale_tasks.dart index 8a8e066c8..b27a40d41 100644 --- a/app_dart/lib/src/request_handlers/scheduler/vacuum_stale_tasks.dart +++ b/app_dart/lib/src/request_handlers/scheduler/vacuum_stale_tasks.dart @@ -6,9 +6,11 @@ import 'dart:async'; import 'package:cocoon_service/cocoon_service.dart'; import 'package:github/github.dart' as gh; +import 'package:googleapis/firestore/v1.dart'; import 'package:meta/meta.dart'; import '../../model/appengine/task.dart'; +import '../../model/firestore/task.dart' as firestore; import '../../service/datastore.dart'; import '../../service/logging.dart'; @@ -64,5 +66,17 @@ class VacuumStaleTasks extends RequestHandler { } log.info('Vacuuming stale tasks: $tasksToBeReset'); await datastore.insert(tasksToBeReset); + + await updateTaskDocuments(tasksToBeReset); + } + + Future updateTaskDocuments(List tasks) async { + if (tasks.isEmpty) { + return; + } + final List taskDocuments = tasks.map((e) => taskToTaskDocument(e)).toList(); + final List writes = documentsToWrites(taskDocuments, exists: true); + final FirestoreService firestoreService = await config.createFirestoreService(); + await firestoreService.writeViaTransaction(writes); } } diff --git a/app_dart/test/request_handlers/scheduler/vacuum_stale_tasks_test.dart b/app_dart/test/request_handlers/scheduler/vacuum_stale_tasks_test.dart index 93f40fb3b..592a3d5e8 100644 --- a/app_dart/test/request_handlers/scheduler/vacuum_stale_tasks_test.dart +++ b/app_dart/test/request_handlers/scheduler/vacuum_stale_tasks_test.dart @@ -5,24 +5,30 @@ import 'package:cocoon_service/cocoon_service.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/firestore/task.dart' as firestore; import 'package:cocoon_service/src/service/datastore.dart'; import 'package:gcloud/db.dart'; +import 'package:googleapis/firestore/v1.dart'; +import 'package:mockito/mockito.dart'; import 'package:test/test.dart'; import '../../src/datastore/fake_config.dart'; import '../../src/request_handling/request_handler_tester.dart'; import '../../src/utilities/entity_generators.dart'; +import '../../src/utilities/mocks.dart'; void main() { group(VacuumStaleTasks, () { late FakeConfig config; late RequestHandlerTester tester; late VacuumStaleTasks handler; + late MockFirestoreService mockFirestoreService; final Commit commit = generateCommit(1); setUp(() { - config = FakeConfig(); + mockFirestoreService = MockFirestoreService(); + config = FakeConfig(firestoreService: mockFirestoreService); config.db.values[commit.key] = commit; tester = RequestHandlerTester(); @@ -50,6 +56,13 @@ void main() { }); test('resets stale task', () async { + when( + mockFirestoreService.writeViaTransaction( + captureAny, + ), + ).thenAnswer((Invocation invocation) { + return Future.value(CommitResponse()); + }); final List originalTasks = [ generateTask( 1, @@ -78,6 +91,15 @@ void main() { expect(tasks[0].status, Task.statusNew); expect(tasks[2].createTimestamp, 0); expect(tasks[2].status, Task.statusNew); + + final List captured = verify(mockFirestoreService.writeViaTransaction(captureAny)).captured; + expect(captured.length, 1); + final List commitResponse = captured[0] as List; + expect(commitResponse.length, 2); + final firestore.Task taskDocuemnt1 = firestore.Task.fromDocument(taskDocument: commitResponse[0].update!); + final firestore.Task taskDocuemnt2 = firestore.Task.fromDocument(taskDocument: commitResponse[0].update!); + expect(taskDocuemnt1.status, firestore.Task.statusNew); + expect(taskDocuemnt2.status, firestore.Task.statusNew); }); }); } From 16b1937fa5f95cdb167751dbae3b0110e2c2c608 Mon Sep 17 00:00:00 2001 From: keyonghan <54558023+keyonghan@users.noreply.github.com> Date: Fri, 23 Feb 2024 09:55:13 -0800 Subject: [PATCH 3/4] Create firestore commit model and reference it everywhere (#3512) Help with https://github.com/flutter/flutter/issues/142951 --- app_dart/lib/src/model/firestore/commit.dart | 78 +++++++++++++++++++ app_dart/lib/src/model/firestore/task.dart | 2 +- app_dart/lib/src/service/firestore.dart | 58 +++++++------- app_dart/lib/src/service/scheduler.dart | 6 +- .../test/model/firestore/commit_test.dart | 39 ++++++++++ app_dart/test/service/firestore_test.dart | 10 ++- .../test/src/utilities/entity_generators.dart | 46 ++++++++--- 7 files changed, 195 insertions(+), 44 deletions(-) create mode 100644 app_dart/lib/src/model/firestore/commit.dart create mode 100644 app_dart/test/model/firestore/commit_test.dart diff --git a/app_dart/lib/src/model/firestore/commit.dart b/app_dart/lib/src/model/firestore/commit.dart new file mode 100644 index 000000000..dbabb7f44 --- /dev/null +++ b/app_dart/lib/src/model/firestore/commit.dart @@ -0,0 +1,78 @@ +// Copyright 2019 The Flutter Authors. 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:cocoon_service/cocoon_service.dart'; +import 'package:github/github.dart'; +import 'package:googleapis/firestore/v1.dart' hide Status; + +import '../../service/firestore.dart'; + +class Commit extends Document { + /// Lookup [Task] from Firestore. + /// + /// `documentName` follows `/projects/{project}/databases/{database}/documents/{document_path}` + static Future fromFirestore({ + required FirestoreService firestoreService, + required String documentName, + }) async { + final Document document = await firestoreService.getDocument(documentName); + return Commit.fromDocument(commitDocument: document); + } + + /// Create [Commit] from a Commit Document. + static Commit fromDocument({ + required Document commitDocument, + }) { + return Commit() + ..fields = commitDocument.fields! + ..name = commitDocument.name!; + } + + /// The timestamp (in milliseconds since the Epoch) of when the commit + /// landed. + int? get createTimestamp => int.parse(fields![kCommitCreateTimestampField]!.integerValue!); + + /// The SHA1 hash of the commit. + String? get sha => fields![kCommitShaField]!.stringValue!; + + /// The GitHub username of the commit author. + String? get author => fields![kCommitAuthorField]!.stringValue!; + + /// URL of the [author]'s profile image / avatar. + /// + /// The bytes loaded from the URL are expected to be encoded image bytes. + String? get avatar => fields![kCommitAvatarField]!.stringValue!; + + /// The commit message. + /// + /// This may be null, since we didn't always load/store this property in + /// the datastore, so historical entries won't have this information. + String? get message => fields![kCommitMessageField]!.stringValue!; + + /// A serializable form of [slug]. + /// + /// This will be of the form `/`. e.g. `flutter/flutter`. + String? get repositoryPath => fields![kCommitRepositoryPathField]!.stringValue!; + + /// The branch of the commit. + String? get branch => fields![kCommitBranchField]!.stringValue!; + + /// [RepositorySlug] of where this commit exists. + RepositorySlug get slug => RepositorySlug.full(repositoryPath!); + + @override + String toString() { + final StringBuffer buf = StringBuffer() + ..write('$runtimeType(') + ..write(', $kCommitCreateTimestampField: $createTimestamp') + ..write(', $kCommitAuthorField: $author') + ..write(', $kCommitAvatarField: $avatar') + ..write(', $kCommitBranchField: $branch') + ..write(', $kCommitMessageField: $message') + ..write(', $kCommitRepositoryPathField: $repositoryPath') + ..write(', $kCommitShaField: $sha') + ..write(')'); + return buf.toString(); + } +} diff --git a/app_dart/lib/src/model/firestore/task.dart b/app_dart/lib/src/model/firestore/task.dart index 773e8d70a..cfca64b4e 100644 --- a/app_dart/lib/src/model/firestore/task.dart +++ b/app_dart/lib/src/model/firestore/task.dart @@ -13,7 +13,7 @@ import '../luci/push_message.dart'; class Task extends Document { /// Lookup [Task] from Firestore. /// - /// `documentName` follows `/projects/{project}/databases/{<}database}/documents/{document_path}` + /// `documentName` follows `/projects/{project}/databases/{database}/documents/{document_path}` static Future fromFirestore({ required FirestoreService firestoreService, required String documentName, diff --git a/app_dart/lib/src/service/firestore.dart b/app_dart/lib/src/service/firestore.dart index 1cec633e0..e9272115f 100644 --- a/app_dart/lib/src/service/firestore.dart +++ b/app_dart/lib/src/service/firestore.dart @@ -10,6 +10,7 @@ import 'package:http/http.dart'; import '../model/appengine/commit.dart'; import '../model/appengine/task.dart'; +import '../model/firestore/commit.dart' as firestore_comit; import '../model/firestore/task.dart' as firestore; import '../model/ci_yaml/target.dart'; import 'access_client_provider.dart'; @@ -21,6 +22,7 @@ const String kFieldFilterOpEqual = 'EQUAL'; const String kTaskCollectionId = 'tasks'; const int kTaskDefaultTimestampValue = 0; +const int kTaskInitialAttempt = 1; const String kTaskBringupField = 'bringup'; const String kTaskBuildNumberField = 'buildNumber'; const String kTaskCommitShaField = 'commitSha'; @@ -111,38 +113,42 @@ class FirestoreService { } /// Generates task documents based on targets. -List targetsToTaskDocuments(Commit commit, List targets) { - final Iterable iterableDocuments = targets.map( - (Target target) => Document( - name: '$kDatabase/documents/$kTaskCollectionId/${commit.sha}_${target.value.name}_1', - fields: { - kTaskCreateTimestampField: Value(integerValue: commit.timestamp!.toString()), - kTaskEndTimestampField: Value(integerValue: kTaskDefaultTimestampValue.toString()), - kTaskBringupField: Value(booleanValue: target.value.bringup), - kTaskNameField: Value(stringValue: target.value.name), - kTaskStartTimestampField: Value(integerValue: kTaskDefaultTimestampValue.toString()), - kTaskStatusField: Value(stringValue: Task.statusNew), - kTaskTestFlakyField: Value(booleanValue: false), - kTaskCommitShaField: Value(stringValue: commit.sha), - }, +List targetsToTaskDocuments(Commit commit, List targets) { + final Iterable iterableDocuments = targets.map( + (Target target) => firestore.Task.fromDocument( + taskDocument: Document( + name: '$kDatabase/documents/$kTaskCollectionId/${commit.sha}_${target.value.name}_$kTaskInitialAttempt', + fields: { + kTaskCreateTimestampField: Value(integerValue: commit.timestamp!.toString()), + kTaskEndTimestampField: Value(integerValue: kTaskDefaultTimestampValue.toString()), + kTaskBringupField: Value(booleanValue: target.value.bringup), + kTaskNameField: Value(stringValue: target.value.name), + kTaskStartTimestampField: Value(integerValue: kTaskDefaultTimestampValue.toString()), + kTaskStatusField: Value(stringValue: Task.statusNew), + kTaskTestFlakyField: Value(booleanValue: false), + kTaskCommitShaField: Value(stringValue: commit.sha), + }, + ), ), ); return iterableDocuments.toList(); } /// Generates commit document based on datastore commit data model. -Document commitToCommitDocument(Commit commit) { - return Document( - name: '$kDatabase/documents/$kCommitCollectionId/${commit.sha}', - fields: { - kCommitAvatarField: Value(stringValue: commit.authorAvatarUrl), - kCommitBranchField: Value(stringValue: commit.branch), - kCommitCreateTimestampField: Value(integerValue: commit.timestamp.toString()), - kCommitAuthorField: Value(stringValue: commit.author), - kCommitMessageField: Value(stringValue: commit.message), - kCommitRepositoryPathField: Value(stringValue: commit.repository), - kCommitShaField: Value(stringValue: commit.sha), - }, +firestore_comit.Commit commitToCommitDocument(Commit commit) { + return firestore_comit.Commit.fromDocument( + commitDocument: Document( + name: '$kDatabase/documents/$kCommitCollectionId/${commit.sha}', + fields: { + kCommitAvatarField: Value(stringValue: commit.authorAvatarUrl), + kCommitBranchField: Value(stringValue: commit.branch), + kCommitCreateTimestampField: Value(integerValue: commit.timestamp.toString()), + kCommitAuthorField: Value(stringValue: commit.author), + kCommitMessageField: Value(stringValue: commit.message), + kCommitRepositoryPathField: Value(stringValue: commit.repository), + kCommitShaField: Value(stringValue: commit.sha), + }, + ), ); } diff --git a/app_dart/lib/src/service/scheduler.dart b/app_dart/lib/src/service/scheduler.dart index 35a4d403d..83cd0ade8 100644 --- a/app_dart/lib/src/service/scheduler.dart +++ b/app_dart/lib/src/service/scheduler.dart @@ -23,6 +23,8 @@ import '../foundation/typedefs.dart'; import '../foundation/utils.dart'; import '../model/appengine/commit.dart'; import '../model/appengine/task.dart'; +import '../model/firestore/commit.dart' as firestore_commmit; +import '../model/firestore/task.dart' as firestore; import '../model/ci_yaml/ci_yaml.dart'; import '../model/ci_yaml/target.dart'; import '../model/github/checks.dart' as cocoon_checks; @@ -165,8 +167,8 @@ class Scheduler { await _batchScheduleBuilds(commit, toBeScheduled); await _uploadToBigQuery(commit); - final Document commitDocument = commitToCommitDocument(commit); - final List taskDocuments = targetsToTaskDocuments(commit, initialTargets); + final firestore_commmit.Commit commitDocument = commitToCommitDocument(commit); + final List taskDocuments = targetsToTaskDocuments(commit, initialTargets); final List writes = documentsToWrites([...taskDocuments, commitDocument]); final FirestoreService firestoreService = await config.createFirestoreService(); // TODO(keyonghan): remove try catch logic after validated to work. diff --git a/app_dart/test/model/firestore/commit_test.dart b/app_dart/test/model/firestore/commit_test.dart new file mode 100644 index 000000000..9181f5016 --- /dev/null +++ b/app_dart/test/model/firestore/commit_test.dart @@ -0,0 +1,39 @@ +// Copyright 2019 The Flutter Authors. 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:cocoon_service/src/model/firestore/commit.dart'; +import 'package:mockito/mockito.dart'; +import 'package:test/test.dart'; + +import '../../src/utilities/entity_generators.dart'; +import '../../src/utilities/mocks.dart'; + +void main() { + group('Commit.fromFirestore', () { + late MockFirestoreService mockFirestoreService; + + setUp(() { + mockFirestoreService = MockFirestoreService(); + }); + + test('generates commit correctly', () async { + final Commit commit = generateFirestoreCommit(1); + when( + mockFirestoreService.getDocument( + captureAny, + ), + ).thenAnswer((Invocation invocation) { + return Future.value( + commit, + ); + }); + final Commit resultedCommit = await Commit.fromFirestore( + firestoreService: mockFirestoreService, + documentName: 'test', + ); + expect(resultedCommit.name, commit.name); + expect(resultedCommit.fields, commit.fields); + }); + }); +} diff --git a/app_dart/test/service/firestore_test.dart b/app_dart/test/service/firestore_test.dart index 351352429..2237e54fa 100644 --- a/app_dart/test/service/firestore_test.dart +++ b/app_dart/test/service/firestore_test.dart @@ -4,6 +4,7 @@ import 'package:cocoon_service/src/model/appengine/commit.dart'; import 'package:cocoon_service/src/model/appengine/task.dart'; +import 'package:cocoon_service/src/model/firestore/commit.dart' as firestore_commmit; import 'package:cocoon_service/src/model/firestore/task.dart' as firestore; import 'package:cocoon_service/src/model/ci_yaml/target.dart'; import 'package:cocoon_service/src/service/firestore.dart'; @@ -20,9 +21,12 @@ void main() { generateTarget(1, platform: 'Mac'), generateTarget(2, platform: 'Linux'), ]; - final List taskDocuments = targetsToTaskDocuments(commit, targets); + final List taskDocuments = targetsToTaskDocuments(commit, targets); expect(taskDocuments.length, 2); - expect(taskDocuments[0].name, '$kDatabase/documents/$kTaskCollectionId/${commit.sha}_${targets[0].value.name}_1'); + expect( + taskDocuments[0].name, + '$kDatabase/documents/$kTaskCollectionId/${commit.sha}_${targets[0].value.name}_$kTaskInitialAttempt', + ); expect(taskDocuments[0].fields![kTaskCreateTimestampField]!.integerValue, commit.timestamp.toString()); expect(taskDocuments[0].fields![kTaskEndTimestampField]!.integerValue, '0'); expect(taskDocuments[0].fields![kTaskBringupField]!.booleanValue, false); @@ -35,7 +39,7 @@ void main() { test('creates commit document correctly from commit data model', () async { final Commit commit = generateCommit(1); - final Document commitDocument = commitToCommitDocument(commit); + final firestore_commmit.Commit commitDocument = commitToCommitDocument(commit); expect(commitDocument.name, '$kDatabase/documents/$kCommitCollectionId/${commit.sha}'); expect(commitDocument.fields![kCommitAvatarField]!.stringValue, commit.authorAvatarUrl); expect(commitDocument.fields![kCommitBranchField]!.stringValue, commit.branch); diff --git a/app_dart/test/src/utilities/entity_generators.dart b/app_dart/test/src/utilities/entity_generators.dart index 957d83fe1..d214de692 100644 --- a/app_dart/test/src/utilities/entity_generators.dart +++ b/app_dart/test/src/utilities/entity_generators.dart @@ -3,14 +3,17 @@ // found in the LICENSE file. import 'package:cocoon_service/ci_yaml.dart'; +import 'package:cocoon_service/cocoon_service.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/firestore/task.dart' as f; +import 'package:cocoon_service/src/model/firestore/commit.dart' as firestore_commit; +import 'package:cocoon_service/src/model/firestore/task.dart' as firestore; import 'package:cocoon_service/src/model/ci_yaml/target.dart'; import 'package:cocoon_service/src/model/gerrit/commit.dart'; import 'package:cocoon_service/src/model/luci/buildbucket.dart'; import 'package:cocoon_service/src/model/luci/push_message.dart' as push_message; import 'package:cocoon_service/src/model/proto/protos.dart' as pb; +import 'package:cocoon_service/src/service/firestore.dart'; import 'package:gcloud/db.dart'; import 'package:googleapis/firestore/v1.dart' hide Status; import 'package:github/github.dart' as github; @@ -96,7 +99,7 @@ Task generateTask( stageName: stage, ); -f.Task generateFirestoreTask( +firestore.Task generateFirestoreTask( int i, { String? name, String status = Task.statusNew, @@ -111,24 +114,43 @@ f.Task generateFirestoreTask( }) { final String taskName = name ?? 'task$i'; final String sha = commitSha ?? 'testSha'; - final f.Task task = f.Task() + final firestore.Task task = firestore.Task() ..name = '${sha}_${taskName}_$attempts' ..fields = { - 'createTimestamp': Value(integerValue: (created?.millisecondsSinceEpoch ?? 0).toString()), - 'startTimestamp': Value(integerValue: (started?.millisecondsSinceEpoch ?? 0).toString()), - 'endTimestamp': Value(integerValue: (ended?.millisecondsSinceEpoch ?? 0).toString()), - 'bringup': Value(booleanValue: bringup), - 'testFlaky': Value(booleanValue: testFlaky), - 'status': Value(stringValue: status), - 'name': Value(stringValue: taskName), - 'commitSha': Value(stringValue: sha), + kTaskCreateTimestampField: Value(integerValue: (created?.millisecondsSinceEpoch ?? 0).toString()), + kTaskStartTimestampField: Value(integerValue: (started?.millisecondsSinceEpoch ?? 0).toString()), + kTaskEndTimestampField: Value(integerValue: (ended?.millisecondsSinceEpoch ?? 0).toString()), + kTaskBringupField: Value(booleanValue: bringup), + kTaskTestFlakyField: Value(booleanValue: testFlaky), + kTaskStatusField: Value(stringValue: status), + kTaskNameField: Value(stringValue: taskName), + kTaskCommitShaField: Value(stringValue: sha), }; if (buildNumber != null) { - task.fields!['buildNumber'] = Value(integerValue: buildNumber.toString()); + task.fields![kTaskBuildNumberField] = Value(integerValue: buildNumber.toString()); } return task; } +firestore_commit.Commit generateFirestoreCommit( + int i, { + String? sha, + String branch = 'master', + String? owner = 'flutter', + String repo = 'flutter', + int? createTimestamp, +}) { + final firestore_commit.Commit commit = firestore_commit.Commit() + ..name = sha ?? '$i' + ..fields = { + kCommitCreateTimestampField: Value(integerValue: (createTimestamp ?? i).toString()), + kCommitRepositoryPathField: Value(stringValue: '$owner/$repo'), + kCommitBranchField: Value(stringValue: branch), + kCommitShaField: Value(stringValue: sha ?? '$i'), + }; + return commit; +} + Target generateTarget( int i, { pb.SchedulerConfig? schedulerConfig, From 857e09afd7085a43c686217469ba9e8dc7200c85 Mon Sep 17 00:00:00 2001 From: keyonghan <54558023+keyonghan@users.noreply.github.com> Date: Fri, 23 Feb 2024 13:37:14 -0800 Subject: [PATCH 4/4] Updates github gold status to Firestore (#3513) Part of https://github.com/flutter/flutter/issues/142951 This PR: 1) starts updating Github Gold Status to Firestore 2) updates status in a try block to avoid breaking prod workflow 3) creates a corresponding Firestore model 4) updates `documentsToWrites` to have default `exists=null` to support generic document writes. --- app_dart/lib/src/model/firestore/commit.dart | 2 +- .../model/firestore/github_gold_status.dart | 73 +++++++++++++++++++ .../push_gold_status_to_github.dart | 24 +++++- app_dart/lib/src/service/firestore.dart | 33 ++++++++- .../lib/src/service/luci_build_service.dart | 2 +- app_dart/lib/src/service/scheduler.dart | 2 +- .../firestore/github_gold_status_test.dart | 39 ++++++++++ .../push_gold_status_to_github_test.dart | 47 ++++++++++++ app_dart/test/service/firestore_test.dart | 29 +++++++- .../test/src/utilities/entity_generators.dart | 22 ++++++ 10 files changed, 266 insertions(+), 7 deletions(-) create mode 100644 app_dart/lib/src/model/firestore/github_gold_status.dart create mode 100644 app_dart/test/model/firestore/github_gold_status_test.dart diff --git a/app_dart/lib/src/model/firestore/commit.dart b/app_dart/lib/src/model/firestore/commit.dart index dbabb7f44..0ac2994b7 100644 --- a/app_dart/lib/src/model/firestore/commit.dart +++ b/app_dart/lib/src/model/firestore/commit.dart @@ -9,7 +9,7 @@ import 'package:googleapis/firestore/v1.dart' hide Status; import '../../service/firestore.dart'; class Commit extends Document { - /// Lookup [Task] from Firestore. + /// Lookup [Commit] from Firestore. /// /// `documentName` follows `/projects/{project}/databases/{database}/documents/{document_path}` static Future fromFirestore({ diff --git a/app_dart/lib/src/model/firestore/github_gold_status.dart b/app_dart/lib/src/model/firestore/github_gold_status.dart new file mode 100644 index 000000000..78218d768 --- /dev/null +++ b/app_dart/lib/src/model/firestore/github_gold_status.dart @@ -0,0 +1,73 @@ +// Copyright 2019 The Flutter Authors. 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:cocoon_service/cocoon_service.dart'; +import 'package:github/github.dart'; +import 'package:googleapis/firestore/v1.dart' hide Status; + +import '../../service/firestore.dart'; + +class GithubGoldStatus extends Document { + /// Lookup [GithubGoldStatus] from Firestore. + /// + /// `documentName` follows `/projects/{project}/databases/{database}/documents/{document_path}` + static Future fromFirestore({ + required FirestoreService firestoreService, + required String documentName, + }) async { + final Document document = await firestoreService.getDocument(documentName); + return GithubGoldStatus.fromDocument(githubGoldStatus: document); + } + + /// Create [Commit] from a Commit Document. + static GithubGoldStatus fromDocument({ + required Document githubGoldStatus, + }) { + return GithubGoldStatus() + ..fields = githubGoldStatus.fields! + ..name = githubGoldStatus.name!; + } + + // The flutter-gold status cannot report a `failure` status + // due to auto-rollers. This is why we hold a `pending` status + // when there are image changes. This provides the opportunity + // for images to be triaged, and the auto-roller to proceed. + // For more context, see: https://github.com/flutter/flutter/issues/48744 + + static const String statusCompleted = 'success'; + + static const String statusRunning = 'pending'; + + int? get prNumber => int.parse(fields![kGithubGoldStatusPrNumberField]!.integerValue!); + + String? get head => fields![kGithubGoldStatusHeadField]!.stringValue!; + + String? get status => fields![kGithubGoldStatusStatusField]!.stringValue!; + + String? get description => fields![kGithubGoldStatusDescriptionField]!.stringValue!; + + int? get updates => int.parse(fields![kGithubGoldStatusUpdatesField]!.integerValue!); + + /// A serializable form of [slug]. + /// + /// This will be of the form `/`. e.g. `flutter/flutter`. + String? get repository => fields![kGithubGoldStatusRepositoryField]!.stringValue!; + + /// [RepositorySlug] of where this commit exists. + RepositorySlug get slug => RepositorySlug.full(repository!); + + @override + String toString() { + final StringBuffer buf = StringBuffer() + ..write('$runtimeType(') + ..write(', $kGithubGoldStatusPrNumberField: $prNumber') + ..write(', $kGithubGoldStatusHeadField: $head') + ..write(', $kGithubGoldStatusStatusField: $status') + ..write(', $kGithubGoldStatusDescriptionField $description') + ..write(', $kGithubGoldStatusUpdatesField: $updates') + ..write(', $kGithubGoldStatusRepositoryField: $repository') + ..write(')'); + return buf.toString(); + } +} diff --git a/app_dart/lib/src/request_handlers/push_gold_status_to_github.dart b/app_dart/lib/src/request_handlers/push_gold_status_to_github.dart index 52d6a54db..7abbdbe91 100644 --- a/app_dart/lib/src/request_handlers/push_gold_status_to_github.dart +++ b/app_dart/lib/src/request_handlers/push_gold_status_to_github.dart @@ -6,17 +6,18 @@ import 'dart:async'; import 'dart:convert'; import 'dart:io'; +import 'package:cocoon_service/cocoon_service.dart'; import 'package:github/github.dart'; +import 'package:googleapis/firestore/v1.dart'; import 'package:gql/language.dart' as lang; import 'package:graphql/client.dart'; import 'package:http/http.dart' as http; import 'package:meta/meta.dart'; import '../model/appengine/github_gold_status_update.dart'; +import '../model/firestore/github_gold_status.dart'; import '../request_handling/api_request_handler.dart'; -import '../request_handling/body.dart'; import '../request_handling/exceptions.dart'; -import '../service/config.dart'; import '../service/datastore.dart'; import '../service/logging.dart'; @@ -205,6 +206,25 @@ class PushGoldStatusToGithub extends ApiRequestHandler { } await datastore.insert(statusUpdates); log.fine('Committed all updates for $slug'); + + // TODO(keyonghan): remove try block after fully migrated to firestore + // https://github.com/flutter/flutter/issues/142951 + try { + await updateGithubGoldStatusDocuments(statusUpdates); + } catch (error) { + log.warning('Failed to update github gold status in Firestore: $error'); + } + } + + Future updateGithubGoldStatusDocuments(List statusUpdates) async { + if (statusUpdates.isEmpty) { + return; + } + final List githubGoldStatusDocuments = + statusUpdates.map((e) => githubGoldStatusToDocument(e)).toList(); + final List writes = documentsToWrites(githubGoldStatusDocuments); + final FirestoreService firestoreService = await config.createFirestoreService(); + await firestoreService.batchWriteDocuments(BatchWriteRequest(writes: writes), kDatabase); } /// Returns a GitHub Status for the given state and description. diff --git a/app_dart/lib/src/service/firestore.dart b/app_dart/lib/src/service/firestore.dart index e9272115f..efd0ac23b 100644 --- a/app_dart/lib/src/service/firestore.dart +++ b/app_dart/lib/src/service/firestore.dart @@ -9,8 +9,10 @@ import 'package:googleapis/firestore/v1.dart'; import 'package:http/http.dart'; import '../model/appengine/commit.dart'; +import '../model/appengine/github_gold_status_update.dart'; import '../model/appengine/task.dart'; import '../model/firestore/commit.dart' as firestore_comit; +import '../model/firestore/github_gold_status.dart'; import '../model/firestore/task.dart' as firestore; import '../model/ci_yaml/target.dart'; import 'access_client_provider.dart'; @@ -42,6 +44,14 @@ const String kCommitMessageField = 'message'; const String kCommitRepositoryPathField = 'repositoryPath'; const String kCommitShaField = 'sha'; +const String kGithubGoldStatusCollectionId = 'githubGoldStatuses'; +const String kGithubGoldStatusPrNumberField = 'prNumber'; +const String kGithubGoldStatusHeadField = 'head'; +const String kGithubGoldStatusStatusField = 'status'; +const String kGithubGoldStatusDescriptionField = 'description'; +const String kGithubGoldStatusUpdatesField = 'updates'; +const String kGithubGoldStatusRepositoryField = 'repository'; + class FirestoreService { const FirestoreService(this.accessClientProvider); @@ -172,8 +182,29 @@ firestore.Task taskToTaskDocument(Task task) { ); } +/// Generates GithubGoldStatus document based on datastore GithubGoldStatusUpdate data model. +GithubGoldStatus githubGoldStatusToDocument(GithubGoldStatusUpdate githubGoldStatus) { + return GithubGoldStatus.fromDocument( + githubGoldStatus: Document( + name: '$kDatabase/documents/$kGithubGoldStatusCollectionId/${githubGoldStatus.head}_${githubGoldStatus.pr}', + fields: { + kGithubGoldStatusDescriptionField: Value(stringValue: githubGoldStatus.description), + kGithubGoldStatusHeadField: Value(stringValue: githubGoldStatus.head), + kGithubGoldStatusPrNumberField: Value(integerValue: githubGoldStatus.pr.toString()), + kGithubGoldStatusRepositoryField: Value(stringValue: githubGoldStatus.repository), + kGithubGoldStatusStatusField: Value(stringValue: githubGoldStatus.status), + kGithubGoldStatusUpdatesField: Value(integerValue: githubGoldStatus.updates.toString()), + }, + ), + ); +} + /// Creates a list of [Write] based on documents. -List documentsToWrites(List documents, {bool exists = false}) { +/// +/// Null `exists` means either update when a document exists or insert when a document doesn't. +/// `exists = false` means inserting a new document, assuming a document doesn't exist. +/// `exists = true` means updating an existing document, assuming it exisits. +List documentsToWrites(List documents, {bool? exists}) { return documents .map( (Document document) => Write( diff --git a/app_dart/lib/src/service/luci_build_service.dart b/app_dart/lib/src/service/luci_build_service.dart index fa0069b2d..0b5f948c9 100644 --- a/app_dart/lib/src/service/luci_build_service.dart +++ b/app_dart/lib/src/service/luci_build_service.dart @@ -699,7 +699,7 @@ class LuciBuildService { final int newAttempt = int.parse(taskDocument.name!.split('_').last) + 1; tags['current_attempt'] = [newAttempt.toString()]; taskDocument.resetAsRetry(attempt: newAttempt); - final List writes = documentsToWrites([taskDocument]); + final List writes = documentsToWrites([taskDocument], exists: false); await firestoreService!.batchWriteDocuments(BatchWriteRequest(writes: writes), kDatabase); } catch (error) { log.warning('Failed to insert retried task in Firestore: $error'); diff --git a/app_dart/lib/src/service/scheduler.dart b/app_dart/lib/src/service/scheduler.dart index 83cd0ade8..6b0ffefd0 100644 --- a/app_dart/lib/src/service/scheduler.dart +++ b/app_dart/lib/src/service/scheduler.dart @@ -169,7 +169,7 @@ class Scheduler { await _uploadToBigQuery(commit); final firestore_commmit.Commit commitDocument = commitToCommitDocument(commit); final List taskDocuments = targetsToTaskDocuments(commit, initialTargets); - final List writes = documentsToWrites([...taskDocuments, commitDocument]); + final List writes = documentsToWrites([...taskDocuments, commitDocument], exists: false); final FirestoreService firestoreService = await config.createFirestoreService(); // TODO(keyonghan): remove try catch logic after validated to work. try { diff --git a/app_dart/test/model/firestore/github_gold_status_test.dart b/app_dart/test/model/firestore/github_gold_status_test.dart new file mode 100644 index 000000000..7350cc96d --- /dev/null +++ b/app_dart/test/model/firestore/github_gold_status_test.dart @@ -0,0 +1,39 @@ +// Copyright 2019 The Flutter Authors. 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:cocoon_service/src/model/firestore/github_gold_status.dart'; +import 'package:mockito/mockito.dart'; +import 'package:test/test.dart'; + +import '../../src/utilities/entity_generators.dart'; +import '../../src/utilities/mocks.dart'; + +void main() { + group('GithubGoldStatus.fromFirestore', () { + late MockFirestoreService mockFirestoreService; + + setUp(() { + mockFirestoreService = MockFirestoreService(); + }); + + test('generates githubGoldStatus correctly', () async { + final GithubGoldStatus githubGoldStatus = generateFirestoreGithubGoldStatus(1); + when( + mockFirestoreService.getDocument( + captureAny, + ), + ).thenAnswer((Invocation invocation) { + return Future.value( + githubGoldStatus, + ); + }); + final GithubGoldStatus resultedGithubGoldStatus = await GithubGoldStatus.fromFirestore( + firestoreService: mockFirestoreService, + documentName: 'test', + ); + expect(resultedGithubGoldStatus.name, githubGoldStatus.name); + expect(resultedGithubGoldStatus.fields, githubGoldStatus.fields); + }); + }); +} diff --git a/app_dart/test/request_handlers/push_gold_status_to_github_test.dart b/app_dart/test/request_handlers/push_gold_status_to_github_test.dart index 261cf28da..58af3b073 100644 --- a/app_dart/test/request_handlers/push_gold_status_to_github_test.dart +++ b/app_dart/test/request_handlers/push_gold_status_to_github_test.dart @@ -6,6 +6,7 @@ import 'dart:async'; import 'dart:io'; import 'package:cocoon_service/src/model/appengine/github_gold_status_update.dart'; +import 'package:cocoon_service/src/model/firestore/github_gold_status.dart'; import 'package:cocoon_service/src/request_handlers/push_gold_status_to_github.dart'; import 'package:cocoon_service/src/request_handling/body.dart'; import 'package:cocoon_service/src/service/datastore.dart'; @@ -13,6 +14,7 @@ import 'package:cocoon_service/src/service/logging.dart'; import 'package:gcloud/db.dart' as gcloud_db; import 'package:gcloud/db.dart'; import 'package:github/github.dart'; +import 'package:googleapis/firestore/v1.dart'; import 'package:graphql/client.dart'; import 'package:http/http.dart' as http; import 'package:http/testing.dart'; @@ -36,6 +38,7 @@ void main() { late FakeClientContext clientContext; FakeAuthenticatedContext authContext; late FakeAuthenticationProvider auth; + late MockFirestoreService mockFirestoreService; late FakeDatastoreDB db; late ApiRequestHandlerTester tester; late PushGoldStatusToGithub handler; @@ -51,12 +54,14 @@ void main() { setUp(() { clientContext = FakeClientContext(); + mockFirestoreService = MockFirestoreService(); authContext = FakeAuthenticatedContext(clientContext: clientContext); auth = FakeAuthenticationProvider(clientContext: clientContext); githubGraphQLClient = FakeGraphQLClient(); db = FakeDatastoreDB(); config = FakeConfig( dbValue: db, + firestoreService: mockFirestoreService, ); tester = ApiRequestHandlerTester(context: authContext); mockHttpClient = MockClient((_) async => http.Response('{}', HttpStatus.ok)); @@ -749,6 +754,16 @@ void main() { }); group('updates GitHub and/or Datastore', () { + setUp(() { + when( + mockFirestoreService.batchWriteDocuments( + captureAny, + captureAny, + ), + ).thenAnswer((Invocation invocation) { + return Future.value(BatchWriteResponse()); + }); + }); test('new commit, checks running', () async { // New commit final PullRequest flutterPr = newPullRequest(123, 'f-abc', 'master'); @@ -779,6 +794,22 @@ void main() { expect(records.where((LogRecord record) => record.level == Level.WARNING), isEmpty); expect(records.where((LogRecord record) => record.level == Level.SEVERE), isEmpty); + final List captured = + verify(mockFirestoreService.batchWriteDocuments(captureAny, captureAny)).captured; + expect(captured.length, 4); + // The first element corresponds to the `status`. + final BatchWriteRequest batchWriteRequest = captured[0] as BatchWriteRequest; + expect(batchWriteRequest.writes!.length, 1); + final GithubGoldStatus updatedDocument = + GithubGoldStatus.fromDocument(githubGoldStatus: batchWriteRequest.writes![0].update!); + expect(updatedDocument.updates, status.updates); + // The third element corresponds to the `engineStatus`. + final BatchWriteRequest batchWriteRequestEngine = captured[2] as BatchWriteRequest; + expect(batchWriteRequestEngine.writes!.length, 1); + final GithubGoldStatus updatedDocumentEngine = + GithubGoldStatus.fromDocument(githubGoldStatus: batchWriteRequest.writes![0].update!); + expect(updatedDocumentEngine.updates, engineStatus.updates); + // Should not apply labels or make comments verifyNever( issuesService.addLabelsToIssue( @@ -1353,6 +1384,14 @@ void main() { }); test('Completed pull request does not skip follow-up prs with early return', () async { + when( + mockFirestoreService.batchWriteDocuments( + captureAny, + captureAny, + ), + ).thenAnswer((Invocation invocation) { + return Future.value(BatchWriteResponse()); + }); final PullRequest completedPR = newPullRequest(123, 'abc', 'master'); final PullRequest followUpPR = newPullRequest(456, 'def', 'master'); prsFromGitHub = [ @@ -1466,6 +1505,14 @@ void main() { }); test('uses the correct Gold endpoint to get status', () async { + when( + mockFirestoreService.batchWriteDocuments( + captureAny, + captureAny, + ), + ).thenAnswer((Invocation invocation) { + return Future.value(BatchWriteResponse()); + }); // New commit final PullRequest pr = newPullRequest(123, 'abc', 'master'); prsFromGitHub = [pr]; diff --git a/app_dart/test/service/firestore_test.dart b/app_dart/test/service/firestore_test.dart index 2237e54fa..30f3bd6ad 100644 --- a/app_dart/test/service/firestore_test.dart +++ b/app_dart/test/service/firestore_test.dart @@ -3,8 +3,10 @@ // found in the LICENSE file. import 'package:cocoon_service/src/model/appengine/commit.dart'; +import 'package:cocoon_service/src/model/appengine/github_gold_status_update.dart'; import 'package:cocoon_service/src/model/appengine/task.dart'; import 'package:cocoon_service/src/model/firestore/commit.dart' as firestore_commmit; +import 'package:cocoon_service/src/model/firestore/github_gold_status.dart'; import 'package:cocoon_service/src/model/firestore/task.dart' as firestore; import 'package:cocoon_service/src/model/ci_yaml/target.dart'; import 'package:cocoon_service/src/service/firestore.dart'; @@ -50,6 +52,31 @@ void main() { expect(commitDocument.fields![kCommitShaField]!.stringValue, commit.sha); }); + test('creates github gold status document correctly from data model', () async { + final GithubGoldStatusUpdate githubGoldStatusUpdate = GithubGoldStatusUpdate( + head: 'sha', + pr: 1, + status: GithubGoldStatusUpdate.statusCompleted, + updates: 2, + description: '', + repository: '', + ); + final GithubGoldStatus commitDocument = githubGoldStatusToDocument(githubGoldStatusUpdate); + expect( + commitDocument.name, + '$kDatabase/documents/$kGithubGoldStatusCollectionId/${githubGoldStatusUpdate.head}_${githubGoldStatusUpdate.pr}', + ); + expect(commitDocument.fields![kGithubGoldStatusHeadField]!.stringValue, githubGoldStatusUpdate.head); + expect(commitDocument.fields![kGithubGoldStatusPrNumberField]!.integerValue, githubGoldStatusUpdate.pr.toString()); + expect(commitDocument.fields![kGithubGoldStatusStatusField]!.stringValue, githubGoldStatusUpdate.status); + expect( + commitDocument.fields![kGithubGoldStatusUpdatesField]!.integerValue, + githubGoldStatusUpdate.updates.toString(), + ); + expect(commitDocument.fields![kGithubGoldStatusDescriptionField]!.stringValue, ''); + expect(commitDocument.fields![kGithubGoldStatusRepositoryField]!.stringValue, ''); + }); + test('creates task document correctly from task data model', () async { final Task task = generateTask(1); final String commitSha = task.commitKey!.id!.split('/').last; @@ -70,7 +97,7 @@ void main() { Document(name: 'd1', fields: {'key1': Value(stringValue: 'value1')}), Document(name: 'd2', fields: {'key1': Value(stringValue: 'value2')}), ]; - final List writes = documentsToWrites(documents); + final List writes = documentsToWrites(documents, exists: false); expect(writes.length, documents.length); expect(writes[0].update, documents[0]); expect(writes[0].currentDocument!.exists, false); diff --git a/app_dart/test/src/utilities/entity_generators.dart b/app_dart/test/src/utilities/entity_generators.dart index d214de692..f0bbacc4c 100644 --- a/app_dart/test/src/utilities/entity_generators.dart +++ b/app_dart/test/src/utilities/entity_generators.dart @@ -7,6 +7,7 @@ import 'package:cocoon_service/cocoon_service.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/firestore/commit.dart' as firestore_commit; +import 'package:cocoon_service/src/model/firestore/github_gold_status.dart'; import 'package:cocoon_service/src/model/firestore/task.dart' as firestore; import 'package:cocoon_service/src/model/ci_yaml/target.dart'; import 'package:cocoon_service/src/model/gerrit/commit.dart'; @@ -151,6 +152,27 @@ firestore_commit.Commit generateFirestoreCommit( return commit; } +GithubGoldStatus generateFirestoreGithubGoldStatus( + int i, { + String? head, + int? pr, + String owner = 'flutter', + String repo = 'flutter', + int? updates, +}) { + pr ??= i; + head ??= 'sha$i'; + final GithubGoldStatus commit = GithubGoldStatus() + ..name = '{$pr}_$head' + ..fields = { + kGithubGoldStatusHeadField: Value(stringValue: head), + kGithubGoldStatusPrNumberField: Value(integerValue: pr.toString()), + kGithubGoldStatusRepositoryField: Value(stringValue: '$owner/$repo'), + kGithubGoldStatusUpdatesField: Value(integerValue: updates.toString()), + }; + return commit; +} + Target generateTarget( int i, { pb.SchedulerConfig? schedulerConfig,