test(render): cover the app-show topology renderer's graph and label branches - #466
Conversation
…branches `src/render/topology.rs` had two tests, both reading a well-formed substrate fixture. Those fixtures have one source, one edge out of each node, no orphans, no loops, and nodes declared in flow order — so every branch that exists for the shapes a hand-written .app can actually have went unexercised. Adds eight hermetic tests over topological_order, node_label and format_dag: fan-out plus an orphan node, a closed edge loop (no source at all), a path that re-enters itself, edge-driven order against declaration order, arrow placement, the three node-label forms, partial row/col coordinates, and the degraded connection line. Deletes linear_topology_lists_nodes_in_flow_order. Its fixture declares the three nodes in the order it asserted, and topological_order falls back to declaration order — so replacing the edge walk with `current = None` left it green. Its residue is covered end-to-end by tests/app_show.rs::shows_welded_to_tc_linear_topology.
|
@codex review Generated by Claude Code |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
A local review panel run against the first commit found defects in the tests it added. Fixes, each re-proven by the mutation that previously survived: - dag_shows_a_grid_position_... asserted 'is not positioned' only for lines it found, so a format_dag that stopped emitting unpositioned nodes left the loop iterating over nothing and the test green. Replacing the '_ => String::new()' arm with '_ => continue' survived the whole suite; it now fails. Presence is asserted before the property, the (None, Some(col)) arm is covered, and the dag header's node count is pinned (len() + 1 also survived before). - New a_connection_naming_an_unknown_node_... covers the None arm of 'by_id.remove(id)' — a typo'd connection id, which no test reached. Replacing the 'if let' with '.expect(..)' passed every other test in the file. - listed() matched '[id]' anywhere, and format_dag renders connection labels in the same brackets, so a node would double-count in any dag with an edge labelled after it. Now anchored to the start of a line. - The inline fixture used 'kind: python', which validate_app rejects (E_APP_INLINE_KIND, only 'predicate' is runnable). The renderer is kind-agnostic, so the valid value costs no coverage. - a_path_that_re_enters_itself_... claimed the seen-guard keeps a node off the list twice. 'by_id.remove' does that; the guard supplies termination. Comment corrected, and the fixture now declares its nodes out of flow order so the added order assertion gives the test a failure mode other than hanging. - Opaque '.unwrap()' on str::find replaced with a panic naming the missing node. - Comment corrections: example apps do contain fan-out and multiple sources (qa-drawings-to-tekla has both), unlabelled dag edges are the common case, and the linear header count comes from app.nodes.len() rather than from the walk.
Round 2 — what changed and whyCodex reviewed Acted on — each was a mutation that previously SURVIVED the whole suite
Comment corrections in the same commit: the example apps do contain fan-out and multiple sources ( While fixing the grid-position test, its new presence assertion immediately caught a flaw in my own fixture: node ids Recorded, deliberately NOT acted on
Gates on
|
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Summary
cli/src/render/topology.rs— the renderer behindaware app show— had two tests, both reading a well-formed substrate fixture. Real.appfixtures have one source, one edge out of each node, no orphans, no loops, and nodes declared in flow order, so every branch that exists for the shapes a hand-written app can actually have was unexercised. Adds 8 hermetic tests; deletes 1 test that could not fail for the reason its name gave.Type of change
Decalog check
Quality bar
Not an agent / skill / app change — those sections deleted.
What was untested
format_topologyfans out to three helpers.topological_orderwalks the edgechain from the one node that is never a destination, then sweeps up whatever the
walk missed;
node_labelhas three shapes;format_dagrenders a grid positionand a connection line, both from
Optionfields. The two fixture tests reachedthe happy path of each and nothing else — no fan-out, no orphan, no cycle, no
node without a command, no inline node, no bare primitive node, no partial
row/col, no unlabelled edge.Tests added, and the mutation that proves each one bites
Each mutation was applied alone to
src/render/topology.rs, the named test run,and the file restored from a pristine copy before the next.
every_node_is_listed_exactly_once_however_the_edges_runtopological_orderlinear_flow_follows_the_edges_rather_than_declaration_ordercurrent = next_by_id.get(id).copied()→current = None(edge walk removed)arrows_join_consecutive_nodes_and_never_lead_the_firstif i == 0→if false && i == 0informat_lineara_closed_loop_of_edges_still_lists_every_nodeif source.is_some() { … }a_path_that_re_enters_itself_terminates_without_repeating_a_nodeif !seen.insert(id) { break; }→seen.insert(id);plusby_id.remove→by_id.get(…).copied(), with a bounded break so the run terminatesblisted twice. (Removing the guard alone makes the walk non-terminating; I confirmed that too, but a hang is a poor demonstration, so the recorded mutation is the bounded variant that reddens cleanly.)a_node_label_names_its_agent_command_its_inline_kind_or_neithernode.command.as_deref().unwrap_or("?")→unwrap_or("")dag_shows_a_grid_position_only_when_both_row_and_col_are_given(Some(r), Some(c))→(Some(r), c)withc.unwrap_or(0)row: 3rendered as(row 3, col 0)dag_connections_carry_label_and_input_and_degrade_to_empty_brackets{input}from the connection-line format stringOn the anti-patterns worth naming in Rust: none of these asserts on a value it
built two lines earlier — each builds a graph and asserts on rendered text.
None round-trips serde. The only
unwrap()s are onserde_yaml::from_strof aliteral fixture and on
str::findfor a substring the same test alreadyasserted is present, so neither can panic ahead of a real assertion.
Test deleted, and the proof it was not carrying anything
linear_topology_lists_nodes_in_flow_orderasserted thatwelded-to-tc.apprenders
tekla-watchbeforefilter-weldedbeforetc-upload. That fixturedeclares those three nodes in exactly that order, and
topological_orderfalls back to declaration order for everything its edge walk does not reach.
Reverse discipline, as required: I broke what it claimed to cover — replaced
current = next_by_id.get(id).copied()withcurrent = None, deleting the edgewalk outright — and it still passed. Under that same mutation the new
linear_flow_follows_the_edges_rather_than_declaration_orderfailed. So theordering claim in the old test's name was never testable by it.
Its residue (the fixture parses, renders
linear, names all three nodes) isasserted end-to-end through the same code path by
tests/app_show.rs::shows_welded_to_tc_linear_topology, so nothing is lost. Acomment at the deletion site records all of this in the file.
dag_topology_lists_nodes_and_connectionswas put through the same check andkept: mutating the connection-line format string (
→→->) reddens it, soit does bite, and its fan-in assertion (
excel-lookup → match-build) is notduplicated in
tests/app_show.rs.Gates
Run from
cli/on the repo-pinned toolchain (1.95.0, perrust-toolchain.toml),with
clang libsecret-1-dev libdbus-1-dev pkg-configinstalled as CI does:cargo fmt --all -- --check— passcargo clippy --all-targets -- -D warnings— passcargo test— pass, 52 test binaries, 0 failuresNotes for reviewers
The two mutations worth your attention are the fourth and fifth rows. The fourth
shows the closed-loop test is load-bearing on its own — the orphan test does not
cover it, because a graph with an orphan still has a source and a closed loop
does not. The fifth is the one place I could not produce a clean red from a
single-line mutation, and the PR body says so rather than dressing it up.
Generated by Claude Code