Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@nturgut
Copy link
Contributor

@nturgut nturgut commented Jun 5, 2020

running ios unit tests from felt by booting/shutingdown simulators. create simulator with felt.
updating the readme.

Enabled felt commands:

felt create --majorVersion=11 --minorVersion=1 --device='iPhone.11.Pro'
felt test --browser=ios-safari --majorVersion=13 --minorVersion=1 --device='iPhone.11.Pro'

Contributes to flutter/flutter#53690

@nturgut nturgut requested review from mdebbar and yjbanov June 5, 2020 22:52
@fluttergithubbot
Copy link
Contributor

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.

Copy link
Contributor

@yjbanov yjbanov left a comment

Choose a reason for hiding this comment

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

Do we need all the command-line options? Can we support using a custom simulator instance by picking whatever is currently running? Then, all you need to do to choose a different simulator is Cmd + Space, type "simulator" and select the one you want.

cleanupCallbacks.forEach((element) {
element.call();
cleanupCallbacks.forEach((element) async {
await element.call();
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the effect of this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I didn't run in like this, I wasn't able to see the INFO, I print out:
"print('INFO: Simulator ${iosSimulator.id} shutdown.');"

The previous implementation was missing the await. Let me know if I'm missing something. I'm open to suggestions on change.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, strange. This shouldn't have any effect because cleanupCallbacks.forEach is synchronous anyway; it ignores async and await. However, the following would have an effect:

for (AsyncCallback cleanupCallback in cleanupCallbacks) {
  await cleanupCallback();
}

This is because the for loop is aware of await and it would not pick up the next element until the previous callback's future is resolved.

Copy link
Contributor Author

@nturgut nturgut Jun 10, 2020

Choose a reason for hiding this comment

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

This worked when I also combined it with adding a await to the call inside test_runner.dart

thanks for the suggestions.

I think, I should have initially used Future.forEach instead: https://api.dart.dev/stable/2.8.4/dart-async/Future/forEach.html

@nturgut nturgut requested a review from yjbanov June 9, 2020 21:58
@nturgut
Copy link
Contributor Author

nturgut commented Jun 10, 2020

Merging the PR since it is related to Flutter Web tooling. No relation to the failing code paths.

@nturgut nturgut merged commit eaa2f7f into flutter:master Jun 10, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 10, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 10, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 10, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 10, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 10, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 11, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants