From 5234762b29e56df88adfd0eb46f79bb9e2903d23 Mon Sep 17 00:00:00 2001 From: Keyong Han Date: Tue, 28 Jul 2020 16:01:46 -0700 Subject: [PATCH 1/6] add try builder file to cocoon --- dev/cocoon_try_builders.json | 8 ++++++++ 1 file changed, 8 insertions(+) create mode 100644 dev/cocoon_try_builders.json diff --git a/dev/cocoon_try_builders.json b/dev/cocoon_try_builders.json new file mode 100644 index 0000000000..d6b09594b5 --- /dev/null +++ b/dev/cocoon_try_builders.json @@ -0,0 +1,8 @@ +{ + "builders":[ + { + "name":"Cocoon", + "repo":"cocoon" + } + ] +} From f8d475c8b57e0944766fe8555110bc50872085d6 Mon Sep 17 00:00:00 2001 From: Keyong Han Date: Wed, 29 Jul 2020 13:58:34 -0700 Subject: [PATCH 2/6] add readme and test for json validity --- app_dart/lib/src/datastore/cocoon_config.dart | 6 +- app_dart/lib/src/foundation/utils.dart | 15 +- .../test/dev/cocoon_try_builders_test.dart | 172 ++++++++++++++++++ app_dart/test/foundation/utils_test.dart | 50 ++--- dev/README.md | 19 ++ 5 files changed, 229 insertions(+), 33 deletions(-) create mode 100644 app_dart/test/dev/cocoon_try_builders_test.dart create mode 100644 dev/README.md diff --git a/app_dart/lib/src/datastore/cocoon_config.dart b/app_dart/lib/src/datastore/cocoon_config.dart index 3a3a611cdf..75ee262c27 100644 --- a/app_dart/lib/src/datastore/cocoon_config.dart +++ b/app_dart/lib/src/datastore/cocoon_config.dart @@ -72,11 +72,7 @@ class Config { // Gets supported luci builders based on github http request. Future>> _getLuciBuilders(String bucket) async { - final String builderContent = - await getBuilders(Providers.freshHttpClient, loggingService, twoSecondLinearBackoff, bucket); - final Map builderMap = json.decode(builderContent) as Map; - final List builderList = builderMap['builders'] as List; - return builderList.map((dynamic builder) => builder as Map).toList(); + return await getBuilders(Providers.freshHttpClient, loggingService, twoSecondLinearBackoff, bucket); } Future _getSingleValue(String id) async { diff --git a/app_dart/lib/src/foundation/utils.dart b/app_dart/lib/src/foundation/utils.dart index 47842c3165..1c206d0556 100644 --- a/app_dart/lib/src/foundation/utils.dart +++ b/app_dart/lib/src/foundation/utils.dart @@ -84,9 +84,18 @@ Future repoNameForBuilder(List> builders, S } /// Gets supported luci builders based on [bucket] via GitHub http request. -Future getBuilders(HttpClientProvider branchHttpClientProvider, Logging log, +Future>> getBuilders(HttpClientProvider branchHttpClientProvider, Logging log, GitHubBackoffCalculator gitHubBackoffCalculator, String bucket) async { final String filename = bucket == 'try' ? 'luci_try_builders.json' : 'luci_prod_builders.json'; - final String content = await remoteFileContent(branchHttpClientProvider, log, gitHubBackoffCalculator, filename); - return content ?? '{"builders":[]}'; + String builderContent = await remoteFileContent(branchHttpClientProvider, log, gitHubBackoffCalculator, filename); + builderContent ??= '{"builders":[]}'; + Map builderMap; + try { + builderMap = json.decode(builderContent) as Map; + } on FormatException catch (e) { + log.error('error: $e'); + builderMap = {'builders':[]}; + } + final List builderList = builderMap['builders'] as List; + return builderList.map((dynamic builder) => builder as Map).toList(); } diff --git a/app_dart/test/dev/cocoon_try_builders_test.dart b/app_dart/test/dev/cocoon_try_builders_test.dart new file mode 100644 index 0000000000..2aed69ee59 --- /dev/null +++ b/app_dart/test/dev/cocoon_try_builders_test.dart @@ -0,0 +1,172 @@ +// Copyright 2020 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 'dart:io'; +import 'dart:typed_data'; + +import 'package:appengine/appengine.dart'; +import 'package:github/github.dart'; +import 'package:test/test.dart'; +import 'package:cocoon_service/src/foundation/utils.dart'; + +import '../src/request_handling/fake_http.dart'; +import '../src/request_handling/fake_logging.dart'; + +const String branchRegExp = ''' + master + flutter-1.1-candidate.1 + '''; +const String luciBuilders = ''' + builders: [ + { + test1:value1, + test2:value2, + },{ + test1:value3, + test2:value4, + test3:value5, + } + ] + '''; + +void main() { + group('Test utils', () { + group('LoadBranches', () { + FakeHttpClient branchHttpClient; + FakeLogging log; + + setUp(() { + branchHttpClient = FakeHttpClient(); + log = FakeLogging(); + }); + + test('returns branches', () async { + branchHttpClient.request.response.body = branchRegExp; + final String branches = + await remoteFileContent(() => branchHttpClient, log, (int attempt) => Duration.zero, 'branches.txt'); + final List branchList = branches.split('\n').map((String branch) => branch.trim()).toList(); + branchList.removeWhere((String branch) => branch.isEmpty); + expect(branchList, ['master', 'flutter-1.1-candidate.1']); + }); + + test('retries branches download upon HTTP failure', () async { + int retry = 0; + branchHttpClient.onIssueRequest = (FakeHttpClientRequest request) { + request.response.statusCode = retry == 0 ? HttpStatus.serviceUnavailable : HttpStatus.ok; + retry++; + }; + + branchHttpClient.request.response.body = branchRegExp; + final String branches = + await remoteFileContent(() => branchHttpClient, log, (int attempt) => Duration.zero, 'branches.txt'); + final List branchList = branches.split('\n').map((String branch) => branch.trim()).toList(); + branchList.removeWhere((String branch) => branch.isEmpty); + expect(retry, 2); + expect(branchList, ['master', 'flutter-1.1-candidate.1']); + expect(log.records.where(hasLevel(LogLevel.WARNING)), isNotEmpty); + expect(log.records.where(hasLevel(LogLevel.ERROR)), isEmpty); + }); + + test('gives up branches download after 3 tries', () async { + int retry = 0; + branchHttpClient.onIssueRequest = (FakeHttpClientRequest request) => retry++; + branchHttpClient.request.response.statusCode = HttpStatus.serviceUnavailable; + branchHttpClient.request.response.body = branchRegExp; + final String branches = + await remoteFileContent(() => branchHttpClient, log, (int attempt) => Duration.zero, 'branches.txt'); + expect(branches, null); + expect(retry, 3); + expect(log.records.where(hasLevel(LogLevel.WARNING)), isNotEmpty); + expect(log.records.where(hasLevel(LogLevel.ERROR)), isNotEmpty); + }); + }); + + group('GetBranches', () { + FakeHttpClient branchHttpClient; + FakeLogging log; + + setUp(() { + branchHttpClient = FakeHttpClient(); + log = FakeLogging(); + }); + test('returns branches', () async { + branchHttpClient.request.response.body = branchRegExp; + final Uint8List branches = await getBranches(() => branchHttpClient, log, (int attempt) => Duration.zero); + expect(String.fromCharCodes(branches), 'master,flutter-1.1-candidate.1'); + }); + + test('returns master when http request fails', () async { + int retry = 0; + branchHttpClient.onIssueRequest = (FakeHttpClientRequest request) => retry++; + branchHttpClient.request.response.statusCode = HttpStatus.serviceUnavailable; + branchHttpClient.request.response.body = luciBuilders; + final Uint8List builders = await getBranches(() => branchHttpClient, log, (int attempt) => Duration.zero); + expect(String.fromCharCodes(builders), 'master'); + }); + }); + + group('GetBuilders', () { + FakeHttpClient lucBuilderHttpClient; + FakeLogging log; + + setUp(() { + lucBuilderHttpClient = FakeHttpClient(); + log = FakeLogging(); + }); + test('returns luci builders', () async { + lucBuilderHttpClient.request.response.body = luciBuilders; + final String builders = + await getBuilders(() => lucBuilderHttpClient, log, (int attempt) => Duration.zero, 'try'); + expect(builders, ''' + builders: [ + { + test1:value1, + test2:value2, + },{ + test1:value3, + test2:value4, + test3:value5, + } + ] + '''); + }); + + test('returns empty list when http request fails', () async { + int retry = 0; + lucBuilderHttpClient.onIssueRequest = (FakeHttpClientRequest request) => retry++; + lucBuilderHttpClient.request.response.statusCode = HttpStatus.serviceUnavailable; + lucBuilderHttpClient.request.response.body = luciBuilders; + final String builders = + await getBuilders(() => lucBuilderHttpClient, log, (int attempt) => Duration.zero, 'try'); + expect(builders, '{"builders":[]}'); + }); + }); + + group('GitHubBackoffCalculator', () { + test('twoSecondLinearBackoff', () { + expect(twoSecondLinearBackoff(0), const Duration(seconds: 2)); + expect(twoSecondLinearBackoff(1), const Duration(seconds: 4)); + expect(twoSecondLinearBackoff(2), const Duration(seconds: 6)); + expect(twoSecondLinearBackoff(3), const Duration(seconds: 8)); + }); + }); + + group('repoNameForBuilder', () { + test('Builder config does not exist', () async { + final List> builders = >[]; + final RepositorySlug result = await repoNameForBuilder(builders, 'DoesNotExist'); + expect(result, isNull); + }); + + test('Builder exists', () async { + final List> builders = >[ + {'name': 'Cocoon', 'repo': 'cocoon'} + ]; + final RepositorySlug result = await repoNameForBuilder(builders, 'Cocoon'); + expect(result, isNotNull); + expect(result.fullName, equals('flutter/cocoon')); + }); + }); + }); +} diff --git a/app_dart/test/foundation/utils_test.dart b/app_dart/test/foundation/utils_test.dart index 2aed69ee59..e536c35de6 100644 --- a/app_dart/test/foundation/utils_test.dart +++ b/app_dart/test/foundation/utils_test.dart @@ -18,16 +18,17 @@ const String branchRegExp = ''' flutter-1.1-candidate.1 '''; const String luciBuilders = ''' - builders: [ - { - test1:value1, - test2:value2, - },{ - test1:value3, - test2:value4, - test3:value5, - } - ] + { + "builders":[ + { + "name":"Cocoon", + "repo":"cocoon" + }, { + "name":"Cocoon2", + "repo":"cocoon" + } + ] + } '''; void main() { @@ -116,20 +117,19 @@ void main() { }); test('returns luci builders', () async { lucBuilderHttpClient.request.response.body = luciBuilders; - final String builders = + final List> builders = await getBuilders(() => lucBuilderHttpClient, log, (int attempt) => Duration.zero, 'try'); - expect(builders, ''' - builders: [ - { - test1:value1, - test2:value2, - },{ - test1:value3, - test2:value4, - test3:value5, - } - ] - '''); + expect(builders.length, 2); + expect(builders[0]['name'], 'Cocoon'); + expect(builders[0]['repo'], 'cocoon'); + }); + + test('logs error and returns empty list when json file is invalid', () async { + lucBuilderHttpClient.request.response.body = '{"builders":['; + final List> builders = + await getBuilders(() => lucBuilderHttpClient, log, (int attempt) => Duration.zero, 'try'); + expect(log.records.where(hasLevel(LogLevel.ERROR)), isNotEmpty); + expect(builders.length, 0); }); test('returns empty list when http request fails', () async { @@ -137,9 +137,9 @@ void main() { lucBuilderHttpClient.onIssueRequest = (FakeHttpClientRequest request) => retry++; lucBuilderHttpClient.request.response.statusCode = HttpStatus.serviceUnavailable; lucBuilderHttpClient.request.response.body = luciBuilders; - final String builders = + final List> builders = await getBuilders(() => lucBuilderHttpClient, log, (int attempt) => Duration.zero, 'try'); - expect(builders, '{"builders":[]}'); + expect(builders.length, 0); }); }); diff --git a/dev/README.md b/dev/README.md new file mode 100644 index 0000000000..f4b65da2ea --- /dev/null +++ b/dev/README.md @@ -0,0 +1,19 @@ +This directory contains resources that the Flutter team uses during +the development of cocoon. + +## Luci builder file +`cocoon_try_builders.json` contains the supported luci try builders +for cocoon. It follows format: +```json +{ + "builders":[ + { + "name":"xxx", + "repo":"cocoon" + } + ] +} +``` +This file will be mainly used in [`flutter/cocoon`](https://github.com/flutter/cocoon) +to update luci task statuses of cocoob pre-submit tests. + From 341469286e157a235825b60ec82defc02c72ac49 Mon Sep 17 00:00:00 2001 From: Keyong Han Date: Wed, 29 Jul 2020 14:01:47 -0700 Subject: [PATCH 3/6] formatting --- app_dart/lib/src/foundation/utils.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app_dart/lib/src/foundation/utils.dart b/app_dart/lib/src/foundation/utils.dart index 1c206d0556..304d7d4725 100644 --- a/app_dart/lib/src/foundation/utils.dart +++ b/app_dart/lib/src/foundation/utils.dart @@ -94,7 +94,7 @@ Future>> getBuilders(HttpClientProvider branchHttpClie builderMap = json.decode(builderContent) as Map; } on FormatException catch (e) { log.error('error: $e'); - builderMap = {'builders':[]}; + builderMap = {'builders': []}; } final List builderList = builderMap['builders'] as List; return builderList.map((dynamic builder) => builder as Map).toList(); From 9e630e271d7cdd952f1ba382f6b0955681a8b823 Mon Sep 17 00:00:00 2001 From: Keyong Han Date: Wed, 29 Jul 2020 14:07:02 -0700 Subject: [PATCH 4/6] clean up --- .../test/dev/cocoon_try_builders_test.dart | 172 ------------------ 1 file changed, 172 deletions(-) delete mode 100644 app_dart/test/dev/cocoon_try_builders_test.dart diff --git a/app_dart/test/dev/cocoon_try_builders_test.dart b/app_dart/test/dev/cocoon_try_builders_test.dart deleted file mode 100644 index 2aed69ee59..0000000000 --- a/app_dart/test/dev/cocoon_try_builders_test.dart +++ /dev/null @@ -1,172 +0,0 @@ -// Copyright 2020 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 'dart:io'; -import 'dart:typed_data'; - -import 'package:appengine/appengine.dart'; -import 'package:github/github.dart'; -import 'package:test/test.dart'; -import 'package:cocoon_service/src/foundation/utils.dart'; - -import '../src/request_handling/fake_http.dart'; -import '../src/request_handling/fake_logging.dart'; - -const String branchRegExp = ''' - master - flutter-1.1-candidate.1 - '''; -const String luciBuilders = ''' - builders: [ - { - test1:value1, - test2:value2, - },{ - test1:value3, - test2:value4, - test3:value5, - } - ] - '''; - -void main() { - group('Test utils', () { - group('LoadBranches', () { - FakeHttpClient branchHttpClient; - FakeLogging log; - - setUp(() { - branchHttpClient = FakeHttpClient(); - log = FakeLogging(); - }); - - test('returns branches', () async { - branchHttpClient.request.response.body = branchRegExp; - final String branches = - await remoteFileContent(() => branchHttpClient, log, (int attempt) => Duration.zero, 'branches.txt'); - final List branchList = branches.split('\n').map((String branch) => branch.trim()).toList(); - branchList.removeWhere((String branch) => branch.isEmpty); - expect(branchList, ['master', 'flutter-1.1-candidate.1']); - }); - - test('retries branches download upon HTTP failure', () async { - int retry = 0; - branchHttpClient.onIssueRequest = (FakeHttpClientRequest request) { - request.response.statusCode = retry == 0 ? HttpStatus.serviceUnavailable : HttpStatus.ok; - retry++; - }; - - branchHttpClient.request.response.body = branchRegExp; - final String branches = - await remoteFileContent(() => branchHttpClient, log, (int attempt) => Duration.zero, 'branches.txt'); - final List branchList = branches.split('\n').map((String branch) => branch.trim()).toList(); - branchList.removeWhere((String branch) => branch.isEmpty); - expect(retry, 2); - expect(branchList, ['master', 'flutter-1.1-candidate.1']); - expect(log.records.where(hasLevel(LogLevel.WARNING)), isNotEmpty); - expect(log.records.where(hasLevel(LogLevel.ERROR)), isEmpty); - }); - - test('gives up branches download after 3 tries', () async { - int retry = 0; - branchHttpClient.onIssueRequest = (FakeHttpClientRequest request) => retry++; - branchHttpClient.request.response.statusCode = HttpStatus.serviceUnavailable; - branchHttpClient.request.response.body = branchRegExp; - final String branches = - await remoteFileContent(() => branchHttpClient, log, (int attempt) => Duration.zero, 'branches.txt'); - expect(branches, null); - expect(retry, 3); - expect(log.records.where(hasLevel(LogLevel.WARNING)), isNotEmpty); - expect(log.records.where(hasLevel(LogLevel.ERROR)), isNotEmpty); - }); - }); - - group('GetBranches', () { - FakeHttpClient branchHttpClient; - FakeLogging log; - - setUp(() { - branchHttpClient = FakeHttpClient(); - log = FakeLogging(); - }); - test('returns branches', () async { - branchHttpClient.request.response.body = branchRegExp; - final Uint8List branches = await getBranches(() => branchHttpClient, log, (int attempt) => Duration.zero); - expect(String.fromCharCodes(branches), 'master,flutter-1.1-candidate.1'); - }); - - test('returns master when http request fails', () async { - int retry = 0; - branchHttpClient.onIssueRequest = (FakeHttpClientRequest request) => retry++; - branchHttpClient.request.response.statusCode = HttpStatus.serviceUnavailable; - branchHttpClient.request.response.body = luciBuilders; - final Uint8List builders = await getBranches(() => branchHttpClient, log, (int attempt) => Duration.zero); - expect(String.fromCharCodes(builders), 'master'); - }); - }); - - group('GetBuilders', () { - FakeHttpClient lucBuilderHttpClient; - FakeLogging log; - - setUp(() { - lucBuilderHttpClient = FakeHttpClient(); - log = FakeLogging(); - }); - test('returns luci builders', () async { - lucBuilderHttpClient.request.response.body = luciBuilders; - final String builders = - await getBuilders(() => lucBuilderHttpClient, log, (int attempt) => Duration.zero, 'try'); - expect(builders, ''' - builders: [ - { - test1:value1, - test2:value2, - },{ - test1:value3, - test2:value4, - test3:value5, - } - ] - '''); - }); - - test('returns empty list when http request fails', () async { - int retry = 0; - lucBuilderHttpClient.onIssueRequest = (FakeHttpClientRequest request) => retry++; - lucBuilderHttpClient.request.response.statusCode = HttpStatus.serviceUnavailable; - lucBuilderHttpClient.request.response.body = luciBuilders; - final String builders = - await getBuilders(() => lucBuilderHttpClient, log, (int attempt) => Duration.zero, 'try'); - expect(builders, '{"builders":[]}'); - }); - }); - - group('GitHubBackoffCalculator', () { - test('twoSecondLinearBackoff', () { - expect(twoSecondLinearBackoff(0), const Duration(seconds: 2)); - expect(twoSecondLinearBackoff(1), const Duration(seconds: 4)); - expect(twoSecondLinearBackoff(2), const Duration(seconds: 6)); - expect(twoSecondLinearBackoff(3), const Duration(seconds: 8)); - }); - }); - - group('repoNameForBuilder', () { - test('Builder config does not exist', () async { - final List> builders = >[]; - final RepositorySlug result = await repoNameForBuilder(builders, 'DoesNotExist'); - expect(result, isNull); - }); - - test('Builder exists', () async { - final List> builders = >[ - {'name': 'Cocoon', 'repo': 'cocoon'} - ]; - final RepositorySlug result = await repoNameForBuilder(builders, 'Cocoon'); - expect(result, isNotNull); - expect(result.fullName, equals('flutter/cocoon')); - }); - }); - }); -} From a98835a08754c85ddf415bef8ddf6234d3b2c23b Mon Sep 17 00:00:00 2001 From: Keyong Han Date: Wed, 29 Jul 2020 15:29:42 -0700 Subject: [PATCH 5/6] typo --- dev/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dev/README.md b/dev/README.md index f4b65da2ea..b06e3b7e75 100644 --- a/dev/README.md +++ b/dev/README.md @@ -15,5 +15,5 @@ for cocoon. It follows format: } ``` This file will be mainly used in [`flutter/cocoon`](https://github.com/flutter/cocoon) -to update luci task statuses of cocoob pre-submit tests. +to update luci task statuses of cocoon pre-submit tests. From cf08adc9d8495bdd999c28f5060bf8f23ddd2406 Mon Sep 17 00:00:00 2001 From: Keyong Han Date: Fri, 31 Jul 2020 11:11:34 -0700 Subject: [PATCH 6/6] add test to validate json contents --- dev/README.md | 5 ++++- dev/validate_json.dart | 38 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 1 deletion(-) create mode 100644 dev/validate_json.dart diff --git a/dev/README.md b/dev/README.md index b06e3b7e75..f027152584 100644 --- a/dev/README.md +++ b/dev/README.md @@ -15,5 +15,8 @@ for cocoon. It follows format: } ``` This file will be mainly used in [`flutter/cocoon`](https://github.com/flutter/cocoon) -to update luci task statuses of cocoon pre-submit tests. +to trigger LUCI presubmit tasks. + +If any new changes, please validate json contents by running +`dart validat_json.dart cocoon_try_builders.json`. diff --git a/dev/validate_json.dart b/dev/validate_json.dart new file mode 100644 index 0000000000..e216093e10 --- /dev/null +++ b/dev/validate_json.dart @@ -0,0 +1,38 @@ +// Copyright 2020 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 'dart:convert'; +import 'dart:io' show File; + +/// Validates if the input builders JSON file has valid contents. +/// +/// Examples: +/// dart validate_json.dart /path/to/json/file +Future main(List args) async { + final String jsonString = await File(args[0]).readAsString(); + Map decodedJson; + bool decodedStatus = true; + try { + decodedJson = json.decode(jsonString) as Map; + final List builders = decodedJson['builders'] as List; + if (builders == null) { + print('Json format is violated: no "builders" exists. Please follow'); + print(''' + { + "builders":[ + { + "name":"xxx", + "repo":"cocoon" + } + ] + }'''); + return decodedStatus = false; + } + } on FormatException catch (e) { + print('error: $e'); + return decodedStatus = false; + } + print('Success.'); + return decodedStatus; +}