forgetest: the lint counts a host test as run only by a step every run of the workflow runs, on push or pull request

A HOST_TESTED row names a workflow and a host test, and the coverage
lint judges the row by what the workflow's steps run. The parser stored
a step's `if:` and `continue-on-error:` and read neither, read no job
key, and read no `on:`. So a step under `if: false`, a step whose
failure the run tolerates, a job under an `if:`, and a workflow on
`workflow_dispatch` alone all counted as running the test, and a
checkout under a condition counted as a checkout: a disabled or
tolerated host test keeps its row green and its paths covered.

The lint now reads the workflow's events and the conditions of a step
and of its job. `workflow_triggers` returns the events of the `on:`
mapping, list or scalar. What is under an event (its branches, its
paths, a dispatch's inputs) is not an event, and a flow mapping names
none. A row's workflow must run on push or pull_request, or the lint
reports that it runs on no push or pull request: a dispatch or a
schedule alone runs on no change, and a call alone is listed as well,
since the lint does not follow it to its caller. `workflow_steps`
tracks the job
each step is in and carries the job's `if:` and `continue-on-error:`
on the step as `job`. `_on_every_run` counts a step only when neither
it nor its job carries an `if:` other than the literal `true` or a
`continue-on-error:` other than the literal `false`. `workflow_runs`
and `checkout_path` use it, so a conditional step runs nothing and a
conditional checkout is no checkout. The reason for such a step stays
"does not run it". `needs:` is not read, nor are the `paths:` and
`branches:` filters of an event: the lint stays a floor there.

Proof: forgetest-ci "Unit tests" (`python -B -m unittest discover -s
tests -v` in forgetest): test_manifest.py runs 36 tests, OK, with the
two new cases, test_a_step_counts_only_when_every_run_of_the_workflow_runs_it_and_its_failure_fails_the_run
and test_the_events_a_workflow_runs_on, and with
test_the_workflow_has_to_run_the_host_test extended by a dispatch-only
workflow and a tolerated step. Negative controls on scratch copies:
`_on_every_run` forced to return True fails the step case and the
host-test case (failures=2); the trigger check deleted from
host_row_problems fails the host-test case (failures=1). The coverage
lint on the tree manifest from the recipe pins reports 0 uncovered
paths, 0 host-tested row problems, 43 tests. pyflakes reports no
undefined name.

Acceptance: none. forgetest is dev-only and in no fingerprint; the
lint runs in CI, not in a campaign.

forgetest/forgetest/manifest.py

@@ -118,12 +118,14 @@ BEHAVIORAL = [
 # the manifest (its tests/ and .github/ files are listed there), or
 # "forgefirm" for this repository (the null-sink harnesses under
 # scripts/bench/, which grblhal-glowforge's workflow runs). The workflow
-# must run the host test: name it, or discover it (a glob, unittest
+# must run the host test: on push or pull request, in a step that runs on
-# discover, pytest on its directory). A host-tested path counts as covered
+# every run of the workflow and whose failure fails it (no `if:` and no
-# in the coverage lint, and like every path no test covers it is in no
+# `continue-on-error:` on the step or its job), and name it, or discover it
-# fingerprint: a change there makes no acceptance run necessary. A path a
+# (a glob, unittest discover, pytest on its directory). A host-tested path
-# catalog test exercises on the hardware belongs in that test's covers, not
+# counts as covered in the coverage lint, and like every path no test
-# here.
+# covers it is in no fingerprint: a change there makes no acceptance run
+# necessary. A path a catalog test exercises on the hardware belongs in
+# that test's covers, not here.
 HOST_TESTED = [
     # An extension package's holds as the engine reads them (forgeext's
     # holdkeep_test and run_test prove the writer's side).
@@ -417,26 +419,97 @@ _STEP_KEYS = ("name", "uses", "run", "id", "if", "with", "env", "working-directo
               "continue-on-error", "timeout-minutes")
 _STEP_RX = re.compile(r"^(\s*)-\s+(%s)\s*:" % "|".join(re.escape(k) for k in _STEP_KEYS))
 _KEY_RX = re.compile(r"^(\s*)([\w-]+)\s*:\s*(.*)$")
+_ON_RX = re.compile(r"""^(?:on|"on"|'on')\s*:\s*(.*)$""")
+# What a job or a step says about whether it runs on every run of the
+# workflow, and whether its failure fails the run.
+_CONDITION_KEYS = ("if", "continue-on-error")
 
 
 def _indent(line):
     return len(line) - len(line.lstrip(" "))
 
 
-def workflow_steps(text):
+def _lines(text):
-    """A workflow's steps, each a dict of its keys: "uses", "run" (the
+    """A workflow's lines with the comments dropped: whole comment lines,
-    command text, a block's lines joined), "working-directory", and "with"
+    and from ' #' to the end of a line."""
-    (a dict). YAML read by its indentation, as far as these workflows use
+    out = []
-    it; comments, whole lines and from ' #' to the end of a line, are not
-    read. Anything outside a step (a trigger's paths, a job's name) is not
-    a step."""
-    lines = []
     for ln in text.splitlines():
         if ln.lstrip().startswith("#"):
             continue
-        lines.append(re.sub(r"\s+#.*$", "", ln).rstrip())
+        out.append(re.sub(r"\s+#.*$", "", ln).rstrip())
+    return out
+
+
+def _scalar(val):
+    """A key's value, a quoted scalar without its quotes (a quote inside a
+    command stays)."""
+    val = val.strip()
+    if len(val) > 1 and val[0] == val[-1] and val[0] in "'\"":
+        return val[1:-1]
+    return val
+
+
+def workflow_triggers(text):
+    """The events a workflow runs on: the keys of its `on:` mapping, the
+    items of its list (`on: [push, pull_request]`, or one per line), or
+    its one scalar (`on: push`). What is under an event (its branches,
+    its paths, a dispatch's inputs) is not an event. A flow mapping
+    (`on: {push: ...}`) is not read, and names no event."""
+    lines = _lines(text)
+    for i, ln in enumerate(lines):
+        m = _ON_RX.match(ln) if not _indent(ln) else None
+        if not m:
+            continue
+        val = m.group(1).strip()
+        if val.startswith("["):
+            return {_scalar(x) for x in val.strip("[]").split(",") if x.strip()}
+        if val:
+            return set() if val.startswith("{") else {_scalar(val)}
+        out, at = set(), None
+        for child in lines[i + 1:]:
+            if not child.strip():
+                continue
+            ind = _indent(child)
+            if ind == 0:
+                break
+            at = ind if at is None else at
+            if ind != at:
+                continue
+            body = child.strip()
+            if body.startswith("- "):
+                out.add(_scalar(body[2:]))
+            else:
+                km = _KEY_RX.match(child)
+                if km:
+                    out.add(km.group(2))
+        return out
+    return set()
+
+
+def workflow_steps(text):
+    """A workflow's steps, each a dict of its keys: "uses", "run" (the
+    command text, a block's lines joined), "working-directory", "with"
+    (a dict), "if" and "continue-on-error" as written, and "job": the
+    "if" and "continue-on-error" of the job the step is in, when it is
+    in one. YAML read by its indentation, as far as these workflows use
+    it; comments, whole lines and from ' #' to the end of a line, are not
+    read. Anything outside a step (a trigger's paths, a job's name) is not
+    a step."""
+    lines = _lines(text)
     steps, i = [], 0
+    in_jobs, job_at, key_at, job = False, None, None, None
     while i < len(lines):
+        km = _KEY_RX.match(lines[i])
+        if km:                      # a mapping key: which job this is, and what the job itself says
+            ind, key = _indent(lines[i]), km.group(2)
+            if ind == 0:
+                in_jobs, job_at, key_at, job = key == "jobs", None, None, None
+            elif in_jobs and (job_at is None or ind == job_at):
+                job_at, key_at, job = ind, None, {}
+            elif in_jobs and job_at is not None and ind > job_at and (key_at is None or ind == key_at):
+                key_at = ind
+                if key in _CONDITION_KEYS:
+                    job[key] = _scalar(km.group(3))
         m = _STEP_RX.match(lines[i])
         if not m:
             i += 1
@@ -454,9 +527,7 @@ def workflow_steps(text):
             if not km or _indent(body[k]) != key_col:
                 k += 1
                 continue
-            key, val = km.group(2), km.group(3).strip()
+            key, val = km.group(2), _scalar(km.group(3))
-            if len(val) > 1 and val[0] == val[-1] and val[0] in "'\"":
-                val = val[1:-1]             # a quoted scalar, not a quote inside a command
             k += 1
             child = []
             while k < len(body) and (not body[k].strip() or _indent(body[k]) > key_col):
@@ -469,17 +540,31 @@ def workflow_steps(text):
                 step["run"] = "\n".join(x.strip() for x in child) if val[:1] in ("|", ">") else val
             else:
                 step[key] = val
+        if job is not None:
+            step["job"] = job       # the job's own dict: a job key written after its steps counts too
         steps.append(step)
     return steps
 
 
+def _on_every_run(step):
+    """Whether a step runs on every run of its workflow and its failure
+    fails the run: neither it nor its job carries an `if:` other than
+    `true`, or a `continue-on-error:` other than `false`. A step that
+    runs only under a condition, or whose failure the run tolerates,
+    proves nothing about what it runs."""
+    for d in (step, step.get("job") or {}):
+        if d.get("if", "true").lower() != "true" or d.get("continue-on-error", "false").lower() != "false":
+            return False
+    return True
+
+
 def checkout_path(steps, name, own):
     """Where a workflow checks the repository `name` out (its path, "" for
     the workspace), or None when it does not: its own repository is the
     checkout that names no repository, another one the checkout whose
     repository is <owner>/<name>."""
     for s in steps:
-        if not s.get("uses", "").startswith("actions/checkout"):
+        if not _on_every_run(s) or not s.get("uses", "").startswith("actions/checkout"):
             continue
         w = s.get("with", {})
         repo = w.get("repository")
@@ -590,13 +675,16 @@ def workflow_runs(steps, test_path):
     (tests.test_x, tests.test_x.Case.test_one); unittest discover from the
     test's own directory with a pattern its name matches; pytest on the
     test or its own directory. An argument to anything else (echo, a build,
-    an install) does not run the test."""
+    an install) does not run the test. Only a step that runs on every run of
+    the workflow, and that fails the run when it fails, counts: a step under
+    an `if:` other than true, or with a `continue-on-error:` other than
+    false, on the step or on its job, runs nothing."""
     full = posixpath.normpath(test_path)
     stem, ext = posixpath.splitext(posixpath.basename(full))
     compiled = ext in (".c", ".cc", ".cpp")
     for s in steps:
         run = s.get("run")
-        if not run:
+        if not run or not _on_every_run(s):
             continue
         wd = s.get("working-directory") or "."
 
@@ -636,8 +724,10 @@ def host_row_problems(manifest, host=HOST_TESTED, root=REPO_ROOT, sources=SOURCE
     """HOST_TESTED rows that prove nothing: a glob that selects no
     behavioral file of its component (the same defect as an empty covers
     entry), a host test or a workflow that does not exist, a workflow that
-    cannot be read where it should be, one that does not check out the
+    cannot be read where it should be, one that runs on no push or pull
-    test's repository, or one with no step that runs the test. Whether a
+    request (a dispatch, a call or a schedule runs on no change), one that
+    does not check out the test's repository, or one with no step that
+    runs the test on every run (workflow_runs). Whether a
     workflow runs its test is not checked where it cannot be read (see
     read_source). Returns [(component, glob, host test, why)]."""
     out = []
@@ -655,6 +745,9 @@ def host_row_problems(manifest, host=HOST_TESTED, root=REPO_ROOT, sources=SOURCE
             continue
         if text is None:
             continue
+        if not workflow_triggers(text) & {"push", "pull_request"}:
+            out.append((comp, pat, ref, "%s runs on no push or pull request" % ci))
+            continue
         steps = workflow_steps(text)
         test_repo, _sep, test_path = ref.partition(":")
         prefix = checkout_path(steps, repo_name(manifest, test_repo), test_repo == ci.partition(":")[0])

forgetest/tests/test_manifest.py

@@ -242,6 +242,18 @@ class CoverageReportTests(unittest.TestCase):
         self.assertEqual(m.host_row_problems(man, [runs], root=root, sources=None), [])
         self.assertEqual(m.host_row_problems(man, [idle], root=root, sources=None),
                          [(idle[0], idle[1], idle[2], ci + " does not run it")])
+        # a workflow that runs on no push or pull request runs it on no change, and a step whose
+        # failure the run tolerates does not run it
+        job = "jobs:\n  test:\n    steps:\n      - uses: actions/checkout@v4\n      - run: python3 scripts/bench/stream_test.py\n"
+        root = self._root({"scripts/bench/stream_test.py": "",
+                           ".github/workflows/manual.yml": "on:\n  workflow_dispatch:\n" + job,
+                           ".github/workflows/soft.yml": "on: [push]\n" + job.replace(
+                               "      - run:", "      - continue-on-error: true\n        run:")})
+        manual = runs[:3] + ("forgefirm:.github/workflows/manual.yml",)
+        soft = runs[:3] + ("forgefirm:.github/workflows/soft.yml",)
+        self.assertEqual(m.host_row_problems(man, [manual, soft], root=root, sources=None),
+                         [manual[:3] + (manual[3] + " runs on no push or pull request",),
+                          soft[:3] + (soft[3] + " does not run it",)])
 
     def _runs(self, yml, path):
         return m.workflow_runs(m.workflow_steps(yml), path)
@@ -305,6 +317,48 @@ class CoverageReportTests(unittest.TestCase):
         self.assertFalse(self._runs(step % "python -m unittest -k test_feeder tests.test_lens",
                                     "tests/test_feeder.py"))
 
+    def test_a_step_counts_only_when_every_run_of_the_workflow_runs_it_and_its_failure_fails_the_run(self):
+        step = "steps:\n  - name: x\n%s    run: ./build/tray_test\n"
+        test = "tests/tray_test.c"
+        # a step under an if: runs it on some runs, not every run; literally true is no condition
+        self.assertFalse(self._runs(step % "    if: false\n", test))
+        self.assertFalse(self._runs(step % "    if: github.event_name == 'push'\n", test))
+        self.assertFalse(self._runs(step % "    if: ${{ always() }}\n", test))
+        self.assertTrue(self._runs(step % "    if: true\n", test))
+        self.assertTrue(self._runs(step % "    if: 'true'\n", test))
+        # a step whose failure the run tolerates proves nothing; literally false is no tolerance
+        self.assertFalse(self._runs(step % "    continue-on-error: true\n", test))
+        self.assertFalse(self._runs(step % "    continue-on-error: ${{ inputs.soft }}\n", test))
+        self.assertTrue(self._runs(step % "    continue-on-error: false\n", test))
+        # the job's own if: or continue-on-error: holds for each of its steps, wherever it is
+        # written, over a step's own, and not for another job's steps
+        job = "jobs:\n  t:\n%s    steps:\n      - run: ./build/tray_test\n%s"
+        self.assertFalse(self._runs(job % ("    if: github.ref == 'refs/heads/main'\n", ""), test))
+        self.assertFalse(self._runs(job % ("", "    if: false\n"), test))
+        self.assertFalse(self._runs(job % ("    continue-on-error: true\n", ""), test))
+        self.assertFalse(self._runs("jobs:\n  t:\n    continue-on-error: true\n    steps:\n"
+                                    "      - continue-on-error: false\n        run: ./build/tray_test\n", test))
+        self.assertTrue(self._runs(job % ("    if: true\n    runs-on: ubuntu-latest\n", ""), test))
+        self.assertTrue(self._runs(job % ("    permissions:\n      contents: read\n", ""), test))
+        two = "jobs:\n  a:\n    if: false\n    steps:\n      - run: %s\n  b:\n    steps:\n      - run: %s\n"
+        self.assertTrue(self._runs(two % ("make", "./build/tray_test"), test))
+        self.assertFalse(self._runs(two % ("./build/tray_test", "make"), test))
+        # a checkout under a condition is no checkout
+        yml = "steps:\n  - uses: actions/checkout@v4\n%s    with:\n      path: own\n"
+        self.assertEqual(m.checkout_path(m.workflow_steps(yml % ""), "x", True), "own")
+        self.assertIsNone(m.checkout_path(m.workflow_steps(yml % "    if: false\n"), "x", True))
+
+    def test_the_events_a_workflow_runs_on(self):
+        self.assertEqual(m.workflow_triggers("on:\n  push:\n    branches: [main]\n  pull_request:\njobs:\n  t:\n"),
+                         {"push", "pull_request"})
+        self.assertEqual(m.workflow_triggers("on: [push, pull_request]\n"), {"push", "pull_request"})
+        self.assertEqual(m.workflow_triggers("on: push\n"), {"push"})
+        self.assertEqual(m.workflow_triggers("on:\n  - push\n  - workflow_dispatch\n"), {"push", "workflow_dispatch"})
+        self.assertEqual(m.workflow_triggers("\"on\":\n  workflow_dispatch:\n    inputs:\n      push:\n"
+                                             "        type: string\n"), {"workflow_dispatch"})
+        self.assertEqual(m.workflow_triggers("on:\n  workflow_call:\n"), {"workflow_call"})
+        self.assertEqual(m.workflow_triggers("name: x\njobs:\n  t:\n    steps: []\n"), set())
+
     def test_the_lint_says_when_it_could_not_read_a_workflow(self):
         import contextlib
         import io
@@ -366,7 +420,7 @@ class CoverageReportTests(unittest.TestCase):
         def blob(text):
             return subprocess.run(["git", "hash-object", "-w", "--stdin"], cwd=cache, input=text.encode(),
                                   capture_output=True, check=True).stdout.decode().strip()
-        yml = "steps:\n  - uses: actions/checkout@v4\n  - run: ./build/tray_test\n"
+        yml = "on: [push, pull_request]\nsteps:\n  - uses: actions/checkout@v4\n  - run: ./build/tray_test\n"
         comps = {"forgectrl": {"srcrev": "aaa", "source": url, "files": [
             ["src/tray.c", "1" * 40], ["tests/tray_test.c", "2" * 40], ["tests/lens_test.c", "3" * 40],
             [".github/workflows/build.yml", blob(yml)], [".github/workflows/lost.yml", "4" * 40]]}}