Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Gardening: mark flaky tests flaky. #82754

Merged
merged 5 commits into from
May 18, 2021

Conversation

gspencergoog
Copy link
Contributor

@gspencergoog gspencergoog commented May 17, 2021

This marks the following tests as flaky, per the gardener rotation:

@flutter-dashboard flutter-dashboard bot added the team Infra upgrades, team productivity, code health, technical debt. See also team: labels. label May 17, 2021
@google-cla google-cla bot added the cla: yes label May 17, 2021
@zanderso
Copy link
Member

Please cross-reference the individual issues for the flaky tests in the PR description so that github will add links, and link to the individual issues for the flaky tests in prod_builders.json.

@keyonghan
Copy link
Contributor

For the tests exceeding 2%, the proposal is to remove them out of the dashboard. They need to go through the staging env. for validation before enabled back: https://github.com/flutter/flutter/wiki/Reducing-Test-Flakiness#fixing-flaky-tests

@gaaclarke
Copy link
Member

Can I get a link to a failure for linux_platform_channels_benchmarks? I want to verify that the issue was intrinsic to the actual test. FWIW that test has been running for a week so you might not have an accurate flake percentage for it. A single failure from before the test was marked not flakey might be giving you a reading with that threshold.

@zanderso
Copy link
Member

@keyonghan If "remove them out of the dashboard" means that they won't run at all, then I don't think that's the right thing to do. Especially since these are benchmarks, removing them entirely will mean that we will miss regressions.

@keyonghan
Copy link
Contributor

@keyonghan If "remove them out of the dashboard" means that they won't run at all, then I don't think that's the right thing to do. Especially since these are benchmarks, removing them entirely will mean that we will miss regressions.

"remove them out of the dashboard" means they will not show up in the flutter build dashboard, but they will still be triggered and run. Milo dashboard will still show the build info.

@gspencergoog
Copy link
Contributor Author

I've removed the entries (and so didn't add the bug cross-references, since there's nothing left to connect them to), but I'll wait to commit this until you LGTM this updated PR again, @zanderso.

@gspencergoog gspencergoog changed the title Mark flaky tests flaky Gardening: remove flaky tests. May 17, 2021
@zanderso
Copy link
Member

I think we need to leave these on the dashboard so that we have a visual reminder of what is marked flaky.

@keyonghan
Copy link
Contributor

I think we need to leave these on the dashboard so that we have a visual reminder of what is marked flaky.

I guess our intention is to unblock the tree and make sure the test passes 50 successful runs in staging pool before enabled back.
I guess your suggestion here is okay just by enforcing the validation in prod pool instead of staging pool.

We can start this way, and adjust the process if needed. I will update the doc accordingly.

@gspencergoog
Copy link
Contributor Author

OK, done. Since this is pure jSON (no comments), I added an extra field called "issue_url" to the flaky tests, that will hopefully be ignored. If not, well, I'll revert it.

@gspencergoog gspencergoog changed the title Gardening: remove flaky tests. Gardening: mark flaky tests flaky. May 17, 2021
@@ -82,7 +82,8 @@
"name": "Linux complex_layout_scroll_perf__devtools_memory",
"repo": "flutter",
"task_name": "linux_complex_layout_scroll_perf__devtools_memory",
"flaky": false
"flaky": true,
"issue_url": "https://github.com/flutter/flutter/issues/82741"
Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't announced it yet, but I'm aiming to deprecate this JSON config Wednesday night in favor of the .ci.yaml. Just commenting here as I'll need to move these to comments in the migration PR (@CaseyHillers @christopherfujino )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think you just announced it. :-)

Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose new properties could be naturally supported in .ci.yaml?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, PRs welcome if you want to add automation around this

Copy link
Member

Choose a reason for hiding this comment

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

I suppose new properties could be naturally supported in .ci.yaml?

You would have to update the protobuf in flutter/cocoon first. I actually see this as a feature, as if you add a new target and mispell a field, CI will fail on an unknown field, rather than silently ignoring it.

@gspencergoog
Copy link
Contributor Author

Sorry, @zanderso, I can't add extra fields or comments to the JSON, so I had to remove the cross-linking for the issues.

@CaseyHillers might be able to add a field for that into .ci.yaml for this (seems useful in any case).

@gspencergoog
Copy link
Contributor Author

When re-enabling these tests, they should still probably go through the "ran 50 times and didn't flake" soak time, right?

@keyonghan
Copy link
Contributor

When re-enabling these tests, they should still probably go through the "ran 50 times and didn't flake" soak time, right?

Yes (https://github.com/flutter/flutter/wiki/Reducing-Test-Flakiness#fixing-flaky-tests)
Whenever a test is marked as flaky, it has to be validated before enabled back.

@gaaclarke
Copy link
Member

Will it show up in go/flutter_15day_flakiness_dashboard to verify that it isn't flakey if it has been marked flakey though? We might want to add instructions on how to validate 50 successful runs.

@keyonghan
Copy link
Contributor

Will it show up in go/flutter_15day_flakiness_dashboard to verify that it isn't flakey if it has been marked flakey though? We might want to add instructions on how to validate 50 successful runs.

It will still show up in the 15 day dashboard. Created #82767 to enable an easy validation. Before that, the flutter build dashboard will be the source of truth (no task box with exclamation point for top 50 consecutive commits).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team Infra upgrades, team productivity, code health, technical debt. See also team: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants