From b67e26420353ef40fdab33fe031638cf83f90378 Mon Sep 17 00:00:00 2001 From: keyonghan <54558023+keyonghan@users.noreply.github.com> Date: Thu, 26 Aug 2021 09:41:22 -0700 Subject: [PATCH] Revert "Skip staging test update to cocoon in test runner (#88835)" (#88971) This reverts commit 884dfc260d5aea0c7bee16548df9ee031b8936e9. --- dev/devicelab/lib/command/upload_results.dart | 17 +++++--- dev/devicelab/lib/framework/cocoon.dart | 18 +++----- .../lib/framework/metrics_center.dart | 10 +---- dev/devicelab/test/cocoon_test.dart | 43 ++++--------------- 4 files changed, 29 insertions(+), 59 deletions(-) diff --git a/dev/devicelab/lib/command/upload_results.dart b/dev/devicelab/lib/command/upload_results.dart index 19f4a3aa4b..df7f06fd7d 100644 --- a/dev/devicelab/lib/command/upload_results.dart +++ b/dev/devicelab/lib/command/upload_results.dart @@ -41,14 +41,21 @@ class UploadResultsCommand extends Command { final String? testStatus = argResults!['test-status'] as String?; final String? commitTime = argResults!['commit-time'] as String?; - // Upload metrics to skia perf from test runner when `resultsPath` is specified. - if (resultsPath != null) { - await uploadToSkiaPerf(resultsPath, commitTime); - print('Successfully uploaded metrics to skia perf'); + // Upload metrics to metrics_center from test runner when `commitTime` is specified. This + // is mainly for testing purpose. + // The upload step will be skipped from cocoon once this is validated. + // TODO(keyong): remove try block to block test when this is validated to work https://github.com/flutter/flutter/issues/88484 + try { + if (commitTime != null) { + await uploadToMetricsCenter(resultsPath, commitTime); + print('Successfully uploaded metrics to metrics center'); + } + } on Exception catch (e, stacktrace) { + print('Uploading metrics failure: $e\n\n$stacktrace'); } final Cocoon cocoon = Cocoon(serviceAccountTokenPath: serviceAccountTokenFile); - return cocoon.sendTaskStatus( + return cocoon.sendResultsPath( resultsPath: resultsPath, isTestFlaky: testFlakyStatus == 'True', gitBranch: gitBranch, diff --git a/dev/devicelab/lib/framework/cocoon.dart b/dev/devicelab/lib/framework/cocoon.dart index 2fe5d0aab9..3d3d38070a 100644 --- a/dev/devicelab/lib/framework/cocoon.dart +++ b/dev/devicelab/lib/framework/cocoon.dart @@ -75,16 +75,16 @@ class Cocoon { return _commitSha = result.stdout as String; } - /// Update test status to Cocoon. + /// Upload the JSON results in [resultsPath] to Cocoon. /// /// Flutter infrastructure's workflow is: - /// 1. Run DeviceLab test + /// 1. Run DeviceLab test, writing results to a known path /// 2. Request service account token from luci auth (valid for at least 3 minutes) - /// 3. Update test status from (1) to Cocoon + /// 3. Upload results from (1) to Cocoon /// /// The `resultsPath` is not available for all tests. When it doesn't show up, we /// need to append `CommitBranch`, `CommitSha`, and `BuilderName`. - Future sendTaskStatus({ + Future sendResultsPath({ String? resultsPath, bool? isTestFlaky, String? gitBranch, @@ -102,7 +102,8 @@ class Cocoon { resultsJson['NewStatus'] = testStatus; } resultsJson['TestFlaky'] = isTestFlaky ?? false; - if (_shouldUpdateCocoon(resultsJson)) { + const List supportedBranches = ['master']; + if (supportedBranches.contains(resultsJson['CommitBranch'])) { await retry( () async => _sendUpdateTaskRequest(resultsJson).timeout(Duration(seconds: requestTimeoutLimit)), retryIf: (Exception e) => e is SocketException || e is TimeoutException || e is ClientException, @@ -111,13 +112,6 @@ class Cocoon { } } - /// Only post-submit tests on `master` are allowed to update in cocoon. - bool _shouldUpdateCocoon(Map resultJson) { - const List supportedBranches = ['master']; - return supportedBranches.contains(resultJson['CommitBranch']) && - !resultJson['BuilderName'].toString().toLowerCase().contains('staging'); - } - /// Write the given parameters into an update task request and store the JSON in [resultsPath]. Future writeTaskResultToFile({ String? builderName, diff --git a/dev/devicelab/lib/framework/metrics_center.dart b/dev/devicelab/lib/framework/metrics_center.dart index c785e0357e..83bbf5d922 100644 --- a/dev/devicelab/lib/framework/metrics_center.dart +++ b/dev/devicelab/lib/framework/metrics_center.dart @@ -45,7 +45,6 @@ Future connectFlutterDestination() async { /// ] /// } List parse(Map resultsJson) { - print('Results to upload to skia perf: $resultsJson'); final List scoreKeys = (resultsJson['BenchmarkScoreKeys'] as List?)?.cast() ?? const []; final Map resultData = @@ -71,13 +70,8 @@ List parse(Map resultsJson) { return metricPoints; } -/// Upload JSON results to skia perf. -/// -/// Flutter infrastructure's workflow is: -/// 1. Run DeviceLab test, writing results to a known path -/// 2. Request service account token from luci auth (valid for at least 3 minutes) -/// 3. Upload results from (1) to skia perf. -Future uploadToSkiaPerf(String? resultsPath, String? commitTime) async { +/// Upload test metrics to metrics center. +Future uploadToMetricsCenter(String? resultsPath, String? commitTime) async { int commitTimeSinceEpoch; if (resultsPath == null) { return; diff --git a/dev/devicelab/test/cocoon_test.dart b/dev/devicelab/test/cocoon_test.dart index 097a28b185..92b2e33e18 100644 --- a/dev/devicelab/test/cocoon_test.dart +++ b/dev/devicelab/test/cocoon_test.dart @@ -132,7 +132,7 @@ void main() { '"ResultData":{},' '"BenchmarkScoreKeys":[]}'; fs.file(resultsPath).writeAsStringSync(updateTaskJson); - await cocoon.sendTaskStatus(resultsPath: resultsPath); + await cocoon.sendResultsPath(resultsPath: resultsPath); }); test('uploads expected update task payload from results file', () async { @@ -154,7 +154,7 @@ void main() { '"ResultData":{"i":0.0,"j":0.0,"not_a_metric":"something"},' '"BenchmarkScoreKeys":["i","j"]}'; fs.file(resultsPath).writeAsStringSync(updateTaskJson); - await cocoon.sendTaskStatus(resultsPath: resultsPath); + await cocoon.sendResultsPath(resultsPath: resultsPath); }); test('Verify retries for task result upload', () async { @@ -186,7 +186,7 @@ void main() { '"ResultData":{"i":0.0,"j":0.0,"not_a_metric":"something"},' '"BenchmarkScoreKeys":["i","j"]}'; fs.file(resultsPath).writeAsStringSync(updateTaskJson); - await cocoon.sendTaskStatus(resultsPath: resultsPath); + await cocoon.sendResultsPath(resultsPath: resultsPath); }); test('Verify timeout and retry for task result upload', () async { @@ -221,7 +221,7 @@ void main() { '"ResultData":{"i":0.0,"j":0.0,"not_a_metric":"something"},' '"BenchmarkScoreKeys":["i","j"]}'; fs.file(resultsPath).writeAsStringSync(updateTaskJson); - await cocoon.sendTaskStatus(resultsPath: resultsPath); + await cocoon.sendResultsPath(resultsPath: resultsPath); }); test('Verify timeout does not trigger for result upload', () async { @@ -256,7 +256,7 @@ void main() { '"ResultData":{"i":0.0,"j":0.0,"not_a_metric":"something"},' '"BenchmarkScoreKeys":["i","j"]}'; fs.file(resultsPath).writeAsStringSync(updateTaskJson); - await cocoon.sendTaskStatus(resultsPath: resultsPath); + await cocoon.sendResultsPath(resultsPath: resultsPath); }); test('Verify failure without retries for task result upload', () async { @@ -288,7 +288,7 @@ void main() { '"ResultData":{"i":0.0,"j":0.0,"not_a_metric":"something"},' '"BenchmarkScoreKeys":["i","j"]}'; fs.file(resultsPath).writeAsStringSync(updateTaskJson); - expect(() => cocoon.sendTaskStatus(resultsPath: resultsPath), throwsA(isA())); + expect(() => cocoon.sendResultsPath(resultsPath: resultsPath), throwsA(isA())); }); test('throws client exception on non-200 responses', () async { @@ -310,10 +310,10 @@ void main() { '"ResultData":{"i":0.0,"j":0.0,"not_a_metric":"something"},' '"BenchmarkScoreKeys":["i","j"]}'; fs.file(resultsPath).writeAsStringSync(updateTaskJson); - expect(() => cocoon.sendTaskStatus(resultsPath: resultsPath), throwsA(isA())); + expect(() => cocoon.sendResultsPath(resultsPath: resultsPath), throwsA(isA())); }); - test('does not update on non-supported branches', () async { + test('does not upload results on non-supported branches', () async { // Any network failure would cause the upoad to fail mockClient = MockClient((Request request) async => Response('', 500)); @@ -335,32 +335,7 @@ void main() { fs.file(resultsPath).writeAsStringSync(updateTaskJson); // This will fail if it decided to upload results - await cocoon.sendTaskStatus(resultsPath: resultsPath); - }); - - test('does not update for staging test', () async { - // Any network failure would cause the upoad to fail - mockClient = MockClient((Request request) async => Response('', 500)); - - cocoon = Cocoon( - serviceAccountTokenPath: serviceAccountTokenPath, - fs: fs, - httpClient: mockClient, - requestRetryLimit: 0, - ); - - const String resultsPath = 'results.json'; - const String updateTaskJson = '{' - '"CommitBranch":"master",' - '"CommitSha":"$commitSha",' - '"BuilderName":"Linux_staging_test",' - '"NewStatus":"Succeeded",' - '"ResultData":{"i":0.0,"j":0.0,"not_a_metric":"something"},' - '"BenchmarkScoreKeys":["i","j"]}'; - fs.file(resultsPath).writeAsStringSync(updateTaskJson); - - // This will fail if it decided to upload results - await cocoon.sendTaskStatus(resultsPath: resultsPath); + await cocoon.sendResultsPath(resultsPath: resultsPath); }); });