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

[flutter_tools] add timeline ANR integration test #79991

Merged
merged 5 commits into from Apr 8, 2021

Conversation

jonahwilliams
Copy link
Member

To prevent #79498 from being able to roll into the framework, add a test which subscribes to all event streams, logs some timeline traces, and then checks if the app is responsive.

The nature of the bug means that this test will hang forever if it fails. To prevent this from being diagnosed as an infra issue, I added a 5 minute warning message which links to the bug in question.

@flutter-dashboard flutter-dashboard bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Apr 7, 2021
@google-cla google-cla bot added the cla: yes label Apr 7, 2021
'for the bug this test is attempting to exercise.'
);
});
await vmService.streamListen(EventStreams.kVM);
Copy link
Member Author

Choose a reason for hiding this comment

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

I just went ahead and subscribed to everything

// found in the LICENSE file.

// @dart = 2.8

Copy link
Member

Choose a reason for hiding this comment

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

nit: extra newline

await vmService.streamListen(EventStreams.kHeapSnapshot);
await vmService.streamListen(EventStreams.kStdout);
await vmService.streamListen(EventStreams.kStderr);
await Future<void>.delayed(const Duration(seconds: 10));
Copy link
Member

Choose a reason for hiding this comment

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

Can this be replaced with polling on the calls we expect to eventually succeed?

Copy link
Member

Choose a reason for hiding this comment

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

Or before suggesting that, I should ask what goes wrong without this delay. What could go wrong if something takes longer than expected?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nothing goes wrong actually, I could probably take it out. I wanted to be sure that at least some timeline events were logged

Copy link
Member Author

Choose a reason for hiding this comment

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

I wasn't able to find a consistent signal in the timeline. Instead, I updated this test to poll for 30 seconds to confirm that the app is still responding.


// Verify that the app can be interacted with by querying the brightness.
// There may be a delay before the app stops responding, so poll for at least
// 30 seconds.
Copy link
Member

Choose a reason for hiding this comment

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

10 interactions separated by a 3 second delay isn't necessarily 30 seconds. It might be less flaky to say something like, "All interactions initiated in the first 30 seconds (and separated by 3 seconds) are completed within 5 minutes" rather than using a fixed number of 10 interactions.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not really sure what you're asking for here. I don't ever fail the test based on the time elapsed, I only leave a note in the logs that it may have failed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Discussed offline

@jonahwilliams
Copy link
Member Author

Update the test to interact with the app for 30 seconds, instead of making 10 interactions 3 seconds apart

Copy link
Member

@zanderso zanderso left a comment

Choose a reason for hiding this comment

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

Thanks!

@jonahwilliams jonahwilliams merged commit 2bada60 into flutter:master Apr 8, 2021
@jonahwilliams jonahwilliams deleted the timeline_test branch April 8, 2021 22:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tool Affects the "flutter" command-line tool. See also t: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants