Skip to content

refactor(basket-compare): rework into a tested, low-noise basket diff#1284

Merged
markuslf merged 1 commit into
mainfrom
refactor/basket-compare
Jun 19, 2026
Merged

refactor(basket-compare): rework into a tested, low-noise basket diff#1284
markuslf merged 1 commit into
mainfrom
refactor/basket-compare

Conversation

@NavidSassan

Copy link
Copy Markdown
Member

What

Reworks tools/basket-compare from a deepdiff-based, change-only script into a
single generic diff engine, and adds golden-file unit tests.

Why

When a new plugin release ships a regenerated Director basket, importing it
overwrites every object it contains - silently clobbering any manual adjustment
an admin made. basket-compare exists to surface those before the import.

The previous version only printed changed objects and silently dropped
additions and removals (and its add/remove detection was broken by a leaked
loop variable). As a result it missed real cases - e.g. a datafield that an
upcoming import would delete never showed up.

Changes

  • Correctness: reports objects only in the left basket, only in the right,
    and changed in both. By default only changed objects are shown (an import
    never touches an object it does not contain); --all adds the Only in ...
    tables.
  • Simplification: replaces the ten per-class copy-paste compare_* /
    get_*_diff functions and the deepdiff dependency with one generic engine
    over a normalised {uuid: {field: value}} shape.
  • Readability / less noise:
    • a whole added/removed sub-object (a service, a command argument, a field
      binding) collapses into a single row instead of one row per attribute;
    • field-binding rows name the datafield by its varname plus a truncated,
      dash-preserving uuid (still greppable against the basket);
    • list-valued fields diff element by element; over-long values are summarised;
    • suppresses known Director export noise - the is_required flag (which does
      not round-trip) and values stored as a number on one side and the same
      digits as a string on the other.
  • Tests: new tools/unit-test/basket-compare/ golden-file tests (fixtures +
    run), robust to the trailing-whitespace / end-of-file pre-commit hooks.
  • Docs: TOOLS.md basket-compare section updated.

Impact

On a real upgrade export the output dropped from ~1700 to ~440 lines while now
catching object additions/removals the old version missed.

Notes

  • tools/ scripts are not auto-discovered by the CI test harness
    (run-unit-tests covers check-plugins/ only); the new test runs via
    tools/unit-test/basket-compare/run. Wiring tools tests into CI is a possible
    follow-up.
  • No CHANGELOG entry: basket-compare is an internal maintainer tool, not part
    of the shipped, admin-facing plugin collection.

Importing a regenerated Director basket overwrites every object it
contains, silently clobbering manual adjustments. basket-compare exists
to surface those first, but the previous version only printed changed
objects and dropped additions and removals (its add/remove detection was
broken by a leaked loop variable), so it missed cases such as a datafield
an import would delete.

- report objects only in the left basket, only in the right, and changed
  in both; default shows changed only, --all adds the "Only in ..." tables
- replace the ten per-class copy-paste compare_/get_diff functions and the
  deepdiff dependency with one generic engine over a normalised
  {uuid: {field: value}} shape
- collapse a whole added/removed sub-object (service, command argument,
  field binding) into a single row instead of one row per attribute
- name field bindings by datafield varname plus a truncated,
  dash-preserving uuid; diff list-valued fields element by element
- suppress known Director export noise: the is_required flag and the
  number-vs-string serialisation of the same value
- add golden-file unit tests under tools/unit-test/basket-compare/
- update TOOLS.md
@NavidSassan NavidSassan marked this pull request as ready for review June 19, 2026 09:52
@NavidSassan NavidSassan requested a review from markuslf June 19, 2026 09:52
@markuslf markuslf merged commit 2f080da into main Jun 19, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants