Skip to content
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.

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

[tool] Consistent FakeProcessManager.run/runSync #103947

Merged
merged 1 commit into from
May 17, 2022

Conversation

cbracken
Copy link
Member

@cbracken cbracken commented May 17, 2022

FakeProcessManager is a test-oriented implementation of ProcessManager
that simulates launching processes and returning ProcessResult objects
whose exitCode, stdout, stderr can be used to write platform-portable,
hermetic tests that don't rely on actually launching processes from
executables on disk. Its run and runSync methods provide asynchronous and
synchronous variants of this functionality.

Previously, the behaviour of run and runSync were inconsistent with
regards to the treatment of the stdoutEncoding (similarly,
stderrEncoding) parameters:

run:

  • if the encoding was null, ProcessResult.stdout was returned as a
    String in UTF-8 encoding. This was incorrect. The behaviour as
    specified in ProcessResult.stdout is that in this case, a raw
    List<int> should be returned.
  • If the encoding was unspecified, ProcessResult.stdout was returned as
    a String in the io.systemEncoding encoding. This was correct.
  • If the encoding was non-null, ProcessResult.stdout was returned as a
    String in the specified encoding. This was correct.

runSync:

  • if the encoding was null, ProcessResult.stdout was returned as a
    List<int> in UTF-8 encoding. This was incorrect. The behaviour as
    specified in ProcessResult.stdout is that in this case, a raw
    List<int> should be returned.
  • If the encoding was unspecified, ProcessResult.stdout was returned as
    List<int> in UTF-8 encoding. This was incorrect. The behaviour as
    specified in ProcessResult.stdout is that in this case, a String a
    String in the io.systemEncoding encoding should be returned.
  • if the encoding was non-null, ProcessResult.stdout was returned as a
    String in unknown (but probably UTF-8) encoding. This was incorrect.
    The behaviour as specified in ProcessResult.stdout is that in this
    case, a String in the specified encoding should be returned.

_FakeProcess, from which we obtain the fake stdout and stderr values now
holds these fields as raw List<int> of bytes rather than as Strings. It
is up to the user to supply values that can be decoded with the encoding
passed to run/runAsync.

run and runAsync have been updated to set stdout (likewise, stderr) as
specified in the ProcessResult documentation.

This is pre-factoring for #102451, in which the tool throws an exception
when processing the JSON output from stdout of the vswhere.exe tool,
whose output was found to include the U+FFFD Unicode replacement
character during UTF-8 decoding, which triggers a toolExit exception
when decoded using our Utf8Decoder configured with reportErrors =
true. Because FakeProcessManager.runAsync did not previously invoke
utf8.decode on its output (behaviour which differs from the non-fake
implementation), it was impossible to write tests to verify the fix.

Ref: https://api.flutter.dev/flutter/dart-io/ProcessResult/stdout.html

Issue: #102451

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 this 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.

FakeProcessManager is a test-oriented implementation of ProcessManager
that simulates launching processes and returning ProcessResult objects
whose exitCode, stdout, stderr can be used to write platform-portable,
hermetic tests that don't rely on actually launching processes from
executables on disk. Its run and runSync methods provide aynchronous and
synchronous variants of this functionality.

Previously, the behaviour of run and runSync were inconsistent with
regards to the treatment of the `stdoutEncoding` (similarly,
`stderrEncoding`) parameters:

`run`:
* if the encoding was null, ProcessResult.stdout was returned as a
  String in UTF-8 encoding. This was incorrect. The behaviour as
  specified in ProcessResult.stdout is that in this case, a raw
  List<int> should be returned.
* If the encoding was unspecified, ProcessResult.stdout was returned as
  a String in the io.systemEncoding encoding. This was correct.
* If the encoding was non-null, ProcessResult.stdout was returned as a
  String in the specified encoding. This was correct.

`runSync`:
* if the encoding was null, ProcessResult.stdout was returned as a
  List<int> in UTF-8 encoding. This was incorrect. The behaviour as
  specified in ProcessResult.stdout is that in this case, a raw
  List<int> should be returned.
* If the encoding was unspecified, ProcessResult.stdout was returned as
  List<int> in UTF-8 encoding. This was incorrect. The behaviour as
  specified in ProcessResult.stdout is that in this case, a String a
  String in the io.systemEncoding encoding should be returned.
* if the encoding was non-null, ProcessResult.stdout was returned as a
  String in unknown (but probably UTF-8) encoding. This was incorrect.
  The behaviour as specified in ProcessResult.stdout is that in this
  case, a String in the specified encoding should be returned.

_FakeProcess, from which we obtain the fake stdout and stderr values now
holds these fields as raw List<int> of bytes rather than as Strings. It
is up to the user to supply values that can be decoded with the encoding
passed to run/runAsync.

run and runAsync have been updated to set stdout (likewise, stderr) as
specified in the ProcessResult documentation.

This is pre-factoring for flutter#102451, in which the tool throws an exception
when processing the JSON output from stdout of the `vswhere.exe` tool,
whose output was found to include the `U+FFFD` Unicode replacement
character during UTF-8 decoding, which triggers a `toolExit` exception
when decoded using our Utf8Decoder configured with `reportErrors` =
true. Because FakeProcessManager.runAsync did not previously invoke
`utf8.decode` on its output, it was impossible to write tests to verify
the fix.

Ref: https://api.flutter.dev/flutter/dart-io/ProcessResult/stdout.html

Issue: flutter#102451
@flutter-dashboard flutter-dashboard bot added the tool Affects the "flutter" command-line tool. See also t: labels. label May 17, 2022
Copy link
Member

@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

@cbracken cbracken merged commit 928bb12 into flutter:master May 17, 2022
@cbracken cbracken deleted the clean-up-fakeprocess-stuff branch May 17, 2022 18:15
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 17, 2022
@cbracken cbracken mentioned this pull request May 17, 2022
8 tasks
cbracken added a commit to cbracken/flutter that referenced this pull request May 17, 2022
Because this class has some subtle behaviour with regards to control of
exit timing and when and how it streams data to stderr and stdout, it's
worth adding unit tests for this class directly, as well as (in a
followup patch) for FakeProcessManager.

This is additional testing relating to refactoring landed in:
flutter#103947

Issue: flutter#102451
cbracken added a commit that referenced this pull request May 17, 2022
Because this class has some subtle behaviour with regards to control of
exit timing and when and how it streams data to stderr and stdout, it's
worth adding unit tests for this class directly, as well as (in a
followup patch) for FakeProcessManager.

This is additional testing relating to refactoring landed in:
#103947

Issue: #102451
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request May 18, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request May 18, 2022
cbracken added a commit to cbracken/flutter that referenced this pull request May 23, 2022
Adds a bit more clarifying documentation to the implementation of the
outputFollowsExit case, and adds tests that verify the behaviour of
stderr, stdout of processes launched via FakeProcessManager.

Specifically:
* Verifies that stderr, stdout are not emitted immediately after process
  exit if outputFollowsExit is true. They must be emitted at least one
  turn through the event loop later.
* Verifies that ProcessResult.stderr, stdout have the type documented
  according to the encoding passted to Process.run/runSync:
  * List<int> if null is passed as the encoding.
  * String (in the default system encoding) if no encoding is specified.
  * String (in the specified encoding) if an encoding is specified.

This is additional testing relating to refactoring landed in:
flutter#103947

Issue: flutter#102451
cbracken added a commit that referenced this pull request May 24, 2022
Adds a bit more clarifying documentation to the implementation of the
outputFollowsExit case, and adds tests that verify the behaviour of
stderr, stdout of processes launched via FakeProcessManager.

Specifically:
* Verifies that stderr, stdout are not emitted immediately after process
  exit if outputFollowsExit is true. They must be emitted at least one
  turn through the event loop later.
* Verifies that ProcessResult.stderr, stdout have the type documented
  according to the encoding passted to Process.run/runSync:
  * List<int> if null is passed as the encoding.
  * String (in the default system encoding) if no encoding is specified.
  * String (in the specified encoding) if an encoding is specified.

This is additional testing relating to refactoring landed in:
#103947

Issue: #102451
loic-sharma pushed a commit to loic-sharma/flutter that referenced this pull request Jun 1, 2022
`FakeProcessManager` is a test-oriented implementation of `ProcessManager`
that simulates launching processes and returning `ProcessResult` objects
whose `exitCode`, `stdout`, `stderr` can be used to write platform-portable,
hermetic tests that don't rely on actually launching processes from
executables on disk. Its `run` and `runSync` methods provide asynchronous and
synchronous variants of this functionality.

Previously, the behaviour of `run` and `runSync` were inconsistent with
regards to the treatment of the `stdoutEncoding` (similarly,
`stderrEncoding`) parameters:

`run`:
* if the encoding was null, `ProcessResult.stdout` was returned as a
  String in UTF-8 encoding. This was incorrect. The behaviour as
  specified in `ProcessResult.stdout` is that in this case, a raw
  `List<int>` should be returned.
* If the encoding was unspecified, `ProcessResult.stdout` was returned as
  a `String` in the `io.systemEncoding` encoding. This was correct.
* If the encoding was non-null, `ProcessResult.stdout` was returned as a
  `String` in the specified encoding. This was correct.

`runSync`:
* if the encoding was null, `ProcessResult.stdout` was returned as a
  `List<int>` in UTF-8 encoding. This was incorrect. The behaviour as
  specified in `ProcessResult.stdout` is that in this case, a raw
  `List<int>` should be returned.
* If the encoding was unspecified, `ProcessResult.stdout` was returned as
  `List<int>` in UTF-8 encoding. This was incorrect. The behaviour as
  specified in `ProcessResult.stdout` is that in this case, a String a
  `String` in the `io.systemEncoding` encoding should be returned.
* if the encoding was non-null, `ProcessResult.stdout` was returned as a
  `String` in unknown (but probably UTF-8) encoding. This was incorrect.
  The behaviour as specified in `ProcessResult.stdout` is that in this
  case, a `String` in the specified encoding should be returned.

`_FakeProcess`, from which we obtain the fake stdout and stderr values now
holds these fields as raw `List<int>` of bytes rather than as `String`s. It
is up to the user to supply values that can be decoded with the encoding
passed to `run`/`runAsync`.

`run` and `runAsync` have been updated to set stdout (likewise, stderr) as
specified in the `ProcessResult` documentation.

This is pre-factoring for flutter#102451, in which the tool throws an exception
when processing the JSON output from stdout of the `vswhere.exe` tool,
whose output was found to include the `U+FFFD` Unicode replacement
character during UTF-8 decoding, which triggers a `toolExit` exception
when decoded using our [Utf8Decoder][decoder] configured with `reportErrors` =
true. Because `FakeProcessManager.runAsync` did not previously invoke
`utf8.decode` on its output (behaviour which differs from the non-fake
implementation), it was impossible to write tests to verify the fix.

Ref: https://api.flutter.dev/flutter/dart-io/ProcessResult/stdout.html

Issue: flutter#102451

