codeant-ai-for-open-source[bot] commented on code in PR #34091:
URL: https://github.com/apache/superset/pull/34091#discussion_r2818021848
##########
tests/unit_tests/db_engine_specs/test_clickhouse.py:
##########
@@ -229,6 +229,33 @@ def test_connect_make_label_compatible(column_name: str,
expected_result: str) -
assert label == expected_result
[email protected](
+ "extra,column_name,expected_result",
+ [
+ # Default behavior (mutate labels)
+ (None, "time", "time_07cc69"),
+ ({}, "time", "time_07cc69"),
+ # Explicitly enable
+ ({"mutate_label_name": True}, "time", "time_07cc69"),
Review Comment:
**Suggestion:** The new parametrized test for label mutation with `extra`
expects a different default mutated label (`time_07cc69`) than the existing
`test_connect_make_label_compatible` (`time_336074`), even though both exercise
the same default behavior for `make_label_compatible`, which will either cause
this test to fail or incorrectly document the function's output. The expected
mutated labels in the new test should be aligned with the existing test so that
default behavior (with or without a `database` argument when
`mutate_label_name` is enabled or absent) is consistent. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ ClickHouseConnect label mutation tests cannot both pass simultaneously.
- ⚠️ Confusing contract for default label mutation with extra.
- ⚠️ CI reliability reduced by contradictory test expectations.
```
</details>
```suggestion
(None, "time", "time_336074"),
({}, "time", "time_336074"),
# Explicitly enable
({"mutate_label_name": True}, "time", "time_336074"),
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run the unit tests for ClickHouse connect engine specs: `pytest
tests/unit_tests/db_engine_specs/test_clickhouse.py`.
2. Observe `test_connect_make_label_compatible` (around line 223) which
imports
`ClickHouseConnectEngineSpec` and asserts that
`spec.make_label_compatible("time")`
returns `"time_336074"` as the default mutated label.
3. Observe `test_connect_make_label_compatible_with_extra` (around line 245)
in the same
file, which also calls
`ClickHouseConnectEngineSpec.make_label_compatible("time",
database=mock_database)` with `extra` set to `None`, `{}`, or
`{"mutate_label_name":
True}`, but asserts the default mutated label is `"time_07cc69"` (lines
235–239).
4. Because both tests exercise the same function and both treat these cases
as the
"default behavior (mutate labels)", there is no possible implementation
where both
expectations (`"time_336074"` and `"time_07cc69"` for the same default
setting) can be
satisfied simultaneously; running the test file will therefore cause at
least one of these
tests to fail, or it will encode an inconsistent contract for default label
mutation if
the implementation is changed to satisfy one side only.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/db_engine_specs/test_clickhouse.py
**Line:** 236:239
**Comment:**
*Logic Error: The new parametrized test for label mutation with `extra`
expects a different default mutated label (`time_07cc69`) than the existing
`test_connect_make_label_compatible` (`time_336074`), even though both exercise
the same default behavior for `make_label_compatible`, which will either cause
this test to fail or incorrectly document the function's output. The expected
mutated labels in the new test should be aligned with the existing test so that
default behavior (with or without a `database` argument when
`mutate_label_name` is enabled or absent) is consistent.
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.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F34091&comment_hash=27a05ef9a1c91200cad787ae5321ad73465afe8dbf9ab94fdf9452d97ad1724f&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F34091&comment_hash=27a05ef9a1c91200cad787ae5321ad73465afe8dbf9ab94fdf9452d97ad1724f&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]
