Skip to content

test(snowflake): fix the mock patch target so the suite actually runs - #2603

Merged
douenergy merged 2 commits into
mainfrom
fix/snowflake-test-mock-target
Jul 28, 2026
Merged

test(snowflake): fix the mock patch target so the suite actually runs#2603
douenergy merged 2 commits into
mainfrom
fix/snowflake-test-mock-target

Conversation

@goldmedal

@goldmedal goldmedal commented Jul 28, 2026

Copy link
Copy Markdown
Collaborator

Summary

tests/connectors/test_snowflake.py has been silently broken: 11 of its 16 tests fail before their bodies run. This fixes the mock patch target, and one assertion the breakage was hiding.

What failure does this repair?

$ uv sync --locked --extra snowflake
$ uv run --no-sync pytest tests/connectors/test_snowflake.py -v
...
AttributeError: module 'wren.connector.snowflake' has no attribute 'snowflake'
11 failed, 5 passed

src/wren/connector/snowflake.py imports the driver inside the functions that use it (import snowflake.connector, with a # noqa: PLC0415), so wren.connector.snowflake never gains a snowflake attribute. The tests patch "wren.connector.snowflake.snowflake.connector.connect", a path that does not exist, and mock.patch fails at attribute resolution before the test body executes.

The fix patches the real module path, "snowflake.connector.connect". The function-local import resolves through sys.modules at call time, so the connector picks up the patched connect. The production import is deliberately function-local — it keeps an optional driver out of module import time — so it is left alone rather than hoisted to make patching convenient.

A second, hidden failure surfaced once the tests ran. test_query_with_limit_slices_result asserted that the connector fetches a full result and slices it to limit rows in Python. query() has not done that since LIMIT pushdown landed: it wraps the SQL as SELECT * FROM (...) AS _wren_sub LIMIT n and returns the driver's table unmodified. The assertion was stale and only survived because the whole file was inert. It is now test_query_with_limit_pushes_limit_into_sql, asserting the pushdown appears in the SQL handed to cursor.execute. The mocked cursor deliberately returns more rows than the limit — a real Snowflake would honour the pushed-down LIMIT — so the test fails if a client-side slice is ever reintroduced.

How is it tested?

tests/connectors/test_snowflake.py: 11 failed / 5 passed → 16 passed.

Note this file does not currently run in CI at all — the connector matrix covers only postgres and mysql. It needs no external service (it is fully mocked), so fixing it is a prerequisite to running it in the default jobs.

Regression check: pytest tests/unit/ --ignore=tests/unit/test_memory.py --ignore=tests/unit/test_mcp_server.py → 1023 passed, 2 skipped. ruff format --check / ruff check clean on src/ and on the touched file.

Duplicate check

No open PR touches tests/connectors/test_snowflake.py. #2593 and #2556 touch src/wren/connector/snowflake.py, which this PR does not modify.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes
    • Corrected Snowflake connection handling in authentication and connector creation tests.
    • Updated query limit behavior to apply limits in SQL and return complete query results without additional client-side slicing.

goldmedal and others added 2 commits July 28, 2026 13:30
…nt attribute path

wren.connector.snowflake imports snowflake.connector inside its functions
(kept out of module import time on purpose), so
wren.connector.snowflake.snowflake.connector.connect was never a valid
mock.patch target — every test using it raised AttributeError. Patch the
actual snowflake.connector.connect module path instead, which the
function-local import resolves through sys.modules.

Also update test_query_with_limit_slices_result to match the connector's
LIMIT-pushdown behavior (LIMIT is appended to the SQL sent to Snowflake,
not sliced client-side), which the mock patch fix exposed as a second,
previously-masked test/implementation mismatch.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…an fail

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added python Pull requests that update Python code core labels Jul 28, 2026
@coderabbitai

coderabbitai Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: aec5dee1-2fc9-4fe4-8aac-168dcd7447d6

📥 Commits

Reviewing files that changed from the base of the PR and between a93184d and 6b98872.

📒 Files selected for processing (1)
  • core/wren/tests/connectors/test_snowflake.py

Walkthrough

Snowflake connector tests now patch the correct connection function and verify that query limits are pushed into SQL without client-side Arrow table slicing.

Changes

Snowflake connector tests

Layer / File(s) Summary
Connection mock targets
core/wren/tests/connectors/test_snowflake.py
Authentication and connector factory tests patch snowflake.connector.connect while preserving connection assertions.
SQL limit pushdown assertions
core/wren/tests/connectors/test_snowflake.py
The limit test checks for LIMIT 2 in executed SQL and expects the complete mocked Arrow table.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

Suggested reviewers: abdullahharunozturk

Poem

A bunny checks the Snowflake gate,
With proper mocks aligned in place.
SQL now limits what streams through,
The full table hops back true.
Carrots cheer the cleaner trace! 🐇

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: fixing the Snowflake test mock patch target so the suite runs.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/snowflake-test-mock-target

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@goldmedal
goldmedal requested a review from douenergy July 28, 2026 08:32
@douenergy
douenergy merged commit db0667f into main Jul 28, 2026
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core python Pull requests that update Python code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants