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
3 changes: 3 additions & 0 deletions app_flutter/lib/main.dart
Original file line number Diff line number Diff line change
Expand Up @@ -40,9 +40,12 @@ class _BuildDashboardPageState extends State<BuildDashboardPage> {

@override
Widget build(BuildContext context) {
final ThemeData theme = Theme.of(context);
return Scaffold(
appBar: AppBar(
title: Text('Flutter Build Dashboard v2'),
backgroundColor:
buildState.isTreeBuilding ? theme.primaryColor : theme.errorColor,

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.

In addition to "Succeeded" and "Failed", it's useful to know how outdated is the information.

),
body: Column(
children: [
Expand Down
42 changes: 42 additions & 0 deletions app_flutter/lib/service/appengine_cocoon.dart
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,48 @@ class AppEngineCocoonService implements CocoonService {
return _commitStatusesFromJson(jsonResponse['Statuses']);
}

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

if (response.statusCode != HttpStatus.ok) {
throw HttpException(
'$_baseApiUrl/public/build-status returned ${response.statusCode}');
}

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

if (!_isBuildStatusResponseValid(jsonResponse)) {

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.

Better log this response, otherwise when we see the exception, we don't know why/how it's malformed.

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.

This will print the error to a local console. Should we think about implementing a logging system such as Firebase Crashlytics so users don't have to report these 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.

The issue with logging to console is that the #1 usecase for this will be the TVs in the immediate aread.

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.

Not only the TV, I always have the build dashboard open on my chrome, so it would be nice to know the errors when they happen.

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.

When errors happen on the TV, we just need to know an error is occurring. We do not need to know what the error is. The only time we would want to know what the error is on the TV is when we can't reproduce it on any other device. However, in that case the issue is most likely with the TV and not the application.

Thus, showing a generic error UI would indicate that someone needs to check what is going on by opening the dashboard on their computer to investigate further.

throw HttpException(
'$_baseApiUrl/public/build-status had a malformed response');
}

return jsonResponse['AnticipatedBuildStatus'] == "Succeeded";
}

/// Check if [Map<String,Object>] follows the format for build-status.
///
/// Template of the response it should receive:
/// ```json
/// {
/// "AnticipatedBuildStatus": "Succeeded"|"Failed"
/// }
/// ```
bool _isBuildStatusResponseValid(Map<String, Object> response) {
if (!response.containsKey('AnticipatedBuildStatus')) {
return false;
}

String treeBuildStatus = response['AnticipatedBuildStatus'];
if (treeBuildStatus != 'Failed' && treeBuildStatus != 'Succeeded') {
return false;
}

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.

Nit: it's more readable to write this in a positive way:

if (treeBuildStatus == "Succeeded" || treeBuildStatus == "Failed") {
  return true;
}

return false;

"In general checking the presence or absence of a positive is simpler for readers to understand than checking the presence or absence of a negative."

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.

For future reference: Testing on the Toilet #530: Improve Readability with Positive Booleans

The reason I chose this way was to have cases that returned false next to each other, instead of case 1 returns false, case 2 returns true, final case returns false. I wanted to try and reduce the cognitive switching on the return value.


return true;
}

List<CommitStatus> _commitStatusesFromJson(List<Object> jsonCommitStatuses) {
assert(jsonCommitStatuses != null);
// TODO(chillers): Remove adapter code to just use proto fromJson method. https://github.com/flutter/cocoon/issues/441
Expand Down
3 changes: 3 additions & 0 deletions app_flutter/lib/service/cocoon.dart
Original file line number Diff line number Diff line change
Expand Up @@ -31,4 +31,7 @@ abstract class CocoonService {
///
/// TODO(chillers): Make configurable to get range of commits
Future<List<CommitStatus>> fetchCommitStatuses();

/// Gets the current build status of flutter/flutter.
Future<bool> fetchTreeBuildStatus();
}
5 changes: 5 additions & 0 deletions app_flutter/lib/service/fake_cocoon.dart
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,11 @@ class FakeCocoonService implements CocoonService {
return Future.value(_createFakeCommitStatuses());
}

@override
Future<bool> fetchTreeBuildStatus() async {
return true;
}

List<CommitStatus> _createFakeCommitStatuses() {
List<CommitStatus> stats = <CommitStatus>[];

Expand Down
17 changes: 16 additions & 1 deletion app_flutter/lib/state/flutter_build.dart
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,10 @@ class FlutterBuildState extends ChangeNotifier {
/// The current status of the commits loaded.
List<CommitStatus> statuses = [];

/// Whether or not flutter/flutter currently passes tests.
bool get isTreeBuilding => _isTreeBuilding;
bool _isTreeBuilding = true;

/// Creates a new [FlutterBuildState].
///
/// If [CocoonService] is not specified, a new [CocoonService] instance is created.
Expand All @@ -39,13 +43,24 @@ class FlutterBuildState extends ChangeNotifier {
return;
}

/// [Timer.periodic] does not necessarily run at the start of the timer.
_fetchBuildStatusUpdate();

refreshTimer =
Timer.periodic(refreshRate, (t) => _fetchBuildStatusUpdate());
}

/// Request the latest [statuses] from [CocoonService].
void _fetchBuildStatusUpdate() async {
statuses = await _cocoonService.fetchCommitStatuses();
await Future.wait([
_cocoonService
.fetchCommitStatuses()
.then((commitStatuses) => statuses = commitStatuses),
_cocoonService
.fetchTreeBuildStatus()
.then((treeStatus) => _isTreeBuilding = treeStatus),
]);

notifyListeners();
}

Expand Down
58 changes: 57 additions & 1 deletion app_flutter/test/service/appengine_cocoon_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -63,8 +63,20 @@ const String jsonGetStatsResponse = """
}
""";

const String jsonBuildStatusTrueResponse = """
{
"AnticipatedBuildStatus": "Succeeded"
}
""";

const String jsonBuildStatusFalseResponse = """
{
"AnticipatedBuildStatus": "Failed"
}
""";

void main() {
group('AppEngine CocoonService', () {
group('AppEngine CocoonService fetchCommitStatus ', () {
AppEngineCocoonService service;

setUp(() async {
Expand Down Expand Up @@ -123,4 +135,48 @@ void main() {
expect(service.fetchCommitStatuses(), throwsException);
});
});

group('AppEngine CocoonService fetchTreeBuildStatus ', () {
AppEngineCocoonService service;

setUp(() async {
service = AppEngineCocoonService(client: MockClient((request) async {
return Response(jsonBuildStatusTrueResponse, 200);
}));
});

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

test('should return true when given Succeeded', () async {
bool treeBuildStatus = await service.fetchTreeBuildStatus();

expect(treeBuildStatus, true);
});

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

bool treeBuildStatus = await service.fetchTreeBuildStatus();

expect(treeBuildStatus, false);
});

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

expect(service.fetchTreeBuildStatus(), throwsException);
});

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

expect(service.fetchTreeBuildStatus(), throwsException);
});
});
}
20 changes: 14 additions & 6 deletions app_flutter/test/state/flutter_build_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -18,27 +18,31 @@ void main() {
setUp(() {
mockService = MockCocoonService();
buildState = FlutterBuildState(cocoonService: mockService);

when(mockService.fetchCommitStatuses())
.thenAnswer((_) => Future.value([]));
when(mockService.fetchTreeBuildStatus())
.thenAnswer((_) => Future.value(true));
});

testWidgets('timer should periodically fetch updates',
(WidgetTester tester) async {
buildState.startFetchingBuildStateUpdates();
verifyZeroInteractions(mockService);

// startFetching immediately starts fetching results
verify(mockService.fetchCommitStatuses()).called(1);

// Periodic timers don't necessarily run at the same time in each interval.
// We double the refreshRate to gurantee a call would have been made.
await tester.pump(buildState.refreshRate * 2);
verify(mockService.fetchCommitStatuses()).called(greaterThan(0));

await tester.pump(buildState.refreshRate * 2);
// Now we check to see if another call had been made
verify(mockService.fetchCommitStatuses()).called(greaterThan(1));

// Tear down fails to cancel the timer before the test is over
buildState.dispose();
});

test('multiple start updates should not change the timer', () {
testWidgets('multiple start updates should not change the timer',
(WidgetTester tester) async {
buildState.startFetchingBuildStateUpdates();
Timer refreshTimer = buildState.refreshTimer;

Expand All @@ -47,6 +51,10 @@ void main() {

expect(refreshTimer, equals(buildState.refreshTimer));

// Since startFetching sends out requests on start, we need to wait
// for them to finish before disposing of the state.
await tester.pumpAndSettle();

// Tear down fails to cancel the timer before the test is over
buildState.dispose();
});
Expand Down