Skip to content

Save startup timeline - #65118

Merged
liyuqian merged 3 commits into
flutter:masterfrom
liyuqian:save_startup_timeline
Sep 15, 2020
Merged

Save startup timeline#65118
liyuqian merged 3 commits into
flutter:masterfrom
liyuqian:save_startup_timeline

Conversation

@liyuqian

@liyuqian liyuqian commented Sep 2, 2020

Copy link
Copy Markdown
Contributor

This would help us investigate issues like
#64781

@flutter-dashboard flutter-dashboard Bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Sep 2, 2020
@flutter-dashboard

flutter-dashboard Bot commented Sep 2, 2020

Copy link
Copy Markdown

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@liyuqian liyuqian mentioned this pull request Sep 2, 2020
Open
@liyuqian
liyuqian requested a review from zanderso September 2, 2020 20:39
@liyuqian

liyuqian commented Sep 2, 2020

Copy link
Copy Markdown
Contributor Author

@jonahwilliams : any suggestion on how to unit test this? packages/flutter_tools/test doesn't seem to have anything related with --trace-startup.

@zanderso

zanderso commented Sep 2, 2020

Copy link
Copy Markdown
Member

The Tracing class looks like it needs some love before it can be tested properly. In particular, the instance method downloadStartupTrace() creates a new instance?

flutter/packages/flutter_tools/lib/src/tracing.dart

Line 97 in cb81950

final Tracing tracing = Tracing(vmService);

@jonahwilliams thoughts?

@jonahwilliams

jonahwilliams commented Sep 2, 2020

Copy link
Copy Markdown
Contributor

There is a FakeVmServiceHost which can be configured for a set of request/responses, there should be a number of tests that use that pattern. You should be able to provide that fake vm service in place of the real one to get a unit test going.

@jonahwilliams

jonahwilliams commented Sep 2, 2020

Copy link
Copy Markdown
Contributor

For example:

flutter/packages/flutter_tools/test/general.shard/coverage_collector_test.dart

Line 12 in 9b7b9d7

final FakeVmServiceHost fakeVmServiceHost = FakeVmServiceHost(

@jonahwilliams

jonahwilliams commented Sep 8, 2020

Copy link
Copy Markdown
Contributor

@dramosv90 I believe we asked you several months ago to stop approving PRs that you have not really reviewed, or are not involved in the design decision for.

@jonahwilliams jonahwilliams mentioned this pull request Sep 9, 2020
Merged
jonahwilliams pushed a commit that referenced this pull request Sep 9, 2020
Jonah Williams
[flutter_tools] remove globals from tracing and add unit tests (#65490)
b4551e3 
Removes global variables and adds unit tests that can be copied for #65118
@liyuqian
Save startup timeline
a8b6999 
This would help us investigate issues like
flutter#64781
@liyuqian
Add a test
8f45f4d
@liyuqian
liyuqian force-pushed the save_startup_timeline branch from 6506b28 to 8f45f4d Compare September 10, 2020 22:51
@liyuqian

liyuqian commented Sep 10, 2020

Copy link
Copy Markdown
Contributor Author

@jonahwilliams : test added :)

jonahwilliams
jonahwilliams reviewed Sep 10, 2020
View reviewed changes
Comment thread packages/flutter_tools/test/general.shard/tracing_test.dart Outdated
logger: logger,
);

final dynamic kExpectedTimeline = json.decode('''

@jonahwilliams jonahwilliams Sep 10, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

if you parse the result and compare a dart map, the test will be resilient to changes in formatting.

@liyuqian liyuqian Sep 11, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

jonahwilliams
jonahwilliams approved these changes Sep 10, 2020
View reviewed changes

@jonahwilliams jonahwilliams left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM with nit

@fluttergithubbot

fluttergithubbot commented Sep 11, 2020

Copy link
Copy Markdown
Contributor

This pull request is not suitable for automatic merging in its current state.

  • The status or check suite Linux web_integration_tests has failed. Please fix the issues identified (or deflake) before re-applying this label.

@zanderso

zanderso commented Sep 15, 2020

Copy link
Copy Markdown
Member

What's the status of this PR? Do the presubmit checks need another kick?

@liyuqian
liyuqian merged commit e3c6979 into flutter:master Sep 15, 2020
@liyuqian

liyuqian commented Sep 15, 2020

Copy link
Copy Markdown
Contributor Author

"Google tests" seems to be stuck. Merge manually now.

@liyuqian
liyuqian deleted the save_startup_timeline branch October 8, 2020 01:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zanderso zanderso Awaiting requested review from zanderso
2 more reviewers
@jonahwilliams jonahwilliams jonahwilliams approved these changes
@darielopper darielopper darielopper approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@liyuqian @zanderso @jonahwilliams @fluttergithubbot @darielopper @googlebot