Skip to content

Test and code hygiene after #94: vacuous tests, duplicated preamble #97

Description

@nojaf

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.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions