feat(admin-config-tool): task 5.6 - mock-controller push/dry-run coverage

Adds end-to-end propagation tests through push_config's real
composition with xceadmin.discovery (not just discovery.py's own
unit tests): a 4-mock-server redirect chain proving TooManyRedirectsError
surfaces correctly, and a monkeypatched-timeout test proving
NoPrimaryError does too, without waiting out a real 30s socket
timeout. Also locks in and documents a real wire-contract limitation:
ProbeConfigTagReply carries no rejection signal, so dry-run's
accepted=False path is unreachable today by design, not an oversight.

This completes all 18 sub-tasks in tasks.md.
This commit is contained in:
Rongsong Shen 2026-07-22 10:05:31 +00:00
parent 48a5a0b4e6
commit 67cc361191
2 changed files with 98 additions and 8 deletions

View file

@ -1,6 +1,6 @@
# Implementation Plan
- [ ] 1. Foundation: package scaffolding, shared errors, build/toolchain integration
- [x] 1. Foundation: package scaffolding, shared errors, build/toolchain integration
- [x] 1.1 Set up the xceadmin Python package skeleton
- Editable install registers a real `xceadmin` console-script entry point backed by an empty, working click group (no subcommands yet — `config build`/`config push` are added by task 4.1, not this task)
- `xcecommon` is importable as a standalone package independent of `xceadmin`'s CLI
@ -21,7 +21,7 @@
- Observable: running the build command for this component from a clean checkout succeeds and reports test pass/fail
- _Requirements: 5.5, 8.1, 8.2, 8.6_
- [ ] 2. Core: xcecommon shared library
- [x] 2. Core: xcecommon shared library
- [x] 2.1 (P) Validate operator-authored YAML against the bundled schemas
- Each of the seven config file types can be checked against its own bundled validation schema, independent of anything the operator supplies
- Reserved queue/host names are rejected even when otherwise schema-valid
@ -56,7 +56,7 @@
- _Boundary: xcecommon.wire_
- _Depends: 2.2_
- [ ] 3. Core: xceadmin CLI orchestration
- [x] 3. Core: xceadmin CLI orchestration
- [x] 3.1 (P) Compile a configs directory into a deployable cluster binary
- Running the build command against a directory of YAML files produces a binary output file with a printed checksum
- A missing individual YAML file is treated as an empty section, not an error
@ -82,7 +82,7 @@
- _Boundary: xceadmin.push_
- _Depends: 2.4, 3.2_
- [ ] 4. Integration: wire the CLI entry point and hook xceadmin into cluster deployment
- [x] 4. Integration: wire the CLI entry point and hook xceadmin into cluster deployment
- [x] 4.1 Expose config build and config push as a single command-line tool
- `xceadmin config build` and `xceadmin config push` (with `--dry-run`) are both reachable from one installed command
- Any failure surfaces as a specific, human-readable message with a non-zero exit code, not a raw traceback
@ -99,7 +99,7 @@
- _Boundary: deploy-ha.sh_
- _Depends: 4.1_
- [ ] 5. Validation: test coverage across build, push, and packaging
- [x] 5. Validation: test coverage across build, push, and packaging
- [x] 5.1 (P) Unit test the field-mapping logic in isolation
- Both mapper shapes (list sections and the scalar section) are verified against in-memory input with no file I/O
- The one file that feeds two separate output fields is verified explicitly
@ -128,13 +128,13 @@
- _Boundary: xceadmin.build_
- _Depends: 3.1_
- [ ] 5.5 (P) Test every YAML validation failure mode
- [x] 5.5 (P) Test every YAML validation failure mode
- Each invalid-fixture case (schema violation, reserved name, duplicate name including a cross-type collision, undefined reference) produces the documented outcome — hard failure or warning, matching the requirement
- _Requirements: 4.1, 4.2, 4.3, 4.4, 4.5_
- _Boundary: xcecommon.validation_
- _Depends: 2.1_
- [ ] 5.6 (P) Test config push and dry-run against a mock controller
- [x] 5.6 (P) Test config push and dry-run against a mock controller
- A mock TLS server standing in for the controller confirms the full push flow: successful push, redirect-following, and every documented error path
- The same mock confirms dry-run reports both an accepted and a rejected outcome correctly
- _Requirements: 2.1, 2.2, 2.3, 2.4, 2.5, 2.6, 2.7, 3.1, 3.2, 3.3, 3.4, 6.1, 6.2, 6.3, 7.1, 7.2, 7.3_
@ -159,3 +159,5 @@
- **5.1, 5.2**: Already fully satisfied by task 2.3's `test_mapping.py` and task 2.4's `test_wire.py` respectively — same requirement lists, same acceptance bullets (in-memory-only mapper coverage + the explicit two-field case for 5.1; a fixed known byte sequence + real loopback round-trips for 5.2). No new tests written; would have been pure duplication.
- **5.3**: Found and fixed a real gap while writing `test_schema_bundling.py` — `xcecommon.validation.validate_yaml()`'s `json.loads(schema_path.read_text())` call (loading the *bundled JSON validation schema itself*, not the operator's YAML) had no error handling, so a corrupted bundled JSON schema crashed with a raw `json.JSONDecodeError` instead of a clean `ValidationError` — the same "crash vs. clean error" class of bug task 2.2 already fixed for `controller.capnp`, just on the JSON-schema side, and not caught by task 2.1 since that task's tests only exercised operator-YAML failure modes, never a corrupted *schema* file. Fixed by wrapping it and raising `ValidationError(..., constraint="schema-corrupted")`, matching the existing missing-schema handling's pattern one function above it. **Gotcha for any future task touching `xcecommon`/`xceadmin` source**: editing a source file does NOT propagate to a running `.venv` until reinstalled — always re-run `xce-build.sh xceadmin` after a source edit before trusting a bare `.venv/bin/python -m pytest` result; a stale venv (especially one left in `release`/pinned mode by a prior profile run) will silently test the *old* code and produce a false failure (or false pass) that looks like a real bug.
- **5.4**: `tests/fixtures/{minimal,full}/configs/*.yaml` + checked-in `expected.bin` golden files added to `test_config_build.py` (not a new file — design.md's File Structure Plan lists the SAME requirement set for `test_config_build.py` under both 3.1 and 5.4). `full/configs/*.yaml` is a byte-for-byte copy of `src/configs/*.yaml`; its golden SHA (`860cfd4f59ef...`) independently matches the tag `deploy-ha.sh` produced in task 4.2's live verification against the same source files — an unplanned but welcome additional confirmation of Requirement 9's cross-context determinism guarantee. Golden-file tests catch representation drift (bytes change even though decoded content is still semantically correct) that purely structural/decode-based assertions elsewhere in the suite cannot detect; regenerate `expected.bin` deliberately (via `build_cluster_config()`, sanity-checked by decoding it back) whenever a real, intentional field-mapping change lands — never hand-edit the binary.
- **5.5**: Already fully satisfied by task 2.1's `test_validation.py` — every sub-case (schema violation, reserved name, cross-type duplicate collision via `test_check_duplicates_allows_limit_name_to_collide_with_queue_name`, undefined reference) already has an explicit test with the exact same requirement list. No new tests written.
- **5.6**: `probe_config()`'s dry-run has no reachable `accepted=False` path — `ProbeConfigTagReply` genuinely carries no rejection signal on the wire (see task 3.3's note). 5.6's bullet asking for both an accepted *and* a rejected outcome cannot be satisfied without inventing undocumented client-side rejection logic; instead added `test_probe_config_has_no_wire_level_rejection_signal_by_design` (parametrized over several `configTag` values) that locks in and documents the limitation explicitly rather than leaving it silently uncovered. Added two new end-to-end propagation tests through `push_config`'s real composition with `xceadmin.discovery` (not just discovery.py's own unit tests): a real 4-mock-server redirect chain proving `TooManyRedirectsError` surfaces correctly, and a `NoPrimaryError`-on-timeout test that monkeypatches `xcecommon.wire.connect` to raise `ConnectionTimeoutError` immediately rather than waiting out a real 30s socket timeout.