[flutter_plugin_tools] Improve process mocking #4254
Conversation
The mock process runner used in most of the tests had poor handling of
stdout/stderr:
- By default it would return the `List<int>` output from the mock
process, which was never correct since the parent process runner
interface always sets encodings, thus the `dynamic` should always be
of type `String`
- The process for setting output on a `MockProcess` was awkward, since
it was based on a `Stream<Lint<int>>`, even though in every case what
we actually want to do is just set the full output to a string.
- A hack was at some point added (presumably due to the above isssues)
to bypass that flow at the process runner level, and instead return a
`String` set there. That meant there were two ways of setting output
(one of which that only worked for one of the ways of running
processes)
- That hack wasn't updated when the ability to return multiple mock
processes instead of a single global mock process was added, so the
API was even more confusing, and there was no way to set different
output for different processes.
This changes the test APIs so that:
- `MockProcess` takes stdout and stderr as strings, and internally
manages converting them to a `Stream<List<int>>`.
- `RecordingProcessRunner` correctly decodes and returns the output
streams when constructing a process result.
It also removes the resultStdout and resultStderr hacks, as well as the
legacy `processToReturn` API, and converts all uses to the new
structure, which is both simpler to use, and clearly associates output
with specific processes.
stuartmorgan-g
force-pushed
the
tool-mock-process-standardization
branch
from
efc262d to
8545057
Compare
| @@ -94,8 +94,9 @@ void main() { | |||
| } | |||
| }; | |||
|
|
|||
| processRunner.processToReturn = MockProcess.succeeding(); | |||
| processRunner.resultStdout = jsonEncode(devices); | |||
| processRunner.mockProcessesForExecutable['xcrun'] = <io.Process>[ | |||
There was a problem hiding this comment.
Wow, there was no specification for xcrun previously? Nice!
There was a problem hiding this comment.
mockProcessesForExecutable is something I added fairly recently; previously there was just processToReturn which was used for every call made during that test. That meant it was just impossible to do things like test the case where the first thing a command ran succeeded and the second failed. Or where the output of two processes was used in the same command. All tests that did process mocking looked like this. Unsurprisingly, we had a ton of test coverage holes as a result. (And it was really hard to follow what process mock results were intended to be for.)
When I added it (to enable writing a bunch of new tests that we should have had in the first place) I didn't go back and clean up all the old cases because I wanted to limit the scope of the PR; this is finally doing that cleanup.
| MockProcess(), // auth | ||
| MockProcess(), // config |
There was a problem hiding this comment.
In the future maybe we could make the mock process a bit smarter about arguments too so we aren't relying on the order in which things are called. This still seems a bit fragile.
There was a problem hiding this comment.
I'm torn on this; on one hand it's somewhat fragile, on the other the flutter/flutter tests used to do process simulation based on mock argument matching and it was really painful to get right sometimes, because you had to have correct matchers for a bunch of stuff you probably didn't care about (and it's a different kind of fragile because then, say, re-ordering flags that are irrelevant to the test is test-breaking, unless you have a wildcard system, but wildcards in mock tooling are usually their own can of worms as Mockito demonstrated during the NNBD tranistion). I believe flutter/flutter switched to something order-based later.
I want to revisit it at some point (having to use comments in cases like this was a bit of a red flag), but I also want to be careful not to make something that actually makes writing tests substantially harder.
script/tool/test/mocks.dart
Outdated
| MockProcess.failing() { | ||
| exitCodeCompleter.complete(1); | ||
| } | ||
| MockProcess.failing() : this(exitCode: 1); |
There was a problem hiding this comment.
nit: I would inline this function and get rid of it. It seems to suggest output and exitCode != 0 are mutually exclusive. What you did with the MockProcess constructor is better.
There was a problem hiding this comment.
Done. I liked it because it made it clear we didn't care what the exit code was, just that it was non-zero, but as you point out the inconsistency with failing-with-output isn't great (although the latter is comparatively very rare).
| processRunner.mockProcessesForExecutable['xcrun'] = <io.Process>[ | ||
| noTestsProcessResult, | ||
| // Exit code 66 from testing indicates no tests. | ||
| MockProcess(exitCode: 66), |
There was a problem hiding this comment.
nit: It would be nice if we could link to some documentation that says 66 means this. I'm not sure looking at the text output would be less fragile.
There was a problem hiding this comment.
I don't have a reference on that; I didn't write the xctest logic. This is just adjusting the way that exit code is being returned, not the use of the exit code or the comment.
| // Simulate a project with unit tests but no integration tests... | ||
| processRunner.resultStdout = '{"project":{"targets":["RunnerTests"]}}'; | ||
| const String projectListOutput = | ||
| '{"project":{"targets":["RunnerTests"]}}'; |
There was a problem hiding this comment.
nit: Doesn't Dart have a function that can turn Dart data structures to json strings?
The mock process runner used in most of the tests had poor handling of
stdout/stderr:
- By default it would return the `List<int>` output from the mock
process, which was never correct since the parent process runner
interface always sets encodings, thus the `dynamic` should always be
of type `String`
- The process for setting output on a `MockProcess` was awkward, since
it was based on a `Stream<Lint<int>>`, even though in every case what
we actually want to do is just set the full output to a string.
- A hack was at some point added (presumably due to the above issues)
to bypass that flow at the process runner level, and instead return a
`String` set there. That meant there were two ways of setting output
(one of which that only worked for one of the ways of running
processes)
- That hack wasn't updated when the ability to return multiple mock
processes instead of a single global mock process was added, so the
API was even more confusing, and there was no way to set different
output for different processes.
This changes the test APIs so that:
- `MockProcess` takes stdout and stderr as strings, and internally
manages converting them to a `Stream<List<int>>`.
- `RecordingProcessRunner` correctly decodes and returns the output
streams when constructing a process result.
It also removes the resultStdout and resultStderr hacks, as well as the
legacy `processToReturn` API, and converts all uses to the new
structure, which is both simpler to use, and clearly associates output
with specific processes.
The mock process runner used in most of the tests had poor handling of
stdout/stderr:
- By default it would return the `List<int>` output from the mock
process, which was never correct since the parent process runner
interface always sets encodings, thus the `dynamic` should always be
of type `String`
- The process for setting output on a `MockProcess` was awkward, since
it was based on a `Stream<Lint<int>>`, even though in every case what
we actually want to do is just set the full output to a string.
- A hack was at some point added (presumably due to the above issues)
to bypass that flow at the process runner level, and instead return a
`String` set there. That meant there were two ways of setting output
(one of which that only worked for one of the ways of running
processes)
- That hack wasn't updated when the ability to return multiple mock
processes instead of a single global mock process was added, so the
API was even more confusing, and there was no way to set different
output for different processes.
This changes the test APIs so that:
- `MockProcess` takes stdout and stderr as strings, and internally
manages converting them to a `Stream<List<int>>`.
- `RecordingProcessRunner` correctly decodes and returns the output
streams when constructing a process result.
It also removes the resultStdout and resultStderr hacks, as well as the
legacy `processToReturn` API, and converts all uses to the new
structure, which is both simpler to use, and clearly associates output
with specific processes.
The mock process runner used in most of the tests had poor handling of
stdout/stderr:
List<int>output from the mockprocess, which was never correct since the parent process runner
interface always sets encodings, thus the
dynamicshould always beof type
StringMockProcesswas awkward, sinceit was based on a
Stream<Lint<int>>, even though in every case whatwe actually want to do is just set the full output to a string.
to bypass that flow at the process runner level, and instead return a
Stringset there. That meant there were two ways of setting output(one of which that only worked for one of the ways of running
processes)
processes instead of a single global mock process was added, so the
API was even more confusing, and there was no way to set different
output for different processes.
This changes the test APIs so that:
MockProcesstakes stdout and stderr as strings, and internallymanages converting them to a
Stream<List<int>>.RecordingProcessRunnercorrectly decodes and returns the outputstreams when constructing a process result.
It also removes the resultStdout and resultStderr hacks, as well as the
legacy
processToReturnAPI, and converts all uses to the newstructure, which is both simpler to use, and clearly associates output
with specific processes.
Replace this paragraph with a description of what this PR is changing or adding, and why. Consider including before/after screenshots.
List which issues are fixed by this PR. You must list at least one issue.
If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.
Pre-launch Checklist
dart format.)[shared_preferences]///).If you need help, consider asking for advice on the #hackers-new channel on Discord.