Skip to content

Commit b67e264

Browse files
authored
Revert "Skip staging test update to cocoon in test runner (flutter#88835)" (flutter#88971)
This reverts commit 884dfc2.
1 parent b0950fd commit b67e264

File tree

4 files changed

+29
-59
lines changed

4 files changed

+29
-59
lines changed

dev/devicelab/lib/command/upload_results.dart

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -41,14 +41,21 @@ class UploadResultsCommand extends Command<void> {
4141
final String? testStatus = argResults!['test-status'] as String?;
4242
final String? commitTime = argResults!['commit-time'] as String?;
4343

44-
// Upload metrics to skia perf from test runner when `resultsPath` is specified.
45-
if (resultsPath != null) {
46-
await uploadToSkiaPerf(resultsPath, commitTime);
47-
print('Successfully uploaded metrics to skia perf');
44+
// Upload metrics to metrics_center from test runner when `commitTime` is specified. This
45+
// is mainly for testing purpose.
46+
// The upload step will be skipped from cocoon once this is validated.
47+
// TODO(keyong): remove try block to block test when this is validated to work https://github.com/flutter/flutter/issues/88484
48+
try {
49+
if (commitTime != null) {
50+
await uploadToMetricsCenter(resultsPath, commitTime);
51+
print('Successfully uploaded metrics to metrics center');
52+
}
53+
} on Exception catch (e, stacktrace) {
54+
print('Uploading metrics failure: $e\n\n$stacktrace');
4855
}
4956

5057
final Cocoon cocoon = Cocoon(serviceAccountTokenPath: serviceAccountTokenFile);
51-
return cocoon.sendTaskStatus(
58+
return cocoon.sendResultsPath(
5259
resultsPath: resultsPath,
5360
isTestFlaky: testFlakyStatus == 'True',
5461
gitBranch: gitBranch,

dev/devicelab/lib/framework/cocoon.dart

Lines changed: 6 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -75,16 +75,16 @@ class Cocoon {
7575
return _commitSha = result.stdout as String;
7676
}
7777

78-
/// Update test status to Cocoon.
78+
/// Upload the JSON results in [resultsPath] to Cocoon.
7979
///
8080
/// Flutter infrastructure's workflow is:
81-
/// 1. Run DeviceLab test
81+
/// 1. Run DeviceLab test, writing results to a known path
8282
/// 2. Request service account token from luci auth (valid for at least 3 minutes)
83-
/// 3. Update test status from (1) to Cocoon
83+
/// 3. Upload results from (1) to Cocoon
8484
///
8585
/// The `resultsPath` is not available for all tests. When it doesn't show up, we
8686
/// need to append `CommitBranch`, `CommitSha`, and `BuilderName`.
87-
Future<void> sendTaskStatus({
87+
Future<void> sendResultsPath({
8888
String? resultsPath,
8989
bool? isTestFlaky,
9090
String? gitBranch,
@@ -102,7 +102,8 @@ class Cocoon {
102102
resultsJson['NewStatus'] = testStatus;
103103
}
104104
resultsJson['TestFlaky'] = isTestFlaky ?? false;
105-
if (_shouldUpdateCocoon(resultsJson)) {
105+
const List<String> supportedBranches = <String>['master'];
106+
if (supportedBranches.contains(resultsJson['CommitBranch'])) {
106107
await retry(
107108
() async => _sendUpdateTaskRequest(resultsJson).timeout(Duration(seconds: requestTimeoutLimit)),
108109
retryIf: (Exception e) => e is SocketException || e is TimeoutException || e is ClientException,
@@ -111,13 +112,6 @@ class Cocoon {
111112
}
112113
}
113114

114-
/// Only post-submit tests on `master` are allowed to update in cocoon.
115-
bool _shouldUpdateCocoon(Map<String, dynamic> resultJson) {
116-
const List<String> supportedBranches = <String>['master'];
117-
return supportedBranches.contains(resultJson['CommitBranch']) &&
118-
!resultJson['BuilderName'].toString().toLowerCase().contains('staging');
119-
}
120-
121115
/// Write the given parameters into an update task request and store the JSON in [resultsPath].
122116
Future<void> writeTaskResultToFile({
123117
String? builderName,

dev/devicelab/lib/framework/metrics_center.dart

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,6 @@ Future<FlutterDestination> connectFlutterDestination() async {
4545
/// ]
4646
/// }
4747
List<MetricPoint> parse(Map<String, dynamic> resultsJson) {
48-
print('Results to upload to skia perf: $resultsJson');
4948
final List<String> scoreKeys =
5049
(resultsJson['BenchmarkScoreKeys'] as List<dynamic>?)?.cast<String>() ?? const <String>[];
5150
final Map<String, dynamic> resultData =
@@ -71,13 +70,8 @@ List<MetricPoint> parse(Map<String, dynamic> resultsJson) {
7170
return metricPoints;
7271
}
7372

74-
/// Upload JSON results to skia perf.
75-
///
76-
/// Flutter infrastructure's workflow is:
77-
/// 1. Run DeviceLab test, writing results to a known path
78-
/// 2. Request service account token from luci auth (valid for at least 3 minutes)
79-
/// 3. Upload results from (1) to skia perf.
80-
Future<void> uploadToSkiaPerf(String? resultsPath, String? commitTime) async {
73+
/// Upload test metrics to metrics center.
74+
Future<void> uploadToMetricsCenter(String? resultsPath, String? commitTime) async {
8175
int commitTimeSinceEpoch;
8276
if (resultsPath == null) {
8377
return;

dev/devicelab/test/cocoon_test.dart

Lines changed: 9 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -132,7 +132,7 @@ void main() {
132132
'"ResultData":{},'
133133
'"BenchmarkScoreKeys":[]}';
134134
fs.file(resultsPath).writeAsStringSync(updateTaskJson);
135-
await cocoon.sendTaskStatus(resultsPath: resultsPath);
135+
await cocoon.sendResultsPath(resultsPath: resultsPath);
136136
});
137137

138138
test('uploads expected update task payload from results file', () async {
@@ -154,7 +154,7 @@ void main() {
154154
'"ResultData":{"i":0.0,"j":0.0,"not_a_metric":"something"},'
155155
'"BenchmarkScoreKeys":["i","j"]}';
156156
fs.file(resultsPath).writeAsStringSync(updateTaskJson);
157-
await cocoon.sendTaskStatus(resultsPath: resultsPath);
157+
await cocoon.sendResultsPath(resultsPath: resultsPath);
158158
});
159159

160160
test('Verify retries for task result upload', () async {
@@ -186,7 +186,7 @@ void main() {
186186
'"ResultData":{"i":0.0,"j":0.0,"not_a_metric":"something"},'
187187
'"BenchmarkScoreKeys":["i","j"]}';
188188
fs.file(resultsPath).writeAsStringSync(updateTaskJson);
189-
await cocoon.sendTaskStatus(resultsPath: resultsPath);
189+
await cocoon.sendResultsPath(resultsPath: resultsPath);
190190
});
191191

192192
test('Verify timeout and retry for task result upload', () async {
@@ -221,7 +221,7 @@ void main() {
221221
'"ResultData":{"i":0.0,"j":0.0,"not_a_metric":"something"},'
222222
'"BenchmarkScoreKeys":["i","j"]}';
223223
fs.file(resultsPath).writeAsStringSync(updateTaskJson);
224-
await cocoon.sendTaskStatus(resultsPath: resultsPath);
224+
await cocoon.sendResultsPath(resultsPath: resultsPath);
225225
});
226226

227227
test('Verify timeout does not trigger for result upload', () async {
@@ -256,7 +256,7 @@ void main() {
256256
'"ResultData":{"i":0.0,"j":0.0,"not_a_metric":"something"},'
257257
'"BenchmarkScoreKeys":["i","j"]}';
258258
fs.file(resultsPath).writeAsStringSync(updateTaskJson);
259-
await cocoon.sendTaskStatus(resultsPath: resultsPath);
259+
await cocoon.sendResultsPath(resultsPath: resultsPath);
260260
});
261261

262262
test('Verify failure without retries for task result upload', () async {
@@ -288,7 +288,7 @@ void main() {
288288
'"ResultData":{"i":0.0,"j":0.0,"not_a_metric":"something"},'
289289
'"BenchmarkScoreKeys":["i","j"]}';
290290
fs.file(resultsPath).writeAsStringSync(updateTaskJson);
291-
expect(() => cocoon.sendTaskStatus(resultsPath: resultsPath), throwsA(isA<ClientException>()));
291+
expect(() => cocoon.sendResultsPath(resultsPath: resultsPath), throwsA(isA<ClientException>()));
292292
});
293293

294294
test('throws client exception on non-200 responses', () async {
@@ -310,10 +310,10 @@ void main() {
310310
'"ResultData":{"i":0.0,"j":0.0,"not_a_metric":"something"},'
311311
'"BenchmarkScoreKeys":["i","j"]}';
312312
fs.file(resultsPath).writeAsStringSync(updateTaskJson);
313-
expect(() => cocoon.sendTaskStatus(resultsPath: resultsPath), throwsA(isA<ClientException>()));
313+
expect(() => cocoon.sendResultsPath(resultsPath: resultsPath), throwsA(isA<ClientException>()));
314314
});
315315

316-
test('does not update on non-supported branches', () async {
316+
test('does not upload results on non-supported branches', () async {
317317
// Any network failure would cause the upoad to fail
318318
mockClient = MockClient((Request request) async => Response('', 500));
319319

@@ -335,32 +335,7 @@ void main() {
335335
fs.file(resultsPath).writeAsStringSync(updateTaskJson);
336336

337337
// This will fail if it decided to upload results
338-
await cocoon.sendTaskStatus(resultsPath: resultsPath);
339-
});
340-
341-
test('does not update for staging test', () async {
342-
// Any network failure would cause the upoad to fail
343-
mockClient = MockClient((Request request) async => Response('', 500));
344-
345-
cocoon = Cocoon(
346-
serviceAccountTokenPath: serviceAccountTokenPath,
347-
fs: fs,
348-
httpClient: mockClient,
349-
requestRetryLimit: 0,
350-
);
351-
352-
const String resultsPath = 'results.json';
353-
const String updateTaskJson = '{'
354-
'"CommitBranch":"master",'
355-
'"CommitSha":"$commitSha",'
356-
'"BuilderName":"Linux_staging_test",'
357-
'"NewStatus":"Succeeded",'
358-
'"ResultData":{"i":0.0,"j":0.0,"not_a_metric":"something"},'
359-
'"BenchmarkScoreKeys":["i","j"]}';
360-
fs.file(resultsPath).writeAsStringSync(updateTaskJson);
361-
362-
// This will fail if it decided to upload results
363-
await cocoon.sendTaskStatus(resultsPath: resultsPath);
338+
await cocoon.sendResultsPath(resultsPath: resultsPath);
364339
});
365340
});
366341

0 commit comments

Comments
 (0)