test(versioning): un-skip the two obsolete activity-view skips - #42710
Conversation
Both skip reasons have been overtaken by merges: - test_activity_excludes_records_after_retention_prune waited on the retention prune extracted to sc-111099; _prune_old_versions_impl is in superset/tasks/version_history_retention.py. - test_activity_surfaces_dashboard_restore_event waited on the restore endpoint, which landed in apache#42469. It passes unchanged. The retention test needed one substantive update beyond removing the decorator: it was written against an age-only prune, but the shipped prune preserves any transaction still anchoring an open shadow row (end_transaction_id IS NULL) — an entity's current state is never dropped, however old. The test's single edit created exactly that row, so the prune correctly removed nothing and the assertion failed. The test now edits the chart twice: the second edit closes the first row, making the first transaction genuinely prunable, and the assertions exercise the keep-live guard as a side effect. Full file passes 40/40 against the integration SQLite DB. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Code Review Agent Run #019f4cActionable Suggestions - 0Additional Suggestions - 1
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #42710 +/- ##
=======================================
Coverage 65.43% 65.43%
=======================================
Files 2810 2810
Lines 159528 159528
Branches 36410 36410
=======================================
+ Hits 104380 104388 +8
+ Misses 53102 53094 -8
Partials 2046 2046
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…t transactions Follow-ups from a clean-code + tidy-first review pass over this PR. Both lenses independently flagged the same weakness: the retention assertions (pruned_transactions >= 1, count decreased) could pass on this persistent integration DB off a leftover old transaction from a prior run, while this test's own backdated transaction survived — green for the wrong reason. The test now captures the two transaction ids its edits create and asserts membership: the closed one is pruned, and the one anchoring the chart's live row survives despite being equally old. That turns the keep-live guard from docstring prose into an assertion. Also: datetime.utcnow() (deprecated on 3.12+) swapped for the same naive-from-aware derivation the prune impl uses; the duplicated two-edit rationale collapsed to one authoritative statement in the docstring; and the restore test's docstring no longer claims a ['__meta__', 'restore'] path — the assertion (and the listener) use ['__meta__'], with the verb in action_kind. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
rusackas
left a comment
rusackas
left a comment
There was a problem hiding this comment.
Thanks for chasing these down, @mikebridge! The retention test rewrite holds up against the actual keep-live guard in _prune_old_versions_impl, and the membership checks are a nice touch given this runs against a persistent DB. Bito's import nit doesn't hold up either. That pattern's already used elsewhere in this file for scoped test imports. LGTM!
Code Review Agent Run #03cf80Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
2 participants
SUMMARY
Two activity-view integration tests were skipped waiting on work that has since merged:
test_activity_excludes_records_after_retention_prunewaited on the retention prune extracted to sc-111099;_prune_old_versions_implis now insuperset/tasks/version_history_retention.py.test_activity_surfaces_dashboard_restore_eventwaited on the restore endpoint, which landed in feat(versioning): version-restore engine and endpoints for charts, dashboards, and datasets #42469. It passes with no changes.The retention test needed one substantive update beyond removing the decorator: it was written against an age-only prune, but the shipped prune preserves any transaction still anchoring an open shadow row (
end_transaction_id IS NULL) — an entity's current state is never dropped, however old. The test's single edit created exactly that row, so the prune correctly removed nothing and the assertion could never pass. The test now edits the chart twice: the second edit closes the first edit's shadow row, making the first transaction genuinely prunable, and the assertions exercise the keep-live guard as a side effect (the live transaction survives despite being backdated past the cutoff).Full file passes 40/40 against the integration SQLite DB.
Disclosure: this change was developed with AI assistance (Claude), on behalf of and reviewed by @mikebridge.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — test-only change.
TESTING INSTRUCTIONS
40 passed (was 38 passed, 2 skipped). The two un-skipped tests are
TestDashboardActivityView::test_activity_excludes_records_after_retention_pruneandTestDashboardActivityView::test_activity_surfaces_dashboard_restore_event.ADDITIONAL INFORMATION