206 lines
9.8 KiB
Markdown
206 lines
9.8 KiB
Markdown
|
|
---
|
||
|
|
paths:
|
||
|
|
- "nodes/*/src/**"
|
||
|
|
- "nodes/*/test/**"
|
||
|
|
- "nodes/*/examples/**"
|
||
|
|
---
|
||
|
|
|
||
|
|
# Output-Coverage Rule — Every Output, Every State, Every Layer
|
||
|
|
|
||
|
|
## Why this rule exists
|
||
|
|
|
||
|
|
On 2026-05-14 the machineGroupControl dashboard crashed the FlowFuse `ui-chart` with
|
||
|
|
`Cannot read properties of null (reading 'y')`. Cause: a 17-output fan-out function
|
||
|
|
where 16 outputs used a safe helper that returned `null` (drop the msg) when its source
|
||
|
|
field was missing, and **one** output was hand-written to emit `{ topic: …, payload: null }`
|
||
|
|
instead. The chart received a msg with a literal null payload, and crashed.
|
||
|
|
|
||
|
|
The bug pattern is generic:
|
||
|
|
|
||
|
|
> "I tested the populated state by watching the dashboard. The empty / pre-first-tick /
|
||
|
|
> degraded state was never tested for that one output."
|
||
|
|
|
||
|
|
The class of bug repeats anywhere a node has many outputs (Port 0 keys, dashboard widgets,
|
||
|
|
function-node ports, InfluxDB fields). The mitigation is process, not vigilance: **every
|
||
|
|
output must be enumerated, and every output must be exercised by a test in every state
|
||
|
|
the output can be in.**
|
||
|
|
|
||
|
|
## Scope — what counts as an "output"
|
||
|
|
|
||
|
|
Anything that can deliver a value to a downstream consumer, on any layer:
|
||
|
|
|
||
|
|
| Layer | Output kind | Examples |
|
||
|
|
|---|---|---|
|
||
|
|
| specificClass | Public method return shape | `getOutput()`, `getStatus()`, `getFlattenedOutput()` keys |
|
||
|
|
| specificClass | Event payloads | `emit('stateChange', …)`, `emit('rejected', …)` |
|
||
|
|
| nodeClass → Port 0 | Process-data keys (after delta compression) | `atEquipment_predicted_flow`, `mode`, `relDistFromPeak` |
|
||
|
|
| nodeClass → Port 1 | InfluxDB telemetry fields | every field name written via `outputUtils.formatForInflux` |
|
||
|
|
| nodeClass → Port 2 | Registration / control msgs | `registerChild`, `unregisterChild`, `assetType` |
|
||
|
|
| examples/*.json | Function node output **ports** | each index in `outputs:N` / `wires:[[…]]` |
|
||
|
|
| examples/*.json | Dashboard widget **sources** | every `ui-text`, `ui-chart`, `ui-template`, `ui-gauge`, `ui-switch`, … node that receives a msg |
|
||
|
|
| examples/*.json | Cross-tab `link-out` channels | each `cmd:*` / `evt:*` / `setup:*` channel name |
|
||
|
|
|
||
|
|
If a downstream consumer can pull `.x.y.z` off a msg, `.x.y.z` is an output and must be
|
||
|
|
tested in every state — populated, missing, zero, NaN, negative, very large.
|
||
|
|
|
||
|
|
## The manifest — required artifact per node
|
||
|
|
|
||
|
|
Every node ships `test/_output-manifest.md` (markdown table, source-controlled). The
|
||
|
|
manifest is the single source of truth for "what does this node emit, and where is it
|
||
|
|
tested?"
|
||
|
|
|
||
|
|
```markdown
|
||
|
|
# <nodeName> output manifest
|
||
|
|
|
||
|
|
## Port 0 (process data)
|
||
|
|
|
||
|
|
| Key | Source method | Type | States tested | Test file |
|
||
|
|
|---|---|---|---|---|
|
||
|
|
| mode | nodeClass._buildPort0 | string ('AUTO'|'MAN'|…) | all 4 modes, missing | test/basic/output-port0.test.js |
|
||
|
|
| atEquipment_predicted_flow | specificClass.getFlattenedOutput | number m³/s, null pre-tick | populated, null | test/basic/output-port0.test.js |
|
||
|
|
| relDistFromPeak | … | number 0..1, null when no BEP curve | populated, null | … |
|
||
|
|
|
||
|
|
## Port 1 (InfluxDB telemetry)
|
||
|
|
|
||
|
|
| Field | Source | Type | States tested | Test file |
|
||
|
|
|---|---|---|---|---|
|
||
|
|
| … | … | … | … | … |
|
||
|
|
|
||
|
|
## Port 2 (registration / control plumbing)
|
||
|
|
|
||
|
|
| Topic | Source | Payload shape | States tested | Test file |
|
||
|
|
|---|---|---|---|---|
|
||
|
|
| … | … | … | … | … |
|
||
|
|
|
||
|
|
## Example flow function-node outputs
|
||
|
|
|
||
|
|
For each example flow in examples/, list every function node with N > 1 outputs:
|
||
|
|
|
||
|
|
### examples/02-Dashboard.json :: fn_status_split (outputs: 17)
|
||
|
|
|
||
|
|
| # | Target node | Topic | Payload shape | Populated test | Empty test |
|
||
|
|
|---|---|---|---|---|---|
|
||
|
|
| 0 | ui_txt_mode | — | string | ✔ flow-fixture.test.js | ✔ flow-fixture.test.js |
|
||
|
|
| 10 | ui_chart_flow | 'Flow' | number, or whole msg null | ✔ | ✔ |
|
||
|
|
| 14 | ui_chart_eta | 'η (%)' | number, or whole msg null | ✔ | ✔ |
|
||
|
|
| … |
|
||
|
|
|
||
|
|
## Dashboard widgets
|
||
|
|
|
||
|
|
| Widget id | Source port | Expected msg shape | Crash-safe on null upstream? |
|
||
|
|
|---|---|---|---|
|
||
|
|
| ui_chart_eta | fn_status_split[14] | `{topic:'η (%)', payload:number}` or no-msg | ✔ |
|
||
|
|
| … |
|
||
|
|
```
|
||
|
|
|
||
|
|
## Internal tests — `test/basic/output-*.test.js`
|
||
|
|
|
||
|
|
Every node has at least one test file whose sole job is enumerating outputs. Every key
|
||
|
|
in the Port-0/1/2 manifest above gets:
|
||
|
|
|
||
|
|
1. **A presence test** — the key exists in the relevant getter / formatter output.
|
||
|
|
2. **A populated-state test** — drive the node into a state where the key has a real
|
||
|
|
value; assert type and (where applicable) range.
|
||
|
|
3. **A degraded-state test** — drive the node into a state where the underlying source
|
||
|
|
is missing / pre-tick / NaN. Assert the key is either **absent** or **explicitly null**.
|
||
|
|
Pick one convention per node and stick to it; never let the same key be sometimes
|
||
|
|
absent and sometimes null.
|
||
|
|
|
||
|
|
Pattern:
|
||
|
|
|
||
|
|
```js
|
||
|
|
test('Port 0 emits every manifest key after warm-up', () => { /* … */ });
|
||
|
|
test('Port 0 keys are absent (not null) before first tick', () => { /* … */ });
|
||
|
|
test('Port 0 omits efficiency keys when no BEP curve is configured', () => { /* … */ });
|
||
|
|
```
|
||
|
|
|
||
|
|
## Node-RED flow tests — `test/integration/flow-*.test.js`
|
||
|
|
|
||
|
|
For every example flow under `examples/`, ship a fixture test that loads the JSON and
|
||
|
|
drives it through `node-red-node-test-helper` (or an equivalent harness). The test must:
|
||
|
|
|
||
|
|
1. **Inject an empty msg** (`{payload:{}}`) into the EVOLV node's input. Assert that
|
||
|
|
**every** downstream function node, link-out, and `ui-*` widget either receives
|
||
|
|
nothing OR receives a msg whose payload satisfies the widget's contract (no
|
||
|
|
`payload: null`, no missing required `topic`, no `payload.x` / `payload.y` undefined
|
||
|
|
for scatter charts).
|
||
|
|
2. **Inject a fully-populated msg** matching the node's real Port-0 shape. Assert that
|
||
|
|
**every** downstream consumer receives the expected payload.
|
||
|
|
3. **Inject a degraded msg** (a real-life partial state — e.g. eta missing but flow
|
||
|
|
present). Assert no consumer receives malformed input.
|
||
|
|
|
||
|
|
Helper sketch:
|
||
|
|
|
||
|
|
```js
|
||
|
|
const helper = require('node-red-node-test-helper');
|
||
|
|
const flow = require('../examples/02-Dashboard.json');
|
||
|
|
|
||
|
|
test('no fan-out output ever emits { payload: null }', async () => {
|
||
|
|
await helper.load(allNodes, flow);
|
||
|
|
const taps = wireTapEveryDownstream(helper, 'fn_status_split');
|
||
|
|
helper.getNode('fn_status_split').receive({ payload: {} }); // empty
|
||
|
|
helper.getNode('fn_status_split').receive({ payload: fullFixture }); // populated
|
||
|
|
helper.getNode('fn_status_split').receive({ payload: partial }); // degraded
|
||
|
|
for (const tap of taps) {
|
||
|
|
for (const msg of tap.messages) {
|
||
|
|
assert.notEqual(msg.payload, null, `${tap.id} got payload:null`);
|
||
|
|
if (tap.node.type === 'ui-chart' && tap.node.yAxisProperty?.includes('.')) {
|
||
|
|
const [, prop] = tap.node.yAxisProperty.split('.');
|
||
|
|
assert.ok(msg.payload?.[prop] !== undefined, `${tap.id} missing payload.${prop}`);
|
||
|
|
}
|
||
|
|
}
|
||
|
|
}
|
||
|
|
});
|
||
|
|
```
|
||
|
|
|
||
|
|
`wireTapEveryDownstream` is a shared helper (lives in `test/helpers/flow-taps.js`) that
|
||
|
|
walks `flow.wires` from the named node and installs a recording listener on each target.
|
||
|
|
|
||
|
|
## Static lint pass — `npm run lint:flow-outputs`
|
||
|
|
|
||
|
|
A repo-level script under `tools/lint-flow-outputs.mjs` (single file, no deps) walks every
|
||
|
|
`examples/*.json`, finds every `function` node with `outputs > 1` and:
|
||
|
|
|
||
|
|
1. Cross-checks that the number of `wires` arrays equals `outputs`.
|
||
|
|
2. Parses the `func` source and, for each `return [...]` element, flags any object literal
|
||
|
|
of the form `{ ... payload: <ternary>: null }` or `{ ... payload: null }`. Those must
|
||
|
|
be rewritten as a helper that returns `null` (the whole msg) so the function node
|
||
|
|
skips the output entirely.
|
||
|
|
3. For each `ui-chart` in the flow, verifies the chart has the full required-property set
|
||
|
|
from `.claude/rules/node-red-flow-layout.md` §4 (`interpolation`, `yAxisProperty`,
|
||
|
|
`yAxisPropertyType`, `xAxisType`, `xAxisPropertyType`, …).
|
||
|
|
4. Exits non-zero on any finding. Wired into CI.
|
||
|
|
|
||
|
|
## Verification checklist — when can you declare a node "done"
|
||
|
|
|
||
|
|
Before merging any change that touches a node's outputs (Port 0/1/2 keys, function-node
|
||
|
|
ports, dashboard widgets, telemetry fields):
|
||
|
|
|
||
|
|
- [ ] `_output-manifest.md` is updated for every added / removed / renamed output.
|
||
|
|
- [ ] `test/basic/output-*.test.js` covers every manifest entry in both **populated** and **degraded** states.
|
||
|
|
- [ ] If you touched an example flow: `test/integration/flow-*.test.js` covers empty, populated, and degraded inputs to the flow.
|
||
|
|
- [ ] `npm run lint:flow-outputs` passes (no `payload: null` literals, no missing chart fields).
|
||
|
|
- [ ] Visual smoke: deploy the example flow, open the dashboard, and **before any data flows in** confirm the page loads without errors in the browser console and without exceptions in the Node-RED log.
|
||
|
|
|
||
|
|
## Anti-patterns
|
||
|
|
|
||
|
|
- ❌ Hand-writing one output of an N-output fan-out instead of using the shared helper.
|
||
|
|
If outputs 0-15 use `chart(topic, v, scale)`, output 16 also uses `chart(topic, v, scale)`.
|
||
|
|
No exceptions.
|
||
|
|
- ❌ "I tested the dashboard, it looks right" — visual confirmation of one warm state is
|
||
|
|
not coverage. The degraded / pre-first-tick state is where dashboards crash.
|
||
|
|
- ❌ Emitting `{ payload: null }` from a function node. Either return the whole msg as
|
||
|
|
`null` (the function-node convention for "don't emit on this output") or supply a
|
||
|
|
default that the consumer can render.
|
||
|
|
- ❌ "I'll add the test later" — the manifest entry without a corresponding passing test
|
||
|
|
is a regression vector. Land them together.
|
||
|
|
- ❌ Mixing conventions per node (sometimes a missing field is `null`, sometimes absent).
|
||
|
|
Pick one per node, document it in the manifest, enforce it in the test.
|
||
|
|
|
||
|
|
## Migration plan for existing nodes
|
||
|
|
|
||
|
|
Existing nodes don't have `_output-manifest.md` yet. The rule applies prospectively:
|
||
|
|
**any PR that touches a node's outputs must add or update the manifest for at least the
|
||
|
|
outputs it touched.** A repo-wide backfill (one PR per node, generating the manifest
|
||
|
|
from existing tests + flows) is tracked in `.agents/improvements/IMPROVEMENTS_BACKLOG.md`.
|