codeant-ai-for-open-source[bot] commented on code in PR #40231:
URL: https://github.com/apache/superset/pull/40231#discussion_r3262283039
##########
tests/integration_tests/charts/api_tests.py:
##########
@@ -1237,6 +1237,69 @@ def test_get_charts_tag_filters(self):
chart["id"] for chart in data_by_name["result"]
), set(chart.id for chart in expected_charts) # noqa: C401
+ def test_get_charts_changed_on_delta_humanized_sort_monotonic(self):
+ """Regression for #27500: sorting the chart list by
+ `changed_on_delta_humanized` desc must yield results whose underlying
+ `changed_on` timestamps are monotonically non-increasing. The original
+ report shows the humanized column visually out of order, suggesting
+ the sort key didn't actually reflect the timestamp."""
+ from datetime import datetime, timedelta
+
+ admin = self.get_user("admin")
+ # Insert two charts with distinct changed_on timestamps. Use raw UPDATE
+ # to force the values since assignment alone can be overridden by the
+ # before-update hook.
+ chart_older = self.insert_chart(
+ "regression_27500_older", [admin.id], 1, description="z"
+ )
+ chart_newer = self.insert_chart(
+ "regression_27500_newer", [admin.id], 1, description="z"
+ )
+ now = datetime.utcnow()
+ chart_older.changed_on = now - timedelta(days=2)
+ chart_newer.changed_on = now
Review Comment:
**Suggestion:** This regression test can produce false negatives because it
uses `now` vs `2 days ago`, which still appears correctly ordered even if the
backend accidentally sorts by the humanized text value instead of the real
timestamp. Use timestamp pairs that are known to break lexical/text sorting
(for example `10 days ago` vs `2 days ago`) so the test actually detects an
incorrect sort key. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Regression test may miss string-based sort regressions.
- ⚠️ Backend bug like #27500 could slip past CI.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In `tests/integration_tests/charts/api_tests.py:1240-1242` the test
`test_get_charts_changed_on_delta_humanized_sort_monotonic` is defined to
validate sort
behavior for `changed_on_delta_humanized` on the `/api/v1/chart/` list
endpoint (verified
via Read on this file and the docstring).
2. The test inserts two charts at
`tests/integration_tests/charts/api_tests.py:33-41` and
sets their timestamps using `now = datetime.utcnow()` and
`chart_older.changed_on = now -
timedelta(days=2)` / `chart_newer.changed_on = now` (lines 1258-1260 in the
PR), so the
humanized values will correspond to "now" vs approximately "2 days ago".
3. The test then calls the chart list API with
`order_column="changed_on_delta_humanized"`
and `order_direction="desc"` at
`tests/integration_tests/charts/api_tests.py:46-57`, which
hits `ChartRestApi` (`superset/charts/api.py:108-113`). `ChartRestApi`
declares
`changed_on_delta_humanized` as a list and order column
(`superset/charts/api.py:137-151`
and `:50-52` in the class snippet; grep shows it in `list_columns` at line
146 and
`order_columns` at line 189).
4. If the backend implementation mistakenly sorts by the humanized string
value (e.g.
`"now"` and `"2 days ago"`) instead of the underlying timestamp, a
descending lexical sort
would still return `"now"` before `"2 days ago"` because `"now"` compares
greater than `"2
days ago"` lexicographically. The assertion at
`tests/integration_tests/charts/api_tests.py:76-78`
(`indices["regression_27500_newer"] <
indices["regression_27500_older"]`) will therefore pass even under the buggy
string-based
sort, causing a false negative. Using values that break lexical order like
`"10 days ago"`
vs `"2 days ago"` (e.g. `now - timedelta(days=10)` and `now -
timedelta(days=2)`) would
cause the string-based order to differ from timestamp order, and the test
would then fail
under the incorrect sort key.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20tests%2Fintegration_tests%2Fcharts%2Fapi_tests.py%0A%2A%2ALine%3A%2A%2A%201258%3A1260%0A%2A%2AComment%3A%2A%2A%0A%09%2ALogic%20Error%3A%20This%20regression%20test%20can%20produce%20false%20negatives%20because%20it%20uses%20%60now%60%20vs%20%602%20days%20ago%60%2C%20which%20still%20appears%20correctly%20ordered%20even%20if%20the%20backend%20accidentally%20sorts%20by%20the%20humanized%20text%20value%20instead%20of%20the%20real%20timestamp.%20Use%20timestamp%20pairs%20that%20are%20known%20to%20break%20lexical%2Ftext%20sorting%20%28for%20example%20%6010%20days%20ago%60%20vs%20%602%20days%20ago%60%29%20so%20the%20test%20actually%20detects%20an%20incorrect%20sort%20key.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%2
0it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20tests%2Fintegration_tests%2Fcharts%2Fapi_tests.py%0A%2A%2ALine%3A%2A%2A%201258%3A1260%0A%2A%2AComment%3A%2A%2A%0A%09%2ALogic%20Error%3A%20This%20regression%20test%20can%20produce%20false%20negatives%20because%20it%20uses%20%60now%60%20vs%20%602%20days%20ago%60%2C%20which%20still%20appears%20correctly%20ordered%20even%20if%20the%20backend%20accidentally%20sorts%20by%20the%20humanized%20text%20value%20instead%20of%20the%20real%20timestamp.%20Use%20t
imestamp%20pairs%20that%20are%20known%20to%20break%20lexical%2Ftext%20sorting%20%28for%20example%20%6010%20days%20ago%60%20vs%20%602%20days%20ago%60%29%20so%20the%20test%20actually%20detects%20an%20incorrect%20sort%20key.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A)
*(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/charts/api_tests.py
**Line:** 1258:1260
**Comment:**
*Logic Error: This regression test can produce false negatives because
it uses `now` vs `2 days ago`, which still appears correctly ordered even if
the backend accidentally sorts by the humanized text value instead of the real
timestamp. Use timestamp pairs that are known to break lexical/text sorting
(for example `10 days ago` vs `2 days ago`) so the test actually detects an
incorrect sort key.
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%2F40231&comment_hash=74d463beeece847a72d1ae55d7acbd0d6999dfceb31ca4a6f0a955824fbf3c66&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40231&comment_hash=74d463beeece847a72d1ae55d7acbd0d6999dfceb31ca4a6f0a955824fbf3c66&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]
