Repository navigation
[native_toolchain_c] Inject ProcessManager for process spawning - #3531
Conversation
Adds an optional processManager parameter alongside the existing logger parameters and routes all process spawning through it, defaulting to LocalProcessManager. Groundwork for mocked unit tests in the flutter_tools style. Issue: dart-lang#3072
dcharkes
left a comment
dcharkes
left a comment
There was a problem hiding this comment.
After this and the file system change, we should start reorganizing the tests into two subdirectories.
- for integration tests that use the file system and processes - and some of them may only run on some host OSes
- unit tests, the should be blazing fast, and run for every target OS on every host OS.
But that's for a next PR then.
| class FakeProcessManager implements ProcessManager { | ||
| FakeProcessManager({this.result = const ScriptedResult()}); | ||
|
|
||
| final ScriptedResult result; |
There was a problem hiding this comment.
Usually this should be a list of results for each invocation.
For example:
There was a problem hiding this comment.
And with the callback you can also create files on the fake file system that way.
There was a problem hiding this comment.
Thanks for the review! The windows failures look like they're from this change, digging in. Will also rework the fake to take a list of results with a callback like the flutter_tools one.
Spell out where.exe in PathToolResolver: package:process resolves extension-less executables via PATHEXT, which the filtered hook environment does not define, crashing every build hook on Windows. Rework FakeProcessManager to a list of FakeCommands with optional command matching and an onRun callback, per review. Issue: dart-lang#3072
…nager # Conflicts: # pkgs/native_toolchain_c/CHANGELOG.md
|
Reworked the fake to a list of FakeCommands with optional exact command matching and an onRun callback, same shape as the flutter_tools one. The callback should come in handy for faking file output once the FileSystem part lands. The windows failures turned out to be package:process itself: without a shell it resolves extension-less names via PATHEXT, and the filtered hook environment doesn't include PATHEXT, so every hook died on the plain "where" lookup. Spelled it out as where.exe. Adding PATHEXT to the hooks_runner environment filter might be worth a separate issue, happy to file it. |
| Future<Map<String, String>> environmentFromBatchFile( | ||
| Uri batchFile, { | ||
| List<String> arguments = const [], | ||
| required ProcessManager processManager, |
There was a problem hiding this comment.
This is a breaking change as this function is exported.
How about ProcessManager processManager = const LocalProcessManager(),?
There was a problem hiding this comment.
Actually, why did our CI bot not catch this. I would have expected this to go red.
There was a problem hiding this comment.
Done, defaulted to const LocalProcessManager(). Hadn't realized this one was exported, good catch.
On the bot: possibly because 0.19.4 is still -wip, so apitool is diffing against the last published version, where this nets out as non-breaking additions.
| class FakeProcessManager implements ProcessManager { | ||
| FakeProcessManager(this._commands); | ||
|
|
||
| final List<FakeCommand> _commands; |
There was a problem hiding this comment.
Could we catch if we did or did not consume all fake commands in a test?
There was a problem hiding this comment.
Added an allCommandsConsumed getter so tests can assert everything scripted actually ran.
SGTM 👍
|
When using `package:process` it tries to look at `PATHEXT` before doing process invocations. And we're trying to start to use that in `package:native_toolchain_c`. Context: * #3531 (comment)
…ed function, allCommandsConsumed on fake
Description
Most CI time in this repo goes to
package:native_toolchain_candpackage:hooks_runnerintegration tests, because every test spawns real toolchains. #3072 proposes flutter_tools style unit tests where the file system and process calls are mocked. This PR does the process half of that seam.Adds an optional
processManagerparameter next to the existingloggerparameters onCBuilder.run,CLinker.run,CLibrary.build,CLibrary.link, andToolResolvingContext, defaulting toLocalProcessManager. All process spawning in the package now goes throughpackage:processinstead ofdart:ioProcessdirectly. The tool resolvers get their logger viaToolResolvingContext, soprocessManageris threaded the same way. This also resolves thepackage:processhalf of the TODO intool_resolver.dart, thepackage:filehalf stays for the follow-up.Adds a fake
ProcessManagerplus hermetic unit tests forrunProcessandenvironmentFromBatchFilethat assert the invoked command lines without running real processes. Follow-up PRs will do theFileSystemside and grow the mocked test coverage from there.Related Issues
Part of #3072. Not marked
Fixeson purpose, the issue stays open for theFileSystemfollow-up.PR Checklist
dart tool/ci.dart --alllocally and resolved all issues identified. This ensures the PR is formatted, has no lint errors, and ran all code generators. This applies to the packages part of the toplevelpubspec.yamlworkspace.CHANGELOG.mdfor the relevant packages. (Not needed for small changes such as doc typos).