Auto flake part1 #1260
Auto flake part1 #1260
Conversation
chunhtai
force-pushed
the
auto-flake-part1
branch
from
0d97753 to
1841b0b
Compare
keyonghan
left a comment
keyonghan
left a comment
There was a problem hiding this comment.
Nit: could we add tests for the newly added logics?
There was a problem hiding this comment.
sorry forgot to mention earlier, could we move corresponding logics to different services, like /lib/src/service/github_service.dart, or the bigquery.dart? The structure should be more clear and easier to follow that way. And it would be easier to add corresponding tests in separate locations.
Sorry missed the test.dart file. Tests look good to me. Thanks! |
|
Hi @keyonghan I factored out different services |
There was a problem hiding this comment.
The bringup property may have been added in other lines, rather than the line next to builder.
There was a problem hiding this comment.
Yes, this is a while loop and it only stop when it see the next builder: line
There was a problem hiding this comment.
IIRC ApiRequestHandler already catches error, we may not need the try catch block here.
There was a problem hiding this comment.
Nit: doc for the table schema will help understand the logic.
There was a problem hiding this comment.
Which table schema are you referring to? flutter-dashboard.datasite.luci_prod_build_status is the table the sql query from, or do you mean the schema of the sql result? Either way, These schemas are abstracted away because I return a dart class instead.
There was a problem hiding this comment.
Yeah, I meant the table luci_prod_build_status result. From the code here, it is not clear what f[0]-f[6] are referring to. Since the table is located in bigquery (internally?), it may be useful to list the basic schema here.
There was a problem hiding this comment.
Do we need to distinguish the two cases: exceeding slo and the top one below slo?
There was a problem hiding this comment.
yes, the issueSummary getter does that.
There was a problem hiding this comment.
I took a look at the original listPullRequests code, and looks like it does the same thing of what github.pullRequests.list is doing underneath. So I swap it out
There was a problem hiding this comment.
Just recalled that for shard tests that contain thousands of unit tests, we may want to skip the step to create a PR. Test owner or the tree gardener will need to look into the flaky test and determine which sub test causes the flake, and then they need to create a PR to disable the sub test instead of marking the whole shard test suite as flaky.
From the auto process side, it will be challenging to find out the sub test and disable it based on current test framework.
There was a problem hiding this comment.
is there a way to determine which tests are shard tests?
There was a problem hiding this comment.
most ones here: https://github.com/flutter/flutter/blob/master/TESTOWNERS#L198
build_tests @zanderso @flutter/tool
framework_tests @HansMuller @flutter/framework
tool_integration_tests @zanderso @flutter/tool
tool_tests @zanderso @flutter/tool
web_integration_tests @ferhatb @flutter/web
web_long_running_tests @ferhatb @flutter/web
web_tests @ferhatb @flutter/web
web_tool_tests @zanderso @flutter/tool
Each of them contains several subshards, which are mapped to the builder name in .ci.yaml. So we will check those subshards flakiness and only file flaky issues rather than create a PR to mark them flaky. (each subshard contains tons of unit tests).
chunhtai
force-pushed
the
auto-flake-part1
branch
2 times, most recently
from
e004843 to
1c46192
Compare
There was a problem hiding this comment.
I guess a pr should also be filed for framework host only tests. The only exception is the shard tests, which contains multiple sub unit tests.
chunhtai
force-pushed
the
auto-flake-part1
branch
from
608cd1d to
c5e0020
Compare
chunhtai
added
the
waiting for tree to go green
3 participants
Auto create issues if there is not already one or has closed one longer than 15 days.
create PR if the test is not already marked flaky and no existing pr to mark flaky