Skip to content

test(reports): regression for alerts CSV missing chart time filters (#25538)#40232

Merged
rusackas merged 1 commit into
masterfrom
tdd/issue-25538-alerts-csv-post-processed
May 20, 2026
Merged

test(reports): regression for alerts CSV missing chart time filters (#25538)#40232
rusackas merged 1 commit into
masterfrom
tdd/issue-25538-alerts-csv-post-processed

Conversation

@rusackas
Copy link
Copy Markdown
Member

SUMMARY

This is a test-only PR opened as a TDD-style validation of issue #25538.

#25538 (filed 2023-10) reports that after upgrading from Superset 2.1.0 to 3.0.0, a table chart with a last 30 days time filter sends a 14-row CSV in 2.1.0 but a 219-row CSV (the full unfiltered table) in 3.0.0 — even though the same chart in the UI and as an image still shows only the 14 filtered rows.

The mechanism that ensures the chart's saved query_context (including time filters) is applied to the CSV is the type=POST_PROCESSED flag on the ChartDataRestApi.get_data URL emitted by BaseReportState._get_url. A regression that emitted type=FULL or omitted type would re-introduce the original bug.

This PR adds one regression test:

  1. test_get_url_for_csv_uses_post_processed_type — constructs a BaseReportState for a chart-based schedule, calls _get_url(result_format=CSV), and asserts format=csv AND type=post_processed appear in the URL.

How to interpret CI

TESTING INSTRUCTIONS

```bash
pytest tests/unit_tests/commands/report/execute_test.py::test_get_url_for_csv_uses_post_processed_type -v
```

ADDITIONAL INFORMATION

🤖 Generated with Claude Code

… filters (#25538)

TDD-style validation of #25538: pins the invariant that
BaseReportState._get_url(result_format=CSV) emits a URL with
type=POST_PROCESSED, which is what carries the chart's saved query_context
(including time filters) through to the CSV renderer. The original report
described a chart with a "last 30 days" filter producing a CSV containing
all 219 rows of the underlying table rather than the 14 filtered rows.

Closes #25538

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@dosubot dosubot Bot added alert-reports Namespace | Anything related to the Alert & Reports feature closes-issue labels May 18, 2026
Copy link
Copy Markdown
Contributor

@bito-code-review bito-code-review Bot left a comment

Choose a reason for hiding this comment

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

Code Review Agent Run #603753

Actionable Suggestions - 1
  • tests/unit_tests/commands/report/execute_test.py - 1
Review Details
  • Files reviewed - 1 · Commit Range: d8fee80..d8fee80
    • tests/unit_tests/commands/report/execute_test.py
  • Files skipped - 0
  • Tools
    • Whispers (Secret Scanner) - ✔︎ Successful
    • Detect-secrets (Secret Scanner) - ✔︎ Successful
    • MyPy (Static Code Analysis) - ✔︎ Successful
    • Astral Ruff (Static Code Analysis) - ✔︎ Successful

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

  • /review - Manually triggers a full AI review.

  • /pause - Pauses automatic reviews on this pull request.

  • /resume - Resumes automatic reviews.

  • /resolve - Marks all Bito-posted review comments as resolved.

  • /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

AI Code Review powered by Bito Logo

Comment thread tests/unit_tests/commands/report/execute_test.py
@codecov
Copy link
Copy Markdown

codecov Bot commented May 18, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 64.16%. Comparing base (9bfa064) to head (d8fee80).

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #40232   +/-   ##
=======================================
  Coverage   64.16%   64.16%           
=======================================
  Files        2591     2591           
  Lines      138257   138257           
  Branches    32075    32075           
=======================================
  Hits        88711    88711           
  Misses      48016    48016           
  Partials     1530     1530           
Flag Coverage Δ
hive 39.44% <ø> (ø)
mysql 59.13% <ø> (ø)
postgres 59.21% <ø> (ø)
presto 41.13% <ø> (ø)
python 60.64% <ø> (ø)
sqlite 58.85% <ø> (ø)
unit 100.00% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@rusackas rusackas requested a review from aminghadersohi May 20, 2026 17:04
Copy link
Copy Markdown
Contributor

@aminghadersohi aminghadersohi left a comment

Choose a reason for hiding this comment

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

Reviewed as Amingor (Amin Ghadersohi's PR review bot).

Verdict: APPROVE

This is a well-constructed regression guard for #25538. One NIT and three PRaise findings:


NIT — Redundant inline imports (lines 1499–1502)

from datetime import datetime, from uuid import UUID, and from superset.commands.report.execute import BaseReportState are all already imported at the module level — the three inline copies can be removed. from superset.common.chart_data import ChartDataResultFormat is the only genuinely new import; it could be lifted to the module-level block. That said, this file already has this pattern (import urllib.parse inline in several tests, from sqlalchemy.orm.exc import StaleDataError inline at line 1036), so it's consistent with existing style. Optional cleanup only.


PRAISE — Test design is exactly right

Calling through to the real get_url_path instead of mocking it is the correct choice. A mock would make the test pass trivially regardless of what type= parameter _get_url passes, defeating the regression guard entirely. ChartDataResultType.POST_PROCESSED.value == "post_processed" confirmed.

PRAISE — "How to interpret CI" section in PR description is exemplary — green/red mapped directly to the bug mechanism. Rare and valuable.

PRAISE — Error message in the assert includes the issue number and original symptom (14 rows vs 219 rows). Future devs breaking this test will immediately understand why it exists.

@rusackas rusackas merged commit aa8255c into master May 20, 2026
68 of 69 checks passed
@rusackas rusackas deleted the tdd/issue-25538-alerts-csv-post-processed branch May 20, 2026 18:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

alert-reports Namespace | Anything related to the Alert & Reports feature closes-issue size/M

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Alerts .csv does not use a chart's time filters -- regression in 3.0.0

3 participants