Support test ownership validation #1301
Conversation
|
One thing I am worry about this approach is that is may be a pain to migrate TESTOWNERS format, also one can break the presubmit by changing the script without knowing. Otherwise it looks good |
That's true, but I don't think we have a short plan to finish ownership info for shards tests or any big format migration. On the other hand, we have seen people adding tests without adding ownership over time, which causes pain to the infra team. The main reason I put it under cocoon is we can extend the functionality to support other repos in the future. One way to avoid any unknown breakage is put it under For now, I added more unit tests for other types to cover all scenarios to try avoiding breakage change from cocoon side without knowing. We need to revisit the logic when we have ownership for shard tests, or when we expand the logic to other repos. |
CaseyHillers
left a comment
There was a problem hiding this comment.
+1 to Chun-Heng's comment about testability. You should add an integration test that ensures this is testing the tip of tree to prevent breakages landing here
| Future<List<String>> remoteCheck(String repo, String ref) async { | ||
| final HttpClient client = HttpClient(); | ||
| final Uri ciYamlUrl = Uri.https('raw.githubusercontent.com', 'flutter/$repo/$ref/.ci.yaml'); | ||
| final HttpClientRequest ciYamlRequest = await client.getUrl(ciYamlUrl); |
There was a problem hiding this comment.
Some of the other bin scripts get ciYaml from a network request. Should we refactor this into a utility so they can all share that logic?
There was a problem hiding this comment.
Used existing utils function instead. Also updated bing/generate_jspb.dart.
+1 to Chun-Heng's comment about testability. You should add an integration test that ensures this is testing the tip of tree to prevent breakages landing here
https://flutter-review.googlesource.com/c/recipes/+/16300 is ready to accept trigger from cocoon. Will create a new builder once the recipe cl is in.
There was a problem hiding this comment.
Didn't know we have integration tests here. Just created one instead.
| import 'dart:convert'; | ||
| import 'dart:io'; | ||
|
|
||
| import 'package:cocoon_service/src/request_handlers/flaky_handler_utils.dart'; |
There was a problem hiding this comment.
Should we move some the getTestowner code out to a more generic util file?
There was a problem hiding this comment.
I would prefer to do that in a separate PR.
|
|
||
| import '../../protos.dart'; | ||
| import '../foundation/typedefs.dart'; | ||
| import '../request_handlers/flaky_handler_utils.dart'; |
There was a problem hiding this comment.
This is weird dependency, I will probably move the methods here
There was a problem hiding this comment.
Yeah, I know, but don't wanna to make this PR too complex.
Created flutter/flutter#87287 to refactor.
| } | ||
| break; | ||
| } | ||
| case BuilderType.firebaselab: |
There was a problem hiding this comment.
not related to this pr, Do we want to file mark flaky pr for firebaselab test?
There was a problem hiding this comment.
I am not sure about that, but i guess we still want to have ownership for those tests.
|
|
||
| for (final String line in lines) { | ||
| final List<String> words = line.trim().split(' '); | ||
| // e.g. words = ['/xxx/xxx/xxx_test.dart', '@stuartmorgan' '@flutter/tool'] |
There was a problem hiding this comment.
nit: This does not seem like a good example
There was a problem hiding this comment.
Removed as this is not necessary here.
This PR uses cocoon's existing functionality to obtain ownership data based on builder configs defined in .ci.yaml, and creates a support to validate test ownership.
This PR also adds support for
Firebaselab tests.A companion recipe change: https://flutter-review.googlesource.com/c/recipes/+/16300.
This is part of flutter/flutter#87144