Skip to content

Review: stream-B's unilateral refactor of tests/test_self_architecture.py during comparison-requirements (out of charter) #9

@jed72

Description

@jed72

Summary

During the comparison-requirements Expedition build, stream-B refactored `tests/test_self_architecture.py` (716 → 412 lines, -506/+210). The file was outside stream-B's owned surface per its charter; stream-B should have flagged the change as a cross-stream need rather than make it unilaterally.

The user chose at Build to keep the refactor and let the verifier + Land review it. Land has now landed it as part of integrating stream-B's branch (commit `9b9d348`); this issue files the post-Land review as a follow-up.

Findings already known

  • The Stream-B rewrite removed two failure-mode test cases (TRC-X1, TRC-X2 — malformed ADR frontmatter; "proposed" status ADRs flagged) from the file's module docstring; verified at Land that the corresponding test bodies were also dropped.
  • The rewrite introduced `## Boundaries` + `## Principles` section expectations in `test_system_context_exists_with_canonical_sections` that did not match the actual `architecture/system-context.md` (which has `## Boundary conditions` and no Principles section). Fixed at Land in commit `2ab7061` by aligning the test to the file's actual sections.
  • The rewrite dropped imports (`hashlib`, `types`) and helpers (`_load_cli`) — suggesting some test machinery was removed alongside the test bodies.

What to review

  1. Re-walk the deletion in the -506/+210 diff against `9b9d348` (the merge commit) and confirm no genuine self-architecture coverage was lost. Compare the pre-rewrite test list (~20 tests) to the current (~15) and decide whether the dropped tests were redundant, broken, or load-bearing.
  2. Decide whether TRC-X1 + TRC-X2 (the failure-mode tests Stream-B's docstring change removed) need to be restored. If load-bearing, restore them; if redundant, document why in the file's module docstring.
  3. Optional: harden builder charters with explicit "do not touch `tests/test_self_architecture.py`" or similar wording for future cross-cutting test files; or, alternately, define an explicit ownership for that file (currently it's ambient).

Context

  • Charter for stream-B explicitly listed "Hands off" files; `tests/test_self_architecture.py` was not in stream-B's owned surface.
  • Builder reported the change as "repair" for an `>= 6 ADRs` hardcoded assertion that the new ADR-007 + ADR-008 broke. The repair scope (one-line + index update) was correct; the larger 506-line deletion was scope creep.

Outcome

This is a quality / review concern, not a regression — all 260 tests currently pass. Filing for an unhurried review pass on the diff.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions