Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions app_flutter/lib/build_dashboard.dart
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,9 @@ class BuildDashboard extends StatelessWidget {
builder: (_, FlutterBuildState buildState, Widget child) => Scaffold(
appBar: AppBar(
title: const Text('Flutter Build Dashboard v2'),
backgroundColor:
buildState.isTreeBuilding ? theme.primaryColor : theme.errorColor,
backgroundColor: buildState.isTreeBuilding.data
? theme.primaryColor
: theme.errorColor,
),
body: Column(
children: const <Widget>[
Expand Down
41 changes: 28 additions & 13 deletions app_flutter/lib/service/appengine_cocoon.dart
Original file line number Diff line number Diff line change
Expand Up @@ -31,40 +31,55 @@ class AppEngineCocoonService implements CocoonService {
final http.Client _client;

@override
Future<List<CommitStatus>> fetchCommitStatuses() async {
Future<CocoonResponse<List<CommitStatus>>> fetchCommitStatuses() async {
/// This endpoint returns JSON [List<Agent>, List<CommitStatus>]
final http.Response response =
await _client.get('$_baseApiUrl/public/get-status');

if (response.statusCode != HttpStatus.ok) {
throw HttpException(
'$_baseApiUrl/public/get-status returned ${response.statusCode}');
print(response.body);
return CocoonResponse<List<CommitStatus>>()
..error =
'$_baseApiUrl/public/get-status returned ${response.statusCode}';
}

final Map<String, Object> jsonResponse = jsonDecode(response.body);

return _commitStatusesFromJson(jsonResponse['Statuses']);
try {
final Map<String, Object> jsonResponse = jsonDecode(response.body);
return CocoonResponse<List<CommitStatus>>()
..data = _commitStatusesFromJson(jsonResponse['Statuses']);
} catch (error) {
return CocoonResponse<List<CommitStatus>>()..error = error.toString();
}
}

@override
Future<bool> fetchTreeBuildStatus() async {
Future<CocoonResponse<bool>> fetchTreeBuildStatus() async {
/// This endpoint returns JSON {AnticipatedBuildStatus: [BuildStatus]}
final http.Response response =
await _client.get('$_baseApiUrl/public/build-status');

if (response.statusCode != HttpStatus.ok) {
throw HttpException(
'$_baseApiUrl/public/build-status returned ${response.statusCode}');
print(response.body);
return CocoonResponse<bool>()
..error =
'$_baseApiUrl/public/build-status returned ${response.statusCode}';
}

final Map<String, Object> jsonResponse = jsonDecode(response.body);
Map<String, Object> jsonResponse;
try {
jsonResponse = jsonDecode(response.body);
} catch (error) {
return CocoonResponse<bool>()
..error = '$_baseApiUrl/public/build-status had a malformed response';
}

if (!_isBuildStatusResponseValid(jsonResponse)) {
throw const HttpException(
'$_baseApiUrl/public/build-status had a malformed response');
return CocoonResponse<bool>()
..error = '$_baseApiUrl/public/build-status had a malformed response';
}

return jsonResponse['AnticipatedBuildStatus'] == 'Succeeded';
return CocoonResponse<bool>()
..data = jsonResponse['AnticipatedBuildStatus'] == 'Succeeded';
}

/// Check if [Map<String,Object>] follows the format for build-status.
Expand Down
15 changes: 13 additions & 2 deletions app_flutter/lib/service/cocoon.dart
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,19 @@ abstract class CocoonService {
/// Gets build information from the last 200 commits.
///
// TODO(chillers): Make configurable to get range of commits, https://github.com/flutter/cocoon/issues/458
Future<List<CommitStatus>> fetchCommitStatuses();
Future<CocoonResponse<List<CommitStatus>>> fetchCommitStatuses();

/// Gets the current build status of flutter/flutter.
Future<bool> fetchTreeBuildStatus();
Future<CocoonResponse<bool>> fetchTreeBuildStatus();
}

/// Wrapper class for data this state serves.
///
/// Holds [data] and possible error information.
class CocoonResponse<T> {
/// The data that gets used from [CocoonService].
T data;

/// Error information that can be used for debugging.
String error;
}
9 changes: 5 additions & 4 deletions app_flutter/lib/service/fake_cocoon.dart
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,14 @@ class FakeCocoonService implements CocoonService {
final Random random;

@override
Future<List<CommitStatus>> fetchCommitStatuses() async {
return _createFakeCommitStatuses();
Future<CocoonResponse<List<CommitStatus>>> fetchCommitStatuses() async {
return CocoonResponse<List<CommitStatus>>()
..data = _createFakeCommitStatuses();
}

@override
Future<bool> fetchTreeBuildStatus() async {
return random.nextBool();
Future<CocoonResponse<bool>> fetchTreeBuildStatus() async {
return CocoonResponse<bool>()..data = random.nextBool();
}

List<CommitStatus> _createFakeCommitStatuses() {
Expand Down
22 changes: 14 additions & 8 deletions app_flutter/lib/state/flutter_build.dart
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,16 @@ class FlutterBuildState extends ChangeNotifier {
Timer refreshTimer;

/// The current status of the commits loaded.
List<CommitStatus> statuses = <CommitStatus>[];
CocoonResponse<List<CommitStatus>> _statuses =
CocoonResponse<List<CommitStatus>>()..data = <CommitStatus>[];
CocoonResponse<List<CommitStatus>> get statuses => _statuses;

/// Whether or not flutter/flutter currently passes tests.
bool get isTreeBuilding => _isTreeBuilding;
bool _isTreeBuilding = true;
CocoonResponse<bool> _isTreeBuilding = CocoonResponse<bool>()..data = false;
CocoonResponse<bool> get isTreeBuilding => _isTreeBuilding;

/// Whether an error occured getting the latest data for the fields of this state.
bool get hasError => _isTreeBuilding.error != null || _statuses.error != null;

/// Start a fixed interval loop that fetches build state updates based on [refreshRate].
Future<void> startFetchingBuildStateUpdates() async {
Expand All @@ -50,14 +55,15 @@ class FlutterBuildState extends ChangeNotifier {
Timer.periodic(refreshRate, (_) => _fetchBuildStatusUpdate());
}

/// Request the latest [statuses] from [CocoonService].
/// Request the latest [statuses] and [isTreeBuilding] from [CocoonService].
Future<void> _fetchBuildStatusUpdate() async {
await Future.wait<void>(<Future<void>>[
_cocoonService.fetchCommitStatuses().then<List<CommitStatus>>(
(List<CommitStatus> commitStatuses) => statuses = commitStatuses),
await Future.wait(<Future<void>>[
_cocoonService.fetchCommitStatuses().then(

@jonahwilliams jonahwilliams Oct 7, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the _cocoonService be responsible for determining what is and isn't an error?

EDIT: server -> service

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Each implementation of CocoonService has to decide what is an error and what isn't an error. I don't think that can be done in FlutterBuildState where it can take in different implementations of CocoonService. This might lead to over generalizing what an error is.

Since the logic behind handling errors is the same, I think we should handle this in FlutterBuildState to just write this error handling code once instead of in each CocoonService implementation.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the example code here, the _cocoonService is already deciding what is and isn't an error by throwing vs returning normally.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the CocoonService should determine what is an isn't an error so FlutterBuildState doesn't need any extra logic handling errors.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not return a CocoonResponse object instead?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

This makes more sense since it simplifies FlutterBuildState to just hold data. No extra responsibilities :)

Thanks for the suggestion!

(CocoonResponse<List<CommitStatus>> response) =>
_statuses = response),
_cocoonService
.fetchTreeBuildStatus()
.then<bool>((bool treeStatus) => _isTreeBuilding = treeStatus),
.then((CocoonResponse<bool> response) => _isTreeBuilding = response),
]);

notifyListeners();
Expand Down
13 changes: 12 additions & 1 deletion app_flutter/lib/status_grid.dart
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,22 @@ import 'task_icon.dart';
class StatusGridContainer extends StatelessWidget {
const StatusGridContainer({Key key}) : super(key: key);

@visibleForTesting
static const String errorCocoonBackend = 'Cocoon Backend is having issues';

@override
Widget build(BuildContext context) {
return Consumer<FlutterBuildState>(
builder: (_, FlutterBuildState buildState, Widget child) {
final List<CommitStatus> statuses = buildState.statuses;
final List<CommitStatus> statuses = buildState.statuses.data;

if (buildState.hasError) {
print('FlutterBuildState has an error');
print('isTreeBuilding: ${buildState.isTreeBuilding.error}');
print('statuses: ${buildState.statuses.error}');

// TODO(chillers): Display the error
}

// Assume if there is no data that it is loading.
if (statuses.isEmpty) {
Expand Down
58 changes: 36 additions & 22 deletions app_flutter/test/service/appengine_cocoon_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,12 @@ import 'package:http/http.dart' show Request, Response;
import 'package:http/testing.dart';
import 'package:test/test.dart';

import 'package:app_flutter/service/appengine_cocoon.dart';
import 'package:cocoon_service/protos.dart'
show Commit, CommitStatus, Stage, Task;

import 'package:app_flutter/service/appengine_cocoon.dart';
import 'package:app_flutter/service/cocoon.dart';

// This is based off data the Cocoon backend sends out from v1.
// It doesn't map directly to protos since the backend does
// not use protos yet.
Expand Down Expand Up @@ -86,13 +88,14 @@ void main() {
}));
});

test('should return List<CommitStatus>', () {
test('should return CocoonResponse<List<CommitStatus>>', () {
expect(service.fetchCommitStatuses(),
const TypeMatcher<Future<List<CommitStatus>>>());
const TypeMatcher<Future<CocoonResponse<List<CommitStatus>>>>());
});

test('should return expected List<CommitStatus>', () async {
final List<CommitStatus> statuses = await service.fetchCommitStatuses();
final CocoonResponse<List<CommitStatus>> statuses =
await service.fetchCommitStatuses();

final CommitStatus expectedStatus = CommitStatus()
..commit = (Commit()
Expand All @@ -118,22 +121,26 @@ void main() {
..stageName = 'devicelab'
..status = 'Succeeded'));

expect(statuses.length, 1);
expect(statuses.first, expectedStatus);
expect(statuses.data.length, 1);
expect(statuses.data.first, expectedStatus);
});

test('should throw exception if given non-200 response', () {
test('should have error if given non-200 response', () async {
service = AppEngineCocoonService(
client: MockClient((Request request) async => Response('', 404)));

expect(service.fetchCommitStatuses(), throwsException);
final CocoonResponse<List<CommitStatus>> response =
await service.fetchCommitStatuses();
expect(response.error, isNotNull);
});

test('should throw exception if given bad response', () {
test('should have error if given bad response', () async {
service = AppEngineCocoonService(
client: MockClient((Request request) async => Response('bad', 200)));

expect(service.fetchCommitStatuses(), throwsException);
final CocoonResponse<List<CommitStatus>> response =
await service.fetchCommitStatuses();
expect(response.error, isNotNull);
});
});

Expand All @@ -147,39 +154,46 @@ void main() {
}));
});

test('should return bool', () {
expect(service.fetchTreeBuildStatus(), const TypeMatcher<Future<bool>>());
test('should return CocoonResponse<bool>', () {
expect(service.fetchTreeBuildStatus(),
const TypeMatcher<Future<CocoonResponse<bool>>>());
});

test('should return true when given Succeeded', () async {
final bool treeBuildStatus = await service.fetchTreeBuildStatus();
test('data should be true when given Succeeded', () async {
final CocoonResponse<bool> treeBuildStatus =
await service.fetchTreeBuildStatus();

expect(treeBuildStatus, true);
expect(treeBuildStatus.data, true);
});

test('should return false when given Failed', () async {
test('data should be false when given Failed', () async {
service =
AppEngineCocoonService(client: MockClient((Request request) async {
return Response(jsonBuildStatusFalseResponse, 200);
}));

final bool treeBuildStatus = await service.fetchTreeBuildStatus();
final CocoonResponse<bool> treeBuildStatus =
await service.fetchTreeBuildStatus();

expect(treeBuildStatus, false);
expect(treeBuildStatus.data, false);
});

test('should throw exception if given non-200 response', () {
test('should have error if given non-200 response', () async {
service = AppEngineCocoonService(
client: MockClient((Request request) async => Response('', 404)));

expect(service.fetchTreeBuildStatus(), throwsException);
final CocoonResponse<bool> response =
await service.fetchTreeBuildStatus();
expect(response.error, isNotNull);
});

test('should throw exception if given bad response', () {
test('should have error if given bad response', () async {
service = AppEngineCocoonService(
client: MockClient((Request request) async => Response('bad', 200)));

expect(service.fetchTreeBuildStatus(), throwsException);
final CocoonResponse<bool> response =
await service.fetchTreeBuildStatus();
expect(response.error, isNotNull);
});
});
}
11 changes: 7 additions & 4 deletions app_flutter/test/state/flutter_build_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import 'package:mockito/mockito.dart';

import 'package:cocoon_service/protos.dart' show CommitStatus;

import 'package:app_flutter/service/cocoon.dart';
import 'package:app_flutter/service/fake_cocoon.dart';
import 'package:app_flutter/state/flutter_build.dart';

Expand All @@ -21,10 +22,12 @@ void main() {
mockService = MockCocoonService();
buildState = FlutterBuildState(cocoonService: mockService);

when(mockService.fetchCommitStatuses()).thenAnswer(
(_) => Future<List<CommitStatus>>.value(<CommitStatus>[]));
when(mockService.fetchTreeBuildStatus())
.thenAnswer((_) => Future<bool>.value(true));
when(mockService.fetchCommitStatuses()).thenAnswer((_) =>
Future<CocoonResponse<List<CommitStatus>>>.value(
CocoonResponse<List<CommitStatus>>()..data = <CommitStatus>[]));
when(mockService.fetchTreeBuildStatus()).thenAnswer((_) =>
Future<CocoonResponse<bool>>.value(
CocoonResponse<bool>()..data = true));
});

testWidgets('timer should periodically fetch updates',
Expand Down
5 changes: 4 additions & 1 deletion app_flutter/test/status_grid_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import 'package:provider/provider.dart';

import 'package:cocoon_service/protos.dart' show CommitStatus, Task;

import 'package:app_flutter/service/cocoon.dart';
import 'package:app_flutter/service/fake_cocoon.dart';
import 'package:app_flutter/state/flutter_build.dart';
import 'package:app_flutter/commit_box.dart';
Expand All @@ -20,7 +21,9 @@ void main() {

setUpAll(() async {
final FakeCocoonService service = FakeCocoonService();
statuses = await service.fetchCommitStatuses();
final CocoonResponse<List<CommitStatus>> response =
await service.fetchCommitStatuses();
statuses = response.data;
});

testWidgets('shows loading indicator when statuses is empty',
Expand Down