Skip to content

remove flutter_test quiver dep, use fake_async and clock instead#54125

Merged
jakemac53 merged 5 commits intoflutter:masterfrom
jakemac53:drop-flutter-test-quiver-dep
Apr 15, 2020
Merged

remove flutter_test quiver dep, use fake_async and clock instead#54125
jakemac53 merged 5 commits intoflutter:masterfrom
jakemac53:drop-flutter-test-quiver-dep

Conversation

@jakemac53
Copy link
Copy Markdown
Contributor

@jakemac53 jakemac53 commented Apr 6, 2020 •
edited
Loading

Description

Removes the flutter_test dependency on quiver, instead using the more targeted clock and fake_async packages.

Related Issues

#53908

Tests

No changes to tests

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.

@jakemac53 jakemac53 requested a review from jonahwilliams April 6, 2020 18:45
@fluttergithubbot fluttergithubbot added a: tests "flutter test", flutter_test, or one of our tests framework flutter/packages/flutter repository. See also f: labels. c: contributor-productivity Team-specific productivity, code health, technical debt. labels Apr 6, 2020
@fluttergithubbot
Copy link
Copy Markdown
Contributor

fluttergithubbot commented Apr 6, 2020

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.

jonahwilliams
jonahwilliams approved these changes Apr 6, 2020
View reviewed changes
Copy link
Copy Markdown
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

Note for future readers. This will increase the number of packages that flutter_test depends on by 1. Each package is significantly smaller than quiver however, so it should be considered a net positive.

@jakemac53 jakemac53 changed the title remove flutter_test quiver dep, use fake_async and time instead remove flutter_test quiver dep, use fake_async and clock instead Apr 6, 2020
@jakemac53
Copy link
Copy Markdown
Contributor Author

jakemac53 commented Apr 6, 2020

Correct it does increase the number of packages - I have filed an issue on quiver to also just export these packages instead of duplicating the code, so ultimately it maybe a reduction in dependencies of 1 if they did choose to do that.

@mit-mit mit-mit mentioned this pull request Apr 6, 2020
Closed
12 tasks
@jakemac53 jakemac53 force-pushed the drop-flutter-test-quiver-dep branch from a3cff11 to 4dfddb4 Compare April 7, 2020 14:47
@jakemac53
Copy link
Copy Markdown
Contributor Author

jakemac53 commented Apr 7, 2020

Note that this is currently blocked on rolling the new fake_async internally which I am doing now

@jakemac53 jakemac53 mentioned this pull request Apr 7, 2020
Merged
@jakemac53
Copy link
Copy Markdown
Contributor Author

jakemac53 commented Apr 7, 2020

Updated with design doc/migration plan as this did turn out to be breaking.

Will rebase and fix conflicts once its ready to land.

@goderbauer
Copy link
Copy Markdown
Member

goderbauer commented Apr 8, 2020

fly-by comment: I saw you wrote a migration guide linked in the PR description. We typically publish those on our website here to make them available to our users: https://github.com/flutter/website/tree/master/src/docs/release/breaking-changes

@jakemac53
Copy link
Copy Markdown
Contributor Author

jakemac53 commented Apr 8, 2020

Oh ok - I will go ahead and send a pr for it there, that gives a better opportunity for commenting on the wording of it etc as well 👍

@jakemac53
Copy link
Copy Markdown
Contributor Author

jakemac53 commented Apr 9, 2020

flutter/website#3932

@jakemac53 jakemac53 requested review from Hixie and redbrogdon and removed request for Hixie April 14, 2020 19:08
@jakemac53 jakemac53 assigned redbrogdon and unassigned Hixie Apr 14, 2020
@redbrogdon
Copy link
Copy Markdown
Contributor

redbrogdon commented Apr 14, 2020

This is the type of breaking change for which the affected audience is likely to be quite sophisticated and able to roll with the new API. The migration guide is great, and should be all the community needs.

@jakemac53 jakemac53 requested a review from Piinks as a code owner April 15, 2020 17:55
@jakemac53 jakemac53 merged commit 6399be6 into flutter:master Apr 15, 2020
@jakemac53 jakemac53 deleted the drop-flutter-test-quiver-dep branch April 15, 2020 19:10
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 31, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@redbrogdon redbrogdon Awaiting requested review from redbrogdon

@Piinks Piinks Awaiting requested review from Piinks

1 more reviewer

@jonahwilliams jonahwilliams jonahwilliams approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@redbrogdon redbrogdon

Labels

a: tests "flutter test", flutter_test, or one of our tests c: contributor-productivity Team-specific productivity, code health, technical debt. 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.

7 participants

@jakemac53 @fluttergithubbot @goderbauer @redbrogdon @jonahwilliams @Hixie @googlebot