[decoder]: https:/flutter/flutter/blob/fd312f1ccff909fde28d2247a489bf210bbc6c48/packages/flutter_tools/lib/src/convert.dart#L51-L60
itsjustkevin added a commit that referenced this pull request Jun 2, 2022
…ere.exe (#105153)

* [tool] Consistent FakeProcessManager.run/runSync (#103947)

`FakeProcessManager` is a test-oriented implementation of `ProcessManager`
that simulates launching processes and returning `ProcessResult` objects
whose `exitCode`, `stdout`, `stderr` can be used to write platform-portable,
hermetic tests that don't rely on actually launching processes from
executables on disk. Its `run` and `runSync` methods provide asynchronous and
synchronous variants of this functionality.

Previously, the behaviour of `run` and `runSync` were inconsistent with
regards to the treatment of the `stdoutEncoding` (similarly,
`stderrEncoding`) parameters:

`run`:
* if the encoding was null, `ProcessResult.stdout` was returned as a
  String in UTF-8 encoding. This was incorrect. The behaviour as
  specified in `ProcessResult.stdout` is that in this case, a raw
  `List<int>` should be returned.
* If the encoding was unspecified, `ProcessResult.stdout` was returned as
  a `String` in the `io.systemEncoding` encoding. This was correct.
* If the encoding was non-null, `ProcessResult.stdout` was returned as a
  `String` in the specified encoding. This was correct.

`runSync`:
* if the encoding was null, `ProcessResult.stdout` was returned as a
  `List<int>` in UTF-8 encoding. This was incorrect. The behaviour as
  specified in `ProcessResult.stdout` is that in this case, a raw
  `List<int>` should be returned.
* If the encoding was unspecified, `ProcessResult.stdout` was returned as
  `List<int>` in UTF-8 encoding. This was incorrect. The behaviour as
  specified in `ProcessResult.stdout` is that in this case, a String a
  `String` in the `io.systemEncoding` encoding should be returned.
* if the encoding was non-null, `ProcessResult.stdout` was returned as a
  `String` in unknown (but probably UTF-8) encoding. This was incorrect.
  The behaviour as specified in `ProcessResult.stdout` is that in this
  case, a `String` in the specified encoding should be returned.

`_FakeProcess`, from which we obtain the fake stdout and stderr values now
holds these fields as raw `List<int>` of bytes rather than as `String`s. It
is up to the user to supply values that can be decoded with the encoding
passed to `run`/`runAsync`.

`run` and `runAsync` have been updated to set stdout (likewise, stderr) as
specified in the `ProcessResult` documentation.

This is pre-factoring for #102451, in which the tool throws an exception
when processing the JSON output from stdout of the `vswhere.exe` tool,
whose output was found to include the `U+FFFD` Unicode replacement
character during UTF-8 decoding, which triggers a `toolExit` exception
when decoded using our [Utf8Decoder][decoder] configured with `reportErrors` =
true. Because `FakeProcessManager.runAsync` did not previously invoke
`utf8.decode` on its output (behaviour which differs from the non-fake
implementation), it was impossible to write tests to verify the fix.

Ref: https://api.flutter.dev/flutter/dart-io/ProcessResult/stdout.html

Issue: #102451

[decoder]: https:/flutter/flutter/blob/fd312f1ccff909fde28d2247a489bf210bbc6c48/packages/flutter_tools/lib/src/convert.dart#L51-L60

* Refactor vswhere.exe integration (#104133)

`VisualStudio` calls `vswhere.exe` to find Visual Studio installations and determine if they satisfy Flutter's requirements. Previously, `VisualStudio` stored the JSON output from `vswhere.exe` as `Map`s, resulting in duplicated logic to read the JSON output (once to validate values, second to expose values). Also, `VisualStudio` stored two copies of the JSON output (the latest valid installation as well as the latest VS installation).

This change simplifies `VisualStudio` by introducing a new `VswhereDetails`. This type contains the logic to read `vswhere.exe`'s JSON output, and, understand whether an installation is usable by Flutter. In the future, this `VswhereDetails` type will be used to make Flutter doctor resilient to bad UTF-8 output from `vswhere.exe`.

Part of #102451.

* Ignore replacement characters from vswhere.exe output (#104284)

Flutter uses `vswhere.exe` to find Visual Studio installations and determine if they satisfy Flutter's requirements. However, `vswhere.exe`'s JSON output is known to contain bad UTF-8. This change ignores bad UTF-8 as long as they affect JSON properties that are either unused, or, used only for display purposes by Flutter.

Fixes: #102451

Co-authored-by: Chris Bracken <[email protected]>
Co-authored-by: Kevin Chisholm <[email protected]>
camsim99 pushed a commit to camsim99/flutter that referenced this pull request Aug 10, 2022
Adds a bit more clarifying documentation to the implementation of the
outputFollowsExit case, and adds tests that verify the behaviour of
stderr, stdout of processes launched via FakeProcessManager.

Specifically:
* Verifies that stderr, stdout are not emitted immediately after process
  exit if outputFollowsExit is true. They must be emitted at least one
  turn through the event loop later.
* Verifies that ProcessResult.stderr, stdout have the type documented
  according to the encoding passted to Process.run/runSync:
  * List<int> if null is passed as the encoding.
  * String (in the default system encoding) if no encoding is specified.
  * String (in the specified encoding) if an encoding is specified.

This is additional testing relating to refactoring landed in:
flutter#103947

Issue: flutter#102451
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 30, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Aug 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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.

3 participants