Skip to content

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

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

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

Conversation

@jonahwilliams
Copy link
Contributor

@jonahwilliams jonahwilliams commented Jun 3, 2021 •
edited
Loading

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

jonahwilliams commented Jun 3, 2021

FYI @Hixie .

@Hixie
Copy link
Contributor

Hixie commented Jun 3, 2021

Oh, haha, good catch.

@jonahwilliams
Copy link
Contributor Author

jonahwilliams commented Jun 3, 2021

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
packages/flutter/lib/src/foundation/binding.dart Outdated
);

registerStringServiceExtension(
name: 'activeDevToolsServerAddress',
Copy link
Member

@kenzieschmoll kenzieschmoll Jun 3, 2021

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?

Copy link
Contributor Author

@jonahwilliams jonahwilliams Jun 3, 2021

Choose a reason for hiding this comment

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

Ahh good catch

@jonahwilliams
Copy link
Contributor Author

jonahwilliams commented Jun 3, 2021

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

Hixie commented Jun 3, 2021

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

@Hixie
Copy link
Contributor

Hixie commented Jun 3, 2021 •
edited
Loading

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

jonahwilliams commented Jun 3, 2021

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

jonahwilliams commented Jun 3, 2021

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