Skip to content

added unawaited function for devicelab and turned on unawaited_futures linter #82833

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 1 commit into from
May 18, 2021
Merged

added unawaited function for devicelab and turned on unawaited_futures linter #82833

fluttergithubbot merged 1 commit into from
May 18, 2021

Conversation

@gaaclarke
Copy link
Member

@gaaclarke gaaclarke commented May 18, 2021 •
edited
Loading

fixes #82829

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I signed the CLA.
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@flutter-dashboard flutter-dashboard bot added the c: contributor-productivity Team-specific productivity, code health, technical debt. label May 18, 2021
@google-cla google-cla bot added the cla: yes label May 18, 2021
@gaaclarke gaaclarke force-pushed the unawaited-devicelab branch from c6e3c90 to 0416b70 Compare May 18, 2021 16:56
gaaclarke
gaaclarke commented May 18, 2021
View reviewed changes
dev/devicelab/bin/run.dart Outdated
Copy link
Member Author

@gaaclarke gaaclarke May 18, 2021

Choose a reason for hiding this comment

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

This should probably have an await.

Copy link
Contributor

@jonahwilliams jonahwilliams May 18, 2021

Choose a reason for hiding this comment

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

This can use writeAsStringSync

dev/devicelab/bin/tasks/flutter_engine_group_performance.dart Outdated
Copy link
Member Author

@gaaclarke gaaclarke May 18, 2021

Choose a reason for hiding this comment

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

This should probably have an await.

Copy link
Contributor

@jonahwilliams jonahwilliams May 18, 2021

Choose a reason for hiding this comment

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

you could return the future from task, i think technically this might be fine since the test runner sets up some handlers to detect the test starting

dev/devicelab/bin/tasks/gradle_desugar_classes_test.dart Outdated
Copy link
Member Author

@gaaclarke gaaclarke May 18, 2021

Choose a reason for hiding this comment

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

This should probably have an await.

Copy link
Contributor

@jonahwilliams jonahwilliams May 18, 2021

Choose a reason for hiding this comment

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

I would make addPlugin sync instead

Copy link
Contributor

@jonahwilliams jonahwilliams May 18, 2021

Choose a reason for hiding this comment

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

this can be done by updating the writeAsString call

dev/devicelab/bin/tasks/gradle_plugin_light_apk_test.dart Outdated
Copy link
Member Author

@gaaclarke gaaclarke May 18, 2021

Choose a reason for hiding this comment

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

This should probably have an await.

dev/devicelab/bin/tasks/ios_content_validation_test.dart Outdated
Copy link
Member Author

@gaaclarke gaaclarke May 18, 2021

Choose a reason for hiding this comment

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

This should probably have an await.

Copy link
Contributor

@jonahwilliams jonahwilliams May 18, 2021

Choose a reason for hiding this comment

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

Add an await?

dev/devicelab/bin/tasks/ios_content_validation_test.dart Outdated
Copy link
Member Author

@gaaclarke gaaclarke May 18, 2021

Choose a reason for hiding this comment

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

This should probably have an await.

dev/devicelab/bin/tasks/module_test_ios.dart Outdated
Copy link
Member Author

@gaaclarke gaaclarke May 18, 2021

Choose a reason for hiding this comment

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

This should probably have an await.

dev/devicelab/bin/tasks/service_extensions_test.dart Outdated
Copy link
Member Author

@gaaclarke gaaclarke May 18, 2021

Choose a reason for hiding this comment

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

This should probably have an await.

dev/devicelab/lib/tasks/build_test_task.dart Outdated
Copy link
Member Author

@gaaclarke gaaclarke May 18, 2021

Choose a reason for hiding this comment

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

This should probably have an await.

@gaaclarke gaaclarke force-pushed the unawaited-devicelab branch from 0416b70 to b367a32 Compare May 18, 2021 17:04
@gaaclarke gaaclarke marked this pull request as ready for review May 18, 2021 17:06
@gaaclarke
Copy link
Member Author

gaaclarke commented May 18, 2021

@zanderso @jonahwilliams I'm not sure if we just want to make this PR a refactor. I marked down the places I thought we might want to actually await. Maybe that should be a different PR in case we have to revert it?

@gaaclarke gaaclarke force-pushed the unawaited-devicelab branch from 86d3aa3 to b367a32 Compare May 18, 2021 18:00
@gaaclarke gaaclarke mentioned this pull request May 18, 2021
Merged
8 tasks
@gaaclarke
Copy link
Member Author

gaaclarke commented May 18, 2021

@jonahwilliams @zanderso I authored a new PR that migrates the unawaited calls in the cases we've identified here: #82839 I'll keep the PR's separate so that this one is just a straight refactor with no logical changes.

@jonahwilliams
Copy link
Contributor

jonahwilliams commented May 18, 2021

Works for me!

jonahwilliams
jonahwilliams approved these changes May 18, 2021
View reviewed changes
Copy link
Contributor

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

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

LGTM

@gaaclarke gaaclarke force-pushed the unawaited-devicelab branch from b367a32 to 272070b Compare May 18, 2021 20:07
@fluttergithubbot fluttergithubbot merged commit 7cdd33f into flutter:master May 18, 2021
@jmagman jmagman mentioned this pull request May 6, 2022
Merged
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zanderso zanderso zanderso approved these changes

+1 more reviewer

@jonahwilliams jonahwilliams jonahwilliams approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

c: contributor-productivity Team-specific productivity, code health, technical debt.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Enable the unawaited Futures lint under dev/devicelab

4 participants

@gaaclarke @jonahwilliams @zanderso @fluttergithubbot