codeant-ai-for-open-source[bot] commented on code in PR #41133:
URL: https://github.com/apache/superset/pull/41133#discussion_r3429842033
##########
tests/integration_tests/dashboards/api_tests.py:
##########
@@ -3277,6 +3278,91 @@ def _base_filter(query):
response_roles = [result["text"] for result in response["result"]]
assert response_roles == ["Alpha"]
+ def test_export_xlsx_501_when_bucket_unset(self):
+ """Dashboard API: export_xlsx returns 501 when the S3 bucket is
unset."""
+ admin = self.get_user("admin")
+ dashboard = self.insert_dashboard("xlsx-501", None, [admin.id])
+ self.login(ADMIN_USERNAME)
+ try:
+ rv =
self.client.post(f"api/v1/dashboard/{dashboard.id}/export_xlsx/")
+ assert rv.status_code == 501
Review Comment:
**Suggestion:** This test never forces `EXCEL_EXPORT_S3_BUCKET` to be unset,
so it becomes environment-dependent: if the test config already defines that
bucket, the endpoint will not return `501` and this assertion will fail
intermittently. Patch `flask.current_app.config` inside this test to explicitly
clear or null out the bucket key before calling the endpoint. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Integration test fails when Excel S3 bucket configured.
- ⚠️ Excel export 501-branch coverage depends on ambient config.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Observe the export handler logic in
`superset/dashboards/api.py:1434-1437`, where
`export_xlsx` returns HTTP 501 only when
`current_app.config["EXCEL_EXPORT_S3_BUCKET"]` is
falsy.
2. Note that the default value for this config key is `None` in
`superset/config.py:1298-1311` (`EXCEL_EXPORT_S3_BUCKET: str | None =
None`), and there is
no explicit override in the test suite except for localized `patch.dict`
usages that set
it to `"exports"` inside `with` blocks.
3. In `tests/integration_tests/dashboards/api_tests.py:3281-3292`, the test
`test_export_xlsx_501_when_bucket_unset` creates an empty dashboard and calls
`self.client.post(f"api/v1/dashboard/{dashboard.id}/export_xlsx/")`, then
asserts
`rv.status_code == 501` without patching `EXCEL_EXPORT_S3_BUCKET` to `None`,
relying on
whatever value is already in `flask.current_app.config`.
4. Configure the app so `EXCEL_EXPORT_S3_BUCKET` is truthy at startup (e.g.,
via a local
config override or environment-based config, which will populate
`current_app.config["EXCEL_EXPORT_S3_BUCKET"]` before tests run); then
execute `pytest
tests/integration_tests/dashboards/api_tests.py::test_export_xlsx_501_when_bucket_unset`
and observe that the handler follows the non-501 branch (eventually
returning 400 for an
empty dashboard as covered by `test_export_xlsx_400_for_empty_dashboard` in
the same
file), causing this test's assertion `assert rv.status_code == 501` at lines
3287-3288 to
fail purely because of ambient config rather than the code path it intends
to validate.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=77b2b0ed51c94b56ae42012ddacac5a7&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=77b2b0ed51c94b56ae42012ddacac5a7&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/integration_tests/dashboards/api_tests.py
**Line:** 3287:3288
**Comment:**
*Logic Error: This test never forces `EXCEL_EXPORT_S3_BUCKET` to be
unset, so it becomes environment-dependent: if the test config already defines
that bucket, the endpoint will not return `501` and this assertion will fail
intermittently. Patch `flask.current_app.config` inside this test to explicitly
clear or null out the bucket key before calling the endpoint.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41133&comment_hash=3be4aeb13521ca14ead0d3c725d6f5818eb43389ed4929903ac061233ef77038&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41133&comment_hash=3be4aeb13521ca14ead0d3c725d6f5818eb43389ed4929903ac061233ef77038&reaction=dislike'>👎</a>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
