Skip to content

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

Merged
fluttergithubbot merged 1 commit intoflutter:masterfrom
gaaclarke:unawaited-devicelab
May 18, 2021
Merged

added unawaited function for devicelab and turned on unawaited_futures linter#82833
fluttergithubbot merged 1 commit intoflutter:masterfrom
gaaclarke:unawaited-devicelab

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