Skip to content

Make sure all isolates start during flutter driver tests. #61841

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
fluttergithubbot merged 4 commits into from
Jul 24, 2020
Merged

Make sure all isolates start during flutter driver tests. #61841

fluttergithubbot merged 4 commits into from
Jul 24, 2020

Conversation

@speaking-in-code
Copy link
Contributor

@speaking-in-code speaking-in-code commented Jul 20, 2020 •
edited by liyuqian
Loading

Description

Fixes #24703.
Fixes #30641
Fixes #30641

Flutter driver has problems when flutter apps use more than a single isolate. The other isolates don't start, so apps malfunction. This PR fixes the problem by listening for new isolates to become runnable, and automatically starting them.

Related Issues

#24703

Tests

  • Manually tested against an app that requires isolates.
  • Added checks in flutter_driver_test.dart to verify that all isolates have resume() invoked.

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I signed the [CLA].
  • I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
  • I read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • I updated/added relevant documentation (doc comments with ///).
  • All existing and new tests are passing.
  • The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read [Handling breaking changes].

  • No, no existing tests failed, so this is not a breaking change.
  • Yes, this is a breaking change.

@fluttergithubbot fluttergithubbot added a: tests "flutter test", flutter_test, or one of our tests framework flutter/packages/flutter repository. See also f: labels. labels Jul 20, 2020
@goderbauer
Copy link
Member

goderbauer commented Jul 22, 2020

@speaking-in-code Can you rebase this to the latest master to fix the analyzer problems?

@dnfield Would you be up for reviewing this one since you're looking at e2e testing?

@dnfield
Copy link
Contributor

dnfield commented Jul 23, 2020

I suspect you've tried to do a rebase that has gotten git a bit confused.

Can you either open a new PR with just your changes, or fix up the history on this one?

@speaking-in-code
Copy link
Contributor Author

speaking-in-code commented Jul 23, 2020

Fixed this PR, I think.

dnfield
dnfield reviewed Jul 23, 2020
View reviewed changes
packages/flutter_driver/lib/src/driver/vmservice_driver.dart Outdated
Copy link
Contributor

@dnfield dnfield Jul 23, 2020

Choose a reason for hiding this comment

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

nit: prefer not shadowing parameter names in the enclosing class, which can get confusing. I don't have a great idea for a name here.

This does make me wonder if there are other cases we should be explicitly passing the isolate reference around though.

dnfield
dnfield reviewed Jul 23, 2020
View reviewed changes
packages/flutter_driver/lib/src/driver/vmservice_driver.dart Outdated
Copy link
Contributor

@dnfield dnfield Jul 23, 2020

Choose a reason for hiding this comment

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

Is there a reason we're not just setting the outter isoalte to isolateRef.load() here?

Should we be asserting isolate == isolateRef.load() somehow?

Copy link
Contributor

@dnfield dnfield Jul 23, 2020

Choose a reason for hiding this comment

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

Ahh I see, this could be other isolates the application uses that also started paused, right?

dnfield
dnfield approved these changes Jul 23, 2020
View reviewed changes
Copy link
Contributor

@dnfield dnfield 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.

dnfield
dnfield reviewed Jul 23, 2020
View reviewed changes
packages/flutter_driver/test/flutter_driver_test.dart Outdated
Copy link
Contributor

@dnfield dnfield Jul 23, 2020

Choose a reason for hiding this comment

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

Analyzer wants a type annotation on this array.

dnfield
dnfield approved these changes Jul 23, 2020
View reviewed changes
Copy link
Contributor

@dnfield dnfield left a comment

Choose a reason for hiding this comment

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

LGTM

@Piinks
Copy link
Contributor

Piinks commented Jul 23, 2020

It looks like the checks are failing here due to a version solving error. Can you try to update your branch with the latest on master?

@dnfield dnfield mentioned this pull request Jul 23, 2020
Closed
@christopherfujino
Copy link
Contributor

christopherfujino commented Jul 23, 2020

It looks like the checks are failing here due to a version solving error. Can you try to update your branch with the latest on master?

So upstream linux is actually broken with this version solving problem: #62146. That will have to be fixed first, then this PR rebased to make CI green.

@christopherfujino christopherfujino removed their request for review July 23, 2020 18:48
@dnfield dnfield mentioned this pull request Jul 23, 2020
Closed
@speaking-in-code
Copy link
Contributor Author

speaking-in-code commented Jul 24, 2020

Looks like this is ready to go except for the flutter-build check. Anything I can do to help resolve that?

@dnfield
Copy link
Contributor

dnfield commented Jul 24, 2020

Added a label that will land this when those issues get resolved

@fluttergithubbot fluttergithubbot merged commit 5fa5701 into flutter:master Jul 24, 2020
jonahwilliams pushed a commit that referenced this pull request Jul 25, 2020
Revert "Make sure all isolates start during flutter driver tests. (#6…
415818b 
…1841)"

This reverts commit 5fa5701.
@jonahwilliams jonahwilliams mentioned this pull request Jul 25, 2020
Merged
jonahwilliams pushed a commit that referenced this pull request Jul 25, 2020
Revert "Make sure all isolates start during flutter driver tests. (#6…
cb615b3 
…1841)" (#62239)

This reverts commit 5fa5701.
@speaking-in-code speaking-in-code mentioned this pull request Jul 27, 2020
Closed
@kf6gpe kf6gpe mentioned this pull request Jul 27, 2020
Closed
@guidezpl guidezpl mentioned this pull request Aug 6, 2020
Merged
Pragya007 pushed a commit to Pragya007/flutter that referenced this pull request Aug 11, 2020
@speaking-in-code @Pragya007
Make sure all isolates start during flutter driver tests. (flutter#61841
60b851b 
)
Pragya007 pushed a commit to Pragya007/flutter that referenced this pull request Aug 11, 2020
@Pragya007
Revert "Make sure all isolates start during flutter driver tests. (fl…
890d134 
…utter#61841)" (flutter#62239)

This reverts commit 5fa5701.
mingwandroid pushed a commit to mingwandroid/flutter that referenced this pull request Sep 6, 2020
@speaking-in-code @mingwandroid
Make sure all isolates start during flutter driver tests. (flutter#61841
afcf88f 
)
mingwandroid pushed a commit to mingwandroid/flutter that referenced this pull request Sep 6, 2020
@mingwandroid
Revert "Make sure all isolates start during flutter driver tests. (fl…
93c80ea 
…utter#61841)" (flutter#62239)

This reverts commit 5fa5701.
speaking-in-code added a commit to speaking-in-code/flutter that referenced this pull request Sep 10, 2020
@speaking-in-code
Make sure all isolates start during flutter driver tests. (flutter#61841
5e08855 
)
@speaking-in-code speaking-in-code mentioned this pull request Sep 13, 2020
Merged
9 tasks
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@dnfield dnfield dnfield approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

a: tests "flutter test", flutter_test, or one of our tests framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Milestone

No milestone

7 participants

@speaking-in-code @goderbauer @dnfield @Piinks @christopherfujino @googlebot @fluttergithubbot