Skip to content

Option to enable the performance overlay from 'flutter run'. #11288

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.

Sign up for GitHub

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

Jump to bottom
Merged
Hixie merged 1 commit into from
Jul 19, 2017
Merged

Option to enable the performance overlay from 'flutter run'. #11288

Hixie merged 1 commit into from
Jul 19, 2017

Conversation

@Hixie
Copy link
Contributor

@Hixie Hixie commented Jul 19, 2017

No description provided.

@Hixie
Copy link
Contributor Author

Hixie commented Jul 19, 2017

cc @yjbanov for review since most of this is devicelab and driver changes to support the test of the actual feature which is just a dozen lines or so. :-)

@Hixie Hixie force-pushed the performance-overlay branch from 24d311b to 9c13afb Compare July 19, 2017 16:15
goderbauer
goderbauer approved these changes Jul 19, 2017
View reviewed changes
Copy link
Member

@goderbauer goderbauer left a comment

Choose a reason for hiding this comment

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

LGTM

goderbauer
goderbauer reviewed Jul 19, 2017
View reviewed changes
dev/devicelab/bin/tasks/commands_test.dart Outdated
Copy link
Member

@goderbauer goderbauer Jul 19, 2017

Choose a reason for hiding this comment

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

nit: update year.

goderbauer
goderbauer reviewed Jul 19, 2017
View reviewed changes
dev/integration_tests/ui/test_driver/commands_debug_paint_test.dart Outdated
Copy link
Member

@goderbauer goderbauer Jul 19, 2017

Choose a reason for hiding this comment

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

license headers? Here and below?

Copy link
Contributor Author

@Hixie Hixie Jul 19, 2017

Choose a reason for hiding this comment

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

added to all files in this directory

yjbanov
yjbanov reviewed Jul 19, 2017
View reviewed changes
dev/devicelab/bin/tasks/commands_test.dart Outdated
Copy link
Contributor

@yjbanov yjbanov Jul 19, 2017

Choose a reason for hiding this comment

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

I think test increases the number of tests interacting with tools' std streams from 1 to 2. Perhaps it's time we created a utility library for these kinds of tests?

Copy link
Contributor Author

@Hixie Hixie Jul 19, 2017

Choose a reason for hiding this comment

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

There's not that much in common. A future version of commands_test will probably further test the output to make sure we're logging the right messages when pressing keys, for example.

dev/integration_tests/ui/test_driver/commands_performance_overlay_test.dart Outdated
Copy link
Contributor

@yjbanov yjbanov Jul 19, 2017

Choose a reason for hiding this comment

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

Is const Duration() == Duration.ZERO? If so, I'd use Duration.ZERO instead.

Copy link
Contributor Author

@Hixie Hixie Jul 19, 2017

Choose a reason for hiding this comment

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

oh right, good call.

packages/flutter_driver/lib/src/driver.dart Outdated
Copy link
Contributor

@yjbanov yjbanov Jul 19, 2017

Choose a reason for hiding this comment

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

nit: wrap "String" and "int" in backticks: String, int.

Copy link
Contributor Author

@Hixie Hixie Jul 19, 2017

Choose a reason for hiding this comment

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

hyperlinked

packages/flutter_driver/lib/src/extension.dart Outdated
Copy link
Contributor

@yjbanov yjbanov Jul 19, 2017

Choose a reason for hiding this comment

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

isEmpty?

Copy link
Contributor

@yjbanov yjbanov Jul 19, 2017

Choose a reason for hiding this comment

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

Actually, I think you don't need the if block at all. _waitForAbsentElement returns iff Finder.precache returns false, which means it found nothing. Otherwise, we rely on the timeout logic. So this method can be simplified to:

final WaitForAbsent waitForAbsentCommand = command;
await _waitForAbsentElement(_createFinder(waitForAbsentCommand.finder));
return new WaitForAbsentResult();

Copy link
Contributor Author

@Hixie Hixie Jul 19, 2017

Choose a reason for hiding this comment

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

done

packages/flutter_driver/lib/src/extension.dart Outdated
Copy link
Contributor

@yjbanov yjbanov Jul 19, 2017

Choose a reason for hiding this comment

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

My intuition tells me that this should throw.

@Hixie Hixie force-pushed the performance-overlay branch from 9c13afb to fc2178d Compare July 19, 2017 19:27
@Hixie
Copy link
Contributor Author

Hixie commented Jul 19, 2017

will land on green

@Hixie Hixie merged commit e1adc52 into flutter:master Jul 19, 2017
@Hixie Hixie deleted the performance-overlay branch July 19, 2017 19:57
@yjbanov yjbanov mentioned this pull request Jul 20, 2017
Closed
@tvolkert
Copy link
Contributor

tvolkert commented Jul 20, 2017

Tried to use this inside Google today, and getting incorrect behavior:

  • If I call await driver.waitForAbsent(find.text('foo'), timeout: const Duration(seconds: 20)), the future never completes, and the test fails with a timeout exception

  • If I first await a delayed future of 6 seconds to let the Snackbar that holds the text go away, then run the await above, then the future completes just fine, and the test succeeds.

@tvolkert
Copy link
Contributor

tvolkert commented Jul 20, 2017

The future is never completing because of an error during this call:

await _waitUntilFrame(() => !finder.precache());

The error is:

'package:flutter_test/src/finders.dart': Failed assertion: line 269: '_cachedResult == null': is not true.

@Hixie
Copy link
Contributor Author

Hixie commented Jul 20, 2017

please file a bug

@tvolkert
Copy link
Contributor

tvolkert commented Jul 20, 2017

#11327

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@yjbanov yjbanov yjbanov left review comments

@goderbauer goderbauer goderbauer approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@Hixie @tvolkert @yjbanov @goderbauer @googlebot