Add tests for (3006,004C) and a CI workflow #3

Merged

sjswerdloff merged 3 commits from test/hdss-source-series-tests-and-ci into main

2026-09-01 22:55:01 +02:00

sjswerdloff commented

2026-09-01 10:04:33 +02:00

Collaborator

Adds tests for the (3006,004C) fix in #2, and a CI workflow.

Now green on your runner — run #3, linux-amd64. The first two runs failed and the reason is
worth writing down, because it was in your own workflow and I did not read far enough.

run #1, #2   failed after ~9s
cause        actions/checkout@v4 and actions/cache@v4 are JavaScript actions, and the
             runner executes them INSIDE the job container. python:3.13-slim has no node.
fix          your dicom-gateway ci.yaml's first step, copied:
                 - name: Install Node.js
                   run: apt-get update -q && apt-get install -y --no-install-recommends nodejs
run #3       success

I read dicom-gateway/.forgejo/workflows/ci.yaml to follow your conventions and took four things
from it — .forgejo/workflows, runs-on: linux-amd64, container-based, workflow_dispatch first,
paths-ignore rather than a paths include-list. I did not read its steps, so I missed the one
that makes container jobs run at all under rust:latest for exactly the same reason. Your file had
the answer before I had the problem.

One incidental finding, in case it is useful: a push touching only .forgejo/workflows/**
still triggered the pull_request run, even though that path is in paths-ignore. The filter is
evaluated against the pull request's whole changeset rather than the individual push, which matches
GitHub's semantics.

Tests

Layered, because the layers catch different failures and only the middle one catches the defect
#2 fixed:

layer	what it pins
unit	`_build_structure_set_tags` populates all six Type 1 sub-attributes, and refuses rather than emitting a partial item when one is absent
integration	the dict `DicomExporter.export` returns carries what `RtssHdExporter` reads

The missing-value case is parametrised per key — a single "it raises" test would pass while five of
six branches were dead.

The integration test is the regression guard. The original defect wasn't a wrong value, it was
a value that existed in one component and couldn't be reached from the other. A unit test on either
side passes equally before and after the fix.

Mutation-proved rather than asserted: renaming series_description on the producing side without
updating the consumer fails exactly the integration test, with all 9 unit tests still passing.
Green before, injection verified present, restored byte-identical, green after.

An e2e check — write a phantom, read both objects back, confirm 004C describes the CT series on
disk — is deliberately not here. It costs a phantom generation and belongs on a release gate
rather than per-push CI. Happy to add it to a release.yaml if you'd like one.

CI — two departures, offered rather than assumed. Say the word and either reverts.

concurrency + cancel-in-progress. With a single shared runner, three quick pushes to a
branch queue three full builds and the first two are already obsolete. Costs nothing, changes
nothing about what is tested.
pull_request + push to main, instead of push to every branch. Same protection where it
matters — the merge — without every work-in-progress push consuming the runner. If you build
every branch on purpose, this is the one to revert.

One thing stated plainly rather than buried

Dependencies are installed directly, not via poetry install. The domain layer is Qt-free
(measured: PySide6 appears only under gui/ and main.py), so the tests need numpy, pydicom and
matplotlib. poetry install would also pull pyside6 and nuitka — minutes per run, and pyside6 on a
slim image is a plausible failure with nothing to do with the code under test. The cost: CI is
then not exercising the locked dependency set. The clean fix is upstream of CI — GUI dependencies
as an optional extra, so poetry install without extras gives exactly the domain layer. That's a
change to your dependency structure, so it's a question rather than something done here.

Written by River (river-2fc1630e), collaborating with @sjswerdloff and posting through his
account. The commits carry river-2fc1630e as author.

Adds tests for the (3006,004C) fix in #2, and a CI workflow. **Now green on your runner** — run #3, `linux-amd64`. The first two runs failed and the reason is worth writing down, because it was in your own workflow and I did not read far enough. run #1, #2 failed after ~9s cause actions/checkout@v4 and actions/cache@v4 are JavaScript actions, and the runner executes them INSIDE the job container. python:3.13-slim has no node. fix your dicom-gateway ci.yaml's first step, copied: - name: Install Node.js run: apt-get update -q && apt-get install -y --no-install-recommends nodejs run #3 success I read `dicom-gateway/.forgejo/workflows/ci.yaml` to follow your conventions and took four things from it — `.forgejo/workflows`, `runs-on: linux-amd64`, container-based, `workflow_dispatch` first, `paths-ignore` rather than a `paths` include-list. I did not read its *steps*, so I missed the one that makes container jobs run at all under `rust:latest` for exactly the same reason. Your file had the answer before I had the problem. One incidental finding, in case it is useful: a push touching **only** `.forgejo/workflows/**` still triggered the `pull_request` run, even though that path is in `paths-ignore`. The filter is evaluated against the pull request's whole changeset rather than the individual push, which matches GitHub's semantics. ## Tests Layered, because the layers catch different failures and **only the middle one catches the defect #2 fixed**: | layer | what it pins | |---|---| | unit | `_build_structure_set_tags` populates all six Type 1 sub-attributes, and refuses rather than emitting a partial item when one is absent | | integration | the dict `DicomExporter.export` **returns** carries what `RtssHdExporter` **reads** | The missing-value case is parametrised per key — a single "it raises" test would pass while five of six branches were dead. **The integration test is the regression guard.** The original defect wasn't a wrong value, it was a value that existed in one component and couldn't be reached from the other. A unit test on either side passes equally before and after the fix. Mutation-proved rather than asserted: renaming `series_description` on the producing side without updating the consumer fails **exactly** the integration test, with all 9 unit tests still passing. Green before, injection verified present, restored byte-identical, green after. An **e2e** check — write a phantom, read both objects back, confirm 004C describes the CT series on disk — is deliberately *not* here. It costs a phantom generation and belongs on a release gate rather than per-push CI. Happy to add it to a `release.yaml` if you'd like one. ## CI — two departures, offered rather than assumed. Say the word and either reverts. 1. **`concurrency` + `cancel-in-progress`.** With a single shared runner, three quick pushes to a branch queue three full builds and the first two are already obsolete. Costs nothing, changes nothing about what is tested. 2. **`pull_request` + `push` to `main`**, instead of push to every branch. Same protection where it matters — the merge — without every work-in-progress push consuming the runner. If you build every branch on purpose, this is the one to revert. ## One thing stated plainly rather than buried **Dependencies are installed directly, not via `poetry install`.** The domain layer is Qt-free (measured: PySide6 appears only under `gui/` and `main.py`), so the tests need numpy, pydicom and matplotlib. `poetry install` would also pull pyside6 and nuitka — minutes per run, and pyside6 on a slim image is a plausible failure with nothing to do with the code under test. **The cost: CI is then not exercising the locked dependency set.** The clean fix is upstream of CI — GUI dependencies as an optional extra, so `poetry install` without extras gives exactly the domain layer. That's a change to your dependency structure, so it's a question rather than something done here. --- Written by **River** (`river-2fc1630e`), collaborating with @sjswerdloff and posting through his account. The commits carry `river-2fc1630e` as author.

sjswerdloff added 1 commit

2026-09-01 10:04:33 +02:00

Add tests for (3006,004C) and a CI workflow

CI / test (pull_request) Failing after 17s

Details

0827cbe611

Stacked on the 004C fix, so this branch targets that branch rather than main.

TESTS -- layered, because the layers catch different failures and only the
middle one catches the defect being fixed:

  unit         _build_structure_set_tags populates all six Type 1
               sub-attributes, and refuses rather than emitting a partial
               item when one is absent. The missing-value case is
               parametrised per key: a single "it raises" test would pass
               while five of six branches were dead.
  integration  the dict DicomExporter.export RETURNS carries what
               RtssHdExporter READS. This is the regression guard. The
               original defect was not a wrong value, it was a value that
               existed in one component and could not be reached from the
               other, so a unit test on either side passed both before and
               after the fix.

Mutation-proved rather than asserted. Renaming series_description on the
producing side without updating the consumer fails EXACTLY the integration
test, 9 unit tests still passing -- which demonstrates that the seam guard
works and that unit coverage structurally cannot replace it. Green before,
injection verified, restored byte-identical, green after.

An end-to-end check -- write a phantom, read both objects back, confirm 004C
describes the CT series on disk -- is deliberately NOT here. It costs a
phantom generation and belongs on a release gate rather than per-push CI.

CI -- follows the dicom-gateway house style: .forgejo/workflows,
runs-on: linux-amd64, container-based, workflow_dispatch first, paths-ignore
rather than a paths include-list (the right polarity -- an exclusion list
covers a new source directory by default, an include-list leaves it silently
untested).

Two departures, offered rather than assumed, both reverted on request:

  concurrency + cancel-in-progress   with one shared runner, three quick
                                     pushes queue three builds and the first
                                     two are already obsolete
  pull_request + push to main        instead of push to every branch: same
                                     protection at the merge, without every
                                     WIP push consuming the runner

Dependencies are installed directly rather than via `poetry install`, because
the domain layer is Qt-free (measured: PySide6 appears only under gui/ and
main.py) and pulling pyside6 plus nuitka costs minutes per run and can fail
on a slim image for reasons unrelated to the code. The cost of that choice is
stated in the workflow: CI is then not exercising the locked dependency set.
The clean fix is upstream -- GUI dependencies as an optional extra -- which is
a change to the project's dependency structure and is left as a question.

NOT VERIFIED ON A RUNNER. The YAML parses and the tests pass locally, but
neither of us has a linux-amd64 runner to execute this on, so the first real
run is the first evidence.

sjswerdloff added 2 commits

2026-09-01 14:06:53 +02:00

Merge pull request 'Emit Source Series Information Sequence (3006,004C) in the HD RT Structure Set' (#2 ) from fix/hdss-source-series-information-sequence into main 125f836632

Merge remote-tracking branch 'origin/main' into test/hdss-source-series-tests-and-ci

CI / test (pull_request) Failing after 9s

Details

3158e5bbf4

sjswerdloff changed title from ~~Add tests for (3006,004C) and a CI workflow (stacked on #2)~~ to WIP: Add tests for (3006,004C) and a CI workflow (stacked on #2)

2026-09-01 14:07:04 +02:00

sjswerdloff changed target branch from fix/hdss-source-series-information-sequence to main

2026-09-01 14:07:04 +02:00

sjswerdloff changed title from ~~WIP: Add tests for (3006,004C) and a CI workflow (stacked on #2)~~ to WIP: Add tests for (3006,004C) and a CI workflow

2026-09-01 14:07:36 +02:00

sjswerdloff added 1 commit

2026-09-01 14:37:09 +02:00

CI: install Node.js before the JS actions, as dicom-gateway does

CI / test (pull_request) Successful in 50s

Details

c9f53df821

Run #2 failed after 9 seconds. actions/checkout@v4 and actions/cache@v4 are JavaScript
actions and the runner executes them inside the job container, which for python:3.13-slim
has no node -- the same reason dicom-gateway's ci.yaml opens with this exact step under
rust:latest.

I read that workflow for its triggers, runner label, container style and path polarity,
adopted all four, and did not read its steps. The fix was in the file I had already opened.

Not claimed as diagnosed: the actions log endpoints 404 through the API, so I cannot read
run #2's output. This is the highest-probability cause given the timing and his own first
step, and the next run is the test.

sjswerdloff changed title from ~~WIP: Add tests for (3006,004C) and a CI workflow~~ to Add tests for (3006,004C) and a CI workflow

2026-09-01 14:38:50 +02:00