test: guard import footprint (no heavy deps, fast cold start)#9
Conversation
- The small install and fast cold start are this client's core value prop (serverless / Lambda), but nothing failed CI if a future change imported pandas at module top or ballooned import time. - Add tests/test_import_footprint.py: a fresh subprocess installs a recording meta-path finder and asserts a bare `import snowflake_sql_api` makes no attempt to import heavy optional deps (pandas/pyarrow/numpy). Stubbing the forbidden names makes the check deterministic in any env: a module-top import is caught even when guarded by try/except and even where pandas is not installed. A second test asserts the import stays under a generous 2.5s budget (best of 3 samples, to stay non-flaky on shared runners). - Add scripts/benchmark.py: a dependency-free manual tool reporting cold-import time and package source size (excludes bytecode/deps). Informational, not a gate, not shipped in the wheel; locates the package relative to itself so it runs from a raw checkout.
📝 WalkthroughWalkthroughAdds ChangesImport Footprint Tooling
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 review |
✅ Action performedReview finished.
|
![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
🧹 Nitpick comments (1)
scripts/benchmark.py (1)
42-48: ⚡ Quick winAdd a timeout to each benchmark subprocess run.
Without a timeout, a stuck child process can hang the benchmark indefinitely. Bound execution so failures are deterministic.
Proposed patch
result = subprocess.run( [sys.executable, "-c", _TIMING_SCRIPT], capture_output=True, text=True, check=True, cwd=_REPO_ROOT, + timeout=30, )🤖 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 `@scripts/benchmark.py` around lines 42 - 48, Add a timeout parameter to the subprocess.run() call that executes _TIMING_SCRIPT to prevent the benchmark from hanging indefinitely if the child process gets stuck. Set an appropriate timeout value that allows normal execution while ensuring failures are deterministic rather than causing the benchmark to hang.
🤖 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 `@tests/test_import_footprint.py`:
- Around line 79-82: The _run() function needs to be modified to ensure
subprocess execution is deterministic and time-bounded. Add a cwd parameter to
the subprocess.run() call to explicitly set the working directory to the
repository root (ensuring the checked-out snowflake_sql_api is used instead of
any installed version), and add a timeout parameter to set a time limit on
subprocess execution to prevent CI hangs. Both parameters should be passed
directly to the subprocess.run() call along with the existing arguments.
---
Nitpick comments:
In `@scripts/benchmark.py`:
- Around line 42-48: Add a timeout parameter to the subprocess.run() call that
executes _TIMING_SCRIPT to prevent the benchmark from hanging indefinitely if
the child process gets stuck. Set an appropriate timeout value that allows
normal execution while ensuring failures are deterministic rather than causing
the benchmark to hang.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 63cb416b-a430-44f0-a364-e885ae36e4f4
📒 Files selected for processing (2)
scripts/benchmark.pytests/test_import_footprint.py
| def _run(script: str) -> subprocess.CompletedProcess: | ||
| return subprocess.run( | ||
| [sys.executable, "-c", script], capture_output=True, text=True | ||
| ) |
There was a problem hiding this comment.
Make subprocess execution deterministic to the checked-out repo and time-bounded.
_run() inherits caller cwd, so these tests can accidentally validate an installed snowflake_sql_api instead of this checkout. It also has no timeout, so CI can hang on a stuck child import.
Proposed patch
import subprocess
import sys
+from pathlib import Path
@@
TIMING_SAMPLES = 3
+_REPO_ROOT = Path(__file__).resolve().parent.parent
@@
def _run(script: str) -> subprocess.CompletedProcess:
return subprocess.run(
- [sys.executable, "-c", script], capture_output=True, text=True
+ [sys.executable, "-c", script],
+ capture_output=True,
+ text=True,
+ cwd=_REPO_ROOT,
+ timeout=30,
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _run(script: str) -> subprocess.CompletedProcess: | |
| return subprocess.run( | |
| [sys.executable, "-c", script], capture_output=True, text=True | |
| ) | |
| import subprocess | |
| import sys | |
| from pathlib import Path | |
| # ... existing code ... | |
| TIMING_SAMPLES = 3 | |
| _REPO_ROOT = Path(__file__).resolve().parent.parent | |
| # ... existing code ... | |
| def _run(script: str) -> subprocess.CompletedProcess: | |
| return subprocess.run( | |
| [sys.executable, "-c", script], | |
| capture_output=True, | |
| text=True, | |
| cwd=_REPO_ROOT, | |
| timeout=30, | |
| ) |
🧰 Tools
🪛 ast-grep (0.43.0)
[error] 79-81: Command coming from incoming request
Context: subprocess.run(
[sys.executable, "-c", script], capture_output=True, text=True
)
Note: [CWE-20].
(subprocess-from-request)
🤖 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 `@tests/test_import_footprint.py` around lines 79 - 82, The _run() function
needs to be modified to ensure subprocess execution is deterministic and
time-bounded. Add a cwd parameter to the subprocess.run() call to explicitly set
the working directory to the repository root (ensuring the checked-out
snowflake_sql_api is used instead of any installed version), and add a timeout
parameter to set a time limit on subprocess execution to prevent CI hangs. Both
parameters should be passed directly to the subprocess.run() call along with the
existing arguments.
1 participant
Summary
tests/test_import_footprint.py: a fresh subprocess installs a recording meta-path finder and asserts a bareimport snowflake_sql_apimakes no attempt to import heavy optional deps (pandas/pyarrow/numpy). Stubbing the forbidden names makes the check deterministic in any env, a module-top import is caught even when guarded bytry/exceptand even where pandas is not installed. A second test asserts the import stays under a generous 2.5s budget (best of 3 samples, to stay non-flaky on shared runners).scripts/benchmark.py: a dependency-free manual tool reporting cold-import time and package source size (excludes bytecode/deps). Informational, not a gate, not shipped in the wheel; locates the package relative to itself so it runs from a raw checkout.Review
External (Codex agentic, Kimi-agentic): first pass flagged three findings (non-deterministic leak check, single-sample timing, benchmark size/validation), all applied. Codex re-review of the amended diff: clean. Claude reviewer skipped (same harness).
Changes
tests/test_import_footprint.py(new, +120)scripts/benchmark.py(new, +82)Test plan
coverage run -m pytest && coverage report: 178 passed, 5 skipped, total 94% (>= 89% gate)pre-commit run --all-filesclean (ruff, black, mypy, yaml/toml, private-key)import pandasis detected with pandas absentpython scripts/benchmark.py: import ~450ms median, 90 KiB source;--runs 0rejectedSummary by CodeRabbit
Tests
Chores