Skip to content

[flutter] ensure vmservice binding is registered in profile mode - #83913

Merged
fluttergithubbot merged 4 commits into
flutter:masterfrom
jonahwilliams:register_vm_url_profile
Jun 4, 2021
Merged

[flutter] ensure vmservice binding is registered in profile mode#83913
fluttergithubbot merged 4 commits into
flutter:masterfrom
jonahwilliams:register_vm_url_profile

Conversation

@jonahwilliams

@jonahwilliams jonahwilliams commented Jun 3, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

Ensure that devtools will launch from the cmdline on a profile build

WIP due to testing

@flutter-dashboard flutter-dashboard Bot added the framework flutter/packages/flutter repository. See also f: labels. label Jun 3, 2021
@google-cla google-cla Bot added the cla: yes label Jun 3, 2021
@jonahwilliams

jonahwilliams commented Jun 3, 2021

Copy link
Copy Markdown
Contributor Author

FYI @Hixie .

@Hixie

Hixie commented Jun 3, 2021

Copy link
Copy Markdown
Contributor

Oh, haha, good catch.

@jonahwilliams

jonahwilliams commented Jun 3, 2021

Copy link
Copy Markdown
Contributor Author

I'd like to find a way to test this without adding a devicelab test, but I'm scratching my head a bit right now

kenzieschmoll
kenzieschmoll reviewed Jun 3, 2021
View reviewed changes
Comment thread packages/flutter/lib/src/foundation/binding.dart Outdated
);

registerStringServiceExtension(
name: 'activeDevToolsServerAddress',

@kenzieschmoll kenzieschmoll Jun 3, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this one probably needs to move out of the assert block too right?

@jonahwilliams jonahwilliams Jun 3, 2021

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.

Ahh good catch

@jonahwilliams

jonahwilliams commented Jun 3, 2021

Copy link
Copy Markdown
Contributor Author

Soliciting testing ideas:

I can't turn off asserts in our unit tests, so I can't show that the extension is still registered.
I can't run a profile build in the tool integration tests, the flutter_tester only supports debug
I could add a new devicelab test to verify devtools connects, but sometimes devtools fails to activate on CI and I'm certain this would be flaky.

@Hixie

Hixie commented Jun 3, 2021

Copy link
Copy Markdown
Contributor

I can't think of a way to really test this other than an end-to-end test in devicelab either.

@Hixie

Hixie commented Jun 3, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

Maybe the first step is to fix the flakiness. :-)

(In the meantime you could also make the test pass in the case where it fails to bootstrap devtools, since we already test that in existing tests. Can a test decide midway through itself that it should be skipped?)

@jonahwilliams

jonahwilliams commented Jun 3, 2021

Copy link
Copy Markdown
Contributor Author

Yeah, I think that's what we have to do. if its flaky we can revisit.

@jonahwilliams
jonahwilliams marked this pull request as ready for review June 3, 2021 21:54
@flutter-dashboard flutter-dashboard Bot added the c: contributor-productivity Team-specific productivity, code health, technical debt. label Jun 3, 2021
@jonahwilliams

jonahwilliams commented Jun 3, 2021

Copy link
Copy Markdown
Contributor Author

I added a devicelab test, will add to infra config after this lands

@fluttergithubbot
fluttergithubbot merged commit 85df73d into flutter:master Jun 4, 2021
@jmagman jmagman mentioned this pull request Jul 21, 2021
Closed
21 of 22 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kenzieschmoll kenzieschmoll kenzieschmoll approved these changes

Assignees

No one assigned

Labels

c: contributor-productivity Team-specific productivity, code health, technical debt. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jonahwilliams @Hixie @kenzieschmoll @fluttergithubbot