Support branching for refresh chromebot status #720
Conversation
CaseyHillers
left a comment
CaseyHillers
left a comment
There was a problem hiding this comment.
LGTM! Just some nits
| } | ||
| await transaction.commit(); | ||
| } catch (error) { | ||
| rethrow; |
There was a problem hiding this comment.
Is the try/catch necessary if it's just being rethrown with no additional logic?
There was a problem hiding this comment.
This is some existing logic there without further changes. I just changed to catch/log the error info.
There was a problem hiding this comment.
There was a problem hiding this comment.
Ha, removed the try catch!
| return Body.empty; | ||
| } | ||
|
|
||
| Future<void> _updateStatus( |
|
|
||
| import 'package:cocoon_service/src/model/appengine/commit.dart'; | ||
| import 'package:cocoon_service/src/model/appengine/task.dart'; | ||
| import 'package:cocoon_service/src/request_handlers/refresh_chromebot_status.dart'; |
There was a problem hiding this comment.
nitty nit: these package:cocoon imports should be between the import package blocks and the relative imports for the test files block
| // ignore: must_be_immutable | ||
| class MockLuciService extends Mock implements LuciService {} | ||
|
|
||
| class MockGitHub extends Mock implements GitHub {} |
There was a problem hiding this comment.
These mocks could also be moved to the test utils file to remove the need for creating them in each file
There was a problem hiding this comment.
Will be included in flutter/flutter#53280.
| List<String> githubBranches; | ||
| FakeHttpClient branchHttpClient; | ||
|
|
||
| Stream<Branch> branchStream() async* { |
There was a problem hiding this comment.
Branch stream seems to be used through the tests. We should move it to a test utils file to remove the duplication through the codebase.
There was a problem hiding this comment.
This is a good point. At least five APIs are using this code. Created a TODO to simply test codes, and prefer to work on it in a separate PR.
2 participants
This is part of flutter/flutter#51807