Merge pull request #30 from ausimian/issue/8-coverage

test: cover the shipped surface, and make the coverage figure mean something

Author: ausimian

.github/workflows/ci.yml

@@ -39,7 +39,7 @@ jobs:
       run: mix precommit
 
   test:
-    name: Test (Elixir ${{ matrix.elixir }} / OTP ${{ matrix.otp }})
+    name: Test (Elixir ${{ matrix.elixir }} / OTP ${{ matrix.otp }})${{ matrix.cover && ' + coverage floor' || '' }}
     # Only Linux for now. Castle is pure Elixir talking to :release_handler,
     # with no shell scripts of its own and no suite that boots a release, so
     # there is nothing platform-sensitive to run. Add macos-15 alongside the
@@ -60,6 +60,27 @@ jobs:
           # Elixir 1.19 supports Erlang/OTP 26-28
           - elixir: '1.19'
             otp: '29'
+        include:
+          # **The one cell that checks the coverage floor, and the only place in
+          # CI that checks it at all.** `mix precommit` enforces the threshold in
+          # `mix.exs`, but the `precommit` job below is pinned to a single
+          # Elixir, so on its own it says nothing about the rest of the
+          # `~> 1.18` range that `mix.exs` declares - and cover's line
+          # attribution is not constant across that range. Elixir 1.20 counts
+          # one more relevant line in `Castle.Peer` than 1.19 does, which is how
+          # a floor set from the pinned toolchain came to fail a clean tree for
+          # contributors on a current Elixir.
+          #
+          # This is the newest Elixir in the matrix, so it reports the lowest
+          # figure and trips first: a canary for exactly that drift. Every other
+          # cell stays on a plain `mix test` deliberately - seven readings that
+          # disagree by version would be noise, not signal.
+          #
+          # Keep this on whichever cell is newest when the matrix moves on. The
+          # threshold note in `mix.exs` explains what to do if it goes red.
+          - elixir: '1.20'
+            otp: '29'
+            cover: true
 
     steps:
     - name: Checkout code
@@ -91,4 +112,23 @@ jobs:
       run: mix compile --warnings-as-errors
 
     - name: Run tests
+      if: ${{ !matrix.cover }}
       run: mix test
+
+    # Split in two rather than one `mix test --cover`, so that the step which
+    # goes red names the thing that broke without anyone opening the log:
+    # exporting cannot fail on coverage, and checking cannot fail on a test.
+    - name: Run tests (exporting coverage)
+      if: ${{ matrix.cover }}
+      run: mix test --cover --export-coverage ci
+
+    - name: Check the coverage floor
+      # Red here means the total fell under the threshold in `mix.exs` while the
+      # tests passed. Either coverage genuinely dropped, or this Elixir
+      # attributes relevant lines differently from the pinned toolchain - read
+      # the threshold note in `mix.exs` before changing the number, because it
+      # is a floor across the whole supported range and not a reading from one.
+      if: ${{ matrix.cover }}
+      env:
+        MIX_ENV: test
+      run: mix test.coverage

AGENTS.md

@@ -41,6 +41,9 @@ Forecastle 1.x and Elixir 1.18 or later.
   the shared Erlang installation.
 - Give actionable recovery instructions when `:release_handler` booted without
   an accepted `RELEASES` file.
+- Fix the wording when a restart install is refused because something unusual,
+  such as a named pipe, is already at the path Castle uses for its restart
+  marker. The message used to read "a other".
 - Report restart installs without raising `CaseClauseError`.
 - Return an empty release list without raising `Enum.EmptyError`.
 - Report `RELEASES` read and write errors instead of raising `MatchError`.

RELEASE.md

@@ -1260,8 +1260,16 @@ defmodule Castle.Commands do
       "Move whatever is there out of the way."
   end
 
+  # Four of `File.lstat/1`'s five types reach here - `:regular` is the pending
+  # marker and is answered above - and each has to read as a noun phrase in that
+  # sentence. `:other` is the awkward one: a named pipe, a socket, anything the
+  # emulator has no name for, and it went into the shipped message as "a other",
+  # which is what a `"a #{type}"` catch-all does with the one type whose atom is
+  # not a noun. The catch-all stays for `:device` and for whatever OTP adds,
+  # since "a device" reads correctly.
   defp describe_type(:directory), do: "a directory"
   defp describe_type(:symlink), do: "a symbolic link"
+  defp describe_type(:other), do: "something of another kind"
   defp describe_type(other), do: "a #{other}"
 
   defp stale(provisional, vsn, reason, refusal) do

lib/castle/commands.ex

@@ -16,6 +16,7 @@ defmodule Castle.MixProject do
       aliases: aliases(),
       package: package(),
       docs: docs(),
+      test_coverage: test_coverage(),
       source_url: @source_url
     ]
   end
@@ -38,6 +39,85 @@ defmodule Castle.MixProject do
   defp elixirc_paths(:test), do: ["lib", "test/support"]
   defp elixirc_paths(_env), do: ["lib"]
 
+  # `mix test --cover` measures the shipped code, which is `lib` - the modules
+  # under `test/support` are fixtures, and a fixture is covered by being run at
+  # all. Left in, they moved the total without ever being the thing measured, and
+  # two of them moved it *down* for a reason that is not about tests:
+  # `Castle.PeerProviderStub` and most of `Castle.IoSink` execute inside the peer
+  # VM, which nothing instruments - see the threshold below. Their code genuinely
+  # runs and genuinely cannot be observed from here, so the figure they
+  # contributed was an artefact of where they run rather than a gap in the suite,
+  # and raising it would have meant calling them directly on the test node, which
+  # tests nothing.
+  #
+  # Named module by module rather than matched by a pattern. A regex over `Stub`
+  # or over `Castle.*Release` would quietly swallow a production module that
+  # happened to be spelled that way, which is the one thing an exclusion list
+  # must not do. Renaming a fixture makes the total drop, which is visible.
+  #
+  # **The threshold is a floor across the supported range, not this machine's
+  # reading.** `elixir: "~> 1.18"` invites in every version from 1.18 upwards,
+  # and cover's line attribution is not the same across them: 1.18.3 through
+  # 1.19.5 count 473 relevant lines and report 88.58%, while 1.20.3 counts one
+  # more in `Castle.Peer` and reports 88.40%. Same tests, same covered lines, a
+  # different denominator. So this is the *lowest* of those readings, rounded
+  # down, so that it absorbs a line of drift instead of sitting on one version's
+  # figure. The per-toolchain measurements are in AGENTS.md.
+  #
+  # Two earlier values were wrong in opposite directions, and both mistakes are
+  # worth keeping written down. 85 sat *below* the figure it was meant to floor,
+  # so it ratcheted nothing and licensed a thirteen-line regression. 88.58 was
+  # the pinned toolchain's exact reading with no slack, which made the *mandatory*
+  # `mix precommit` fail on a clean tree under Elixir 1.20 - a false failure for
+  # any contributor on a current release, and the comment beside it had already
+  # said attribution varies by version while the number ignored it. So: do not
+  # set this from one machine, and do not raise it to 88.40 or above, which
+  # re-creates the trap the moment another version attributes differently.
+  #
+  # 88 absorbs two uncovered lines added to `lib` and fails on the third, which
+  # was measured on 1.20 (the least slack of the range) rather than estimated -
+  # 88.40, 88.21, 88.03, then 87.84 and exit 3. That is looser than a floor
+  # ideally is, and it is the deliberate price of enough headroom that one more
+  # line of attribution drift does not fail a clean tree. The direction of the
+  # trade is the point: a floor that fires on a clean tree teaches people to
+  # bypass the gate.
+  #
+  # 90% would need 426 covered on 1.19's denominator, seven more than there are.
+  # What is left there is 33 lines in the peer's VM (below) plus 21 observable in
+  # principle: five are the compiler's own default-argument clauses for arities
+  # nothing calls, and the other sixteen need a file mode, a device node, or a
+  # config provider sabotaging Castle's working directory. So the seven would
+  # have to include all five of the default-argument clauses, whose only effect
+  # is on this number. That is the move this project does not make. See AGENTS.md
+  # for the line-by-line account.
+  #
+  # **What cannot be measured is the peer's VM, and the reason is where
+  # instrumentation is applied rather than anything cover cannot do.**
+  # `:cover.start/0` works perfectly well in a VM with no node name -
+  # `is_alive() == false` is no obstacle to it. What happens here is that Mix
+  # starts cover on *this* node and instruments the modules loaded here; the peer
+  # is a separate VM that loads `Castle.Peer` from the target release's own beam
+  # files on disk, which nothing has instrumented. Cover's only mechanism for
+  # another VM is `:cover.start/1` over a *distributed* node, and this peer
+  # deliberately has no distribution at all. So `resolve/1` and everything below
+  # the `## In the peer` comment - 33 lines, about 7% of the shipped total - run
+  # on every `Castle.PeerTest` and are counted as missed, which puts the
+  # observable ceiling near 93%.
+  defp test_coverage do
+    [
+      summary: [threshold: 88],
+      ignore_modules: [
+        Castle.DeploymentStub,
+        Castle.InitStub,
+        Castle.IoSink,
+        Castle.PeerProviderStub,
+        Castle.PeerStub,
+        Castle.ReleaseHandlerStub,
+        Castle.SyntheticRelease
+      ]
+    ]
+  end
+
   # Run "mix help deps" to learn about dependencies.
   defp deps do
     [
@@ -59,7 +139,19 @@ defmodule Castle.MixProject do
         "deps.unlock --unused",
         "format",
         "credo --strict",
-        "test"
+        # With `--cover`, so the threshold in `test_coverage/0` is a gate rather
+        # than decoration: nothing else runs it, and a floor nothing enforces is
+        # a number in a comment.
+        #
+        # **This runs on whatever Elixir the contributor has**, which is the
+        # thing to keep in mind before touching the threshold. CI's `test` matrix
+        # stays on a plain `mix test` and CI's `precommit` job is pinned to one
+        # version, so a floor set from that one version is not checked anywhere
+        # against the rest of the `~> 1.18` range - it is checked here, on a
+        # machine CI never sees, in the gate this project makes mandatory. That
+        # is why the number is a floor across the range and not a reading. See
+        # `test_coverage/0`.
+        "test --cover"
       ]
     ]
   end