Skip to content

Save startup timeline#65118

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

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

Conversation

@liyuqian
Copy link
Contributor

@liyuqian liyuqian commented Sep 2, 2020

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
Copy link

flutter-dashboard bot commented Sep 2, 2020

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
Copy link
Contributor Author

liyuqian commented Sep 2, 2020

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

@zanderso
Copy link
Member

zanderso commented Sep 2, 2020

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
Copy link
Contributor

jonahwilliams commented Sep 2, 2020

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
Copy link
Contributor

jonahwilliams commented Sep 2, 2020

For example:

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

Line 12 in 9b7b9d7

final FakeVmServiceHost fakeVmServiceHost = FakeVmServiceHost(

@jonahwilliams
Copy link
Contributor

jonahwilliams commented Sep 8, 2020

@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
[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
Copy link
Contributor Author

liyuqian commented Sep 10, 2020

@jonahwilliams : test added :)

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

final dynamic kExpectedTimeline = json.decode('''
Copy link
Contributor

@jonahwilliams jonahwilliams Sep 10, 2020

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.

Copy link
Contributor Author

@liyuqian liyuqian Sep 11, 2020

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
Copy link
Contributor

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

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

LGTM with nit

@fluttergithubbot
Copy link
Contributor

fluttergithubbot commented Sep 11, 2020

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
Copy link
Member

zanderso commented Sep 15, 2020

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
Copy link
Contributor Author

liyuqian commented Sep 15, 2020

"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