Skip to content

fix(compare): show dropped_triggers-only diffs in text output - #90

Merged
leo-aa88 merged 1 commit into
LAA-Software-Engineering:mainfrom
VedantMadane:fix/dropped-triggers-has-changes
Aug 27, 2026
Merged

leo-aa88 merged 1 commit into
LAA-Software-Engineering:mainfrom
VedantMadane:fix/dropped-triggers-has-changes

Conversation

@VedantMadane

Copy link
Copy Markdown
Contributor

Summary

CompareResult.has_changes omitted dropped_triggers, so CLI and POST /query/compare text format printed "No significant changes between windows" whenever the only difference was a trigger present in baseline window B but not in A. JSON still listed the dropped trigger.

Fix

Include dropped_triggers in has_changes (same as new_triggers). Text renderers already render the dropped-trigger section after the has_changes guard.

Tests

  • test_true_when_dropped_triggers_only — regression for this bug
  • Adjusted test_false_when_only_stable_clusters so it no longer incorrectly couples stable clusters with dropped triggers

Fixes #70

Text renderers early-return on not has_changes, so a dropped-trigger-only
diff was hidden while still present in JSON. Count dropped_triggers the
same way as new_triggers.

Fixes LAA-Software-Engineering#70

Signed-off-by: Vedant Madane <6527493+VedantMadane@users.noreply.github.com>
@VedantMadane
VedantMadane requested a review from leo-aa88 as a code owner August 27, 2026 01:06
@leo-aa88

Copy link
Copy Markdown
Member

LGTM.

@leo-aa88
leo-aa88 merged commit 638e95c into LAA-Software-Engineering:main Aug 27, 2026
2 checks passed

@leo-aa88 leo-aa88 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

APPROVE

The fix is correct, and it's actually broader — and more justified — than the PR description claims. has_changes is also a real field in the JSON response schema (CompareResponse.has_changes), populated unconditionally from result.has_changes, not just when format=text. So POST /query/compare was returning "has_changes": false in JSON for a dropped-trigger-only diff too, even though dropped_triggers was correctly populated in the same payload — any consumer checking the summary boolean rather than the array would have missed it, and there was no existing test coverage for that case at all.

Fixing the shared has_changes property (rather than the issue's suggested alternative of having renderers check the dropped-trigger list independently) is the right architectural call for exactly this reason: it's the single source of truth consumed by CLI text, API text, and API JSON, so one fix closes all three gaps — including one nobody had filed a bug for. Traced the fourth consumer too: the dashboard frontend (app.js, renderCompare) branches on data.has_changes from the same API response, so it inherits the fix automatically; the frontend had already independently worked around the bug by rendering dropped_triggers even in the !has_changes branch, which is why the UI wasn't broken, just showing a contradictory "No significant changes" banner directly above a listed trigger. This fix removes that contradiction as a side effect.

The test change (test_false_when_only_stable_clusters dropping dropped_triggers from its input) is a correction, not a coverage regression — the old version was pinning the bug as expected behavior under a misleading docstring.

Ran it: pytest tests/unit/test_compare.py tests/unit/test_api.py — 152 passed. ruff check clean on both changed files.

No defects found. LGTM.

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.

compare text output hides dropped_triggers when they are the only change

2 participants