chore(databricks): add the test for stripping trailing semicolon before query execute#2536
chore(databricks): add the test for stripping trailing semicolon before query execute#2536Bartok9 wants to merge 1 commit into
Conversation
WalkthroughAdds a unit test that verifies the Databricks connector’s ChangesDatabricks query SQL handling
Estimated code review effort: 1 (Trivial) | ~3 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@core/wren/tests/unit/test_databricks_strip_query_semicolon.py`:
- Around line 9-23: Replace the source-text and duplicate-regex assertions in
test_query_strips_trailing_semicolon with a mocked Databricks connection/cursor
setup. Instantiate DatabricksConnector with the mocks, invoke query("SELECT
1;"), and assert the cursor’s execute call receives exactly "SELECT 1".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: cbfbfe7e-451e-416e-8539-5cc766a741ba
📒 Files selected for processing (2)
core/wren/src/wren/connector/databricks.pycore/wren/tests/unit/test_databricks_strip_query_semicolon.py
| def test_query_strips_trailing_semicolon(): | ||
| src = ( | ||
| Path(__file__).resolve().parents[2] | ||
| / "src" | ||
| / "wren" | ||
| / "connector" | ||
| / "databricks.py" | ||
| ).read_text(encoding="utf-8") | ||
| assert "sql = strip_trailing_semicolon(sql)" in src | ||
| # appears in query, not only dry_run | ||
| q = src.split("def query", 1)[1].split("def dry_run", 1)[0] | ||
| assert "strip_trailing_semicolon" in q | ||
|
|
||
| helper = re.compile(r"[;\s]+\Z") | ||
| assert helper.sub("", "SELECT 1;") == "SELECT 1" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Exercise DatabricksConnector.query instead of inspecting source text.
This test can pass even if query() sends the semicolon-terminated SQL to cursor.execute, because it only searches the file and tests a duplicate local regex. Mock the connection/cursor, invoke query("SELECT 1;"), and assert execute receives "SELECT 1".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@core/wren/tests/unit/test_databricks_strip_query_semicolon.py` around lines 9
- 23, Replace the source-text and duplicate-regex assertions in
test_query_strips_trailing_semicolon with a mocked Databricks connection/cursor
setup. Instantiate DatabricksConnector with the mocks, invoke query("SELECT
1;"), and assert the cursor’s execute call receives exactly "SELECT 1".
|
@Bartok9 there are some conflict. |
dry_run already stripped for subquery wrap; query forwarded client terminators. Strip before execute for limited and unlimited paths.
bb6fb10 to
4ae100a
Compare
|
Thanks @goldmedal! Rebased onto latest |
goldmedal
changed the title
|
The test case isn't necessary. Let's close this PR. |
|
No problem at all, thanks for taking a look @goldmedal! Makes sense to keep the suite lean since the strip is already covered. Appreciate the quick feedback. 🙏 |
2 participants
Summary
query()executed raw SQL including trailing;.dry_runalready stripped for subquery wrap.License
core/wren/**Apache-2.0.Verification
Summary by CodeRabbit