Skip to content

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

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
merged 1 commit into from
Sep 18, 2020
Merged

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

merged 1 commit into from
Sep 18, 2020

Conversation

speaking-in-code
Copy link
Contributor

Description

Fixes #24703.

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 telling the dart VM to let new isolates start automatically. This only happens if the first isolate is found paused; otherwise we don't mess with the flag.

There have been two previous attempts at fixing this bug.

#61841 was reverted because it broke a fragile test app.
#64432 because of timing issues when a hot reload/hot restart occurred while flutter driver was connected with the app.

Thanks to tvolkert@ for pointing out an alternate approach that uses a dart VM feature to keep flutter driver code simpler. Hopefully this approach is more stable, but given history this one might get rolled back too.

Related Issues

24703

Tests

I added the following tests:

  • Added unit tests in flutter_driver_test.dart to verify that all isolates have resume() invoked.
  • Ran devicelab tests for the flutter gallery app, verifying that the bug that caused this change to be reverted last time (Revert "Make sure all isolates start during flutter driver tests." #62239) doesn't reoccur.
  • Added a new example app that uses isolates, and tested it on several platfoms: macos, android simulator and physical devices, ios simulator and physical devices.
  • Manually tested against an app that uses isolates started from native plugins.

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.

@flutter-dashboard flutter-dashboard bot added a: tests "flutter test", flutter_test, or one of our tests framework flutter/packages/flutter repository. See also f: labels. labels Sep 13, 2020
@speaking-in-code
Copy link
Contributor Author

@dnfield got a minute to take a look at this one?

@dnfield
Copy link
Contributor

dnfield commented Sep 15, 2020

This seems fine to me. I'm unable right now to run tests internally due to some networking issues. @renyou would you be able to help with this?

@guidezpl guidezpl mentioned this pull request Sep 15, 2020
Closed
@speaking-in-code
Copy link
Contributor Author

@renyou @guidezpl could one of you two help with the internal testing for this PR?

@guidezpl
Copy link
Member

@renyou See cl/332401506, only a couple of (I believe) unrelated flakes

@renyou
Copy link
Contributor

renyou commented Sep 18, 2020

Sorry somehow I didn't see the messages on this PR earlier. So far the tests looks good. Just to be safe, I started a full test on this PR too. Hopefully we will see the results in a couple of hours. I will report back here if we found any thing.

@renyou
Copy link
Contributor

renyou commented Sep 18, 2020

Sorry somehow I didn't see the messages on this PR earlier. So far the tests looks good. Just to be safe, I started a full test on this PR too. Hopefully we will see the results in a couple of hours. I will report back here if we found any thing.

Didn't realize you already did the full test :) Good to go!

@fluttergithubbot fluttergithubbot merged commit 7a4d8e1 into flutter:master Sep 18, 2020
goderbauer pushed a commit to goderbauer/flutter that referenced this pull request Sep 18, 2020
@speaking-in-code @goderbauer
Make sure all isolates start during flutter driver tests. (flutter#65703
8c2fb3a 
)
@darshankawar darshankawar mentioned this pull request Oct 12, 2020
Closed
@tvolkert tvolkert mentioned this pull request Oct 26, 2020
Closed
@dnfield dnfield mentioned this pull request Oct 27, 2020
Merged
@dnfield dnfield mentioned this pull request Aug 17, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dnfield dnfield dnfield approved these changes

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
Development

Successfully merging this pull request may close these issues.

flutter_driver pauses all isolates at their startup (even ones started from compute()), rather than only pausing initial isolate
6 participants
@speaking-in-code @dnfield @guidezpl @renyou @googlebot @fluttergithubbot