codeant-ai-for-open-source[bot] commented on code in PR #41133:
URL: https://github.com/apache/superset/pull/41133#discussion_r3471265699
##########
tests/integration_tests/dashboards/api_tests.py:
##########
@@ -3279,6 +3280,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 is not hermetic because it never forces
`EXCEL_EXPORT_S3_BUCKET` to be unset; if the test environment defines that
config, the endpoint will return `202` instead of `501` and the test will fail
nondeterministically. Patch `flask.current_app.config` in this test to
explicitly set `EXCEL_EXPORT_S3_BUCKET` to `None` (or empty) before making the
request. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Integration test becomes dependent on external config overrides.
- ⚠️ Local Excel-export config can cause spurious test failures.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Note the default configuration `EXCEL_EXPORT_S3_BUCKET: str | None =
None` in
`superset/config.py:1345-1352`, and that the export endpoint returns 501
only when this
value is falsy in `DashboardRestApi.export_xlsx` at
`superset/dashboards/api.py:199-251`,
specifically the guard `if not
current_app.config["EXCEL_EXPORT_S3_BUCKET"]:` at line 248.
2. Observe that integration tests use a config that imports from
`superset.config`
(`tests/integration_tests/superset_test_config.py:25-27`) and that Docker
dev/test
environments optionally import `superset_config_docker` from
`docker/pythonpath_dev/superset_config.py:13-19`, allowing a local override
such as
`EXCEL_EXPORT_S3_BUCKET = "exports"` to be defined outside this repo.
3. With such an override in place (e.g. `superset_config_docker.py` setting
`EXCEL_EXPORT_S3_BUCKET = "exports"`), run the integration test
`test_export_xlsx_501_when_bucket_unset` defined in
`tests/integration_tests/dashboards/api_tests.py:44-55`, which creates a
dashboard, logs
in, and posts to `api/v1/dashboard/<id>/export_xlsx/` at lines 49-51 without
patching
`current_app.config`.
4. Because `current_app.config["EXCEL_EXPORT_S3_BUCKET"]` is now truthy, the
guard in
`export_xlsx` at `superset/dashboards/api.py:248-251` does not return 501;
the endpoint
proceeds toward the normal 202 path, so the response status code is not 501
and the
assertion `assert rv.status_code == 501` at
`tests/integration_tests/dashboards/api_tests.py:50-51` fails, demonstrating
the test's
non-hermetic dependence on ambient configuration.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=5eae340d067c4c4fa416e8a4e9d010f6&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=5eae340d067c4c4fa416e8a4e9d010f6&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:** 3289:3290
**Comment:**
*Logic Error: This test is not hermetic because it never forces
`EXCEL_EXPORT_S3_BUCKET` to be unset; if the test environment defines that
config, the endpoint will return `202` instead of `501` and the test will fail
nondeterministically. Patch `flask.current_app.config` in this test to
explicitly set `EXCEL_EXPORT_S3_BUCKET` to `None` (or empty) before making the
request.
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=c37bd72d415661e51ed385a5b3d5cf80a49cf27ef1a3048f687174dfaafe20bf&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41133&comment_hash=c37bd72d415661e51ed385a5b3d5cf80a49cf27ef1a3048f687174dfaafe20bf&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]
