Skip to content

Conversation

@zanderso
Copy link
Member

@zanderso zanderso commented Apr 6, 2020

Description

This PR is #52666, but refactored to allow tests to be written.

Tests

I added the following tests:

Tests added to devfs_test.dart and os_test.dart.

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.

@zanderso zanderso added tool Affects the "flutter" command-line tool. See also t: labels. work in progress; do not review labels Apr 6, 2020
@zanderso zanderso requested a review from jonahwilliams April 7, 2020 20:18
Copy link
Contributor

@jonahwilliams jonahwilliams Apr 7, 2020

Choose a reason for hiding this comment

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

Thinking about how this API will evolve in a world without globals. Either:

  1. every DevFSContent needs a reference to OperatingSystemUtils
  2. contentsAsCompressedStream accepts an OperatingSystemUtils or a Stream<List<int>> Function(Stream<List<int>>) torn off from one.

The latter case sounds more appealing to me, since it simplifies the construction of the DevFSContent classes and reduces the dependencies. In that case, instead of mocking the OperatingSystemUtils for the current test, you might need to do something different.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

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

@fluttergithubbot fluttergithubbot merged commit 090fc5c into flutter:master Apr 8, 2020
@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.

Labels

tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants