You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Housekeeping items noticed while reviewing #94, all of them about that PR rather than anything older.
Two of the three new tests can pass without asserting anything
RunCommandCaptureAll should return exit code, stdout and stderr and RunSensitiveCommandCaptureAll should work put their assertions inside platform-guarded stages but are not wrapped in shouldBeCalled. RunCommandCaptureAll should not fail the stage on a non zero exit code uses the same two-stage shape and does wrap it, threading call () into both branches, which is exactly the guard the other two are missing. A pipeline whose stages are all skipped completes successfully, so a condition regression would leave those two green while the code under test never runs.
Also, RunSensitiveCommandCaptureAll should work asserts only exit code, stdout and stderr, which all go through the same path as the non-sensitive member. Nothing asserts the masking, which is the one thing that member does differently. That is why the stderr leak in #95 is uncaught.
The command preamble now exists four times
RunCommand, RunCommandCaptureOutput, RunSensitiveCommandCaptureOutput and the new RunCommandCaptureAllInternal each rebuild the step prefix, print the command and call Process.StartAsync with argument-for-argument identical parameters. The stderr fix in #94 landed in one place only because StartAsync is shared; the preamble is not, so the next change to prefix rendering or print suppression will reach some copies and not others.
Expressing the three existing members on top of RunCommandCaptureAllInternal (giving it an optional captureOutput, since RunCommand deliberately does not capture and would otherwise lose the child's colours) would remove roughly 60 lines. The Array.create commandStr.ArgumentCount "*" masking idiom is likewise now in two places, and would be better as one private helper.
demo.fsx is missing the new example
The usage example was added to README.md but not to demo.fsx, which is the line-for-line runnable twin of that same block and the only copy that is type-checked against the built dll.
Edited to drop two items:
"Add RunCommandCaptureAll #94 published nothing, and CI went green anyway." On reflection this is not a bug. Every version heading in this changelog's history was added by the maintainer, never by an outside contributor, and with a breaking change in the mix the next version number is a maintainer decision. Leaving the entries under ## [Unreleased] is the correct thing for a contributor to do, and "on master but not on NuGet" is just the normal waiting state.
"Rename encryptiedStr to maskedStr." Moot once the preamble is shared, since that local stops existing.
One thing worth raising separately: the PR workflow runs only ubuntu-latest, so the whenWindows halves of these tests and their \r\n expectations have never executed on CI.
Housekeeping items noticed while reviewing #94, all of them about that PR rather than anything older.
Two of the three new tests can pass without asserting anything
RunCommandCaptureAll should return exit code, stdout and stderrandRunSensitiveCommandCaptureAll should workput their assertions inside platform-guarded stages but are not wrapped inshouldBeCalled.RunCommandCaptureAll should not fail the stage on a non zero exit codeuses the same two-stage shape and does wrap it, threadingcall ()into both branches, which is exactly the guard the other two are missing. A pipeline whose stages are all skipped completes successfully, so a condition regression would leave those two green while the code under test never runs.Also,
RunSensitiveCommandCaptureAll should workasserts only exit code, stdout and stderr, which all go through the same path as the non-sensitive member. Nothing asserts the masking, which is the one thing that member does differently. That is why the stderr leak in #95 is uncaught.The command preamble now exists four times
RunCommand,RunCommandCaptureOutput,RunSensitiveCommandCaptureOutputand the newRunCommandCaptureAllInternaleach rebuild the step prefix, print the command and callProcess.StartAsyncwith argument-for-argument identical parameters. The stderr fix in #94 landed in one place only becauseStartAsyncis shared; the preamble is not, so the next change to prefix rendering or print suppression will reach some copies and not others.Expressing the three existing members on top of
RunCommandCaptureAllInternal(giving it an optionalcaptureOutput, sinceRunCommanddeliberately does not capture and would otherwise lose the child's colours) would remove roughly 60 lines. TheArray.create commandStr.ArgumentCount "*"masking idiom is likewise now in two places, and would be better as one private helper.demo.fsx is missing the new example
The usage example was added to
README.mdbut not todemo.fsx, which is the line-for-line runnable twin of that same block and the only copy that is type-checked against the built dll.Edited to drop two items:
## [Unreleased]is the correct thing for a contributor to do, and "on master but not on NuGet" is just the normal waiting state.encryptiedStrtomaskedStr." Moot once the preamble is shared, since that local stops existing.One thing worth raising separately: the PR workflow runs only
ubuntu-latest, so thewhenWindowshalves of these tests and their\r\nexpectations have never executed on CI.