test: add comprehensive special character tests for all fields - #155
test: add comprehensive special character tests for all fields#155NTLx wants to merge 7 commits into
Conversation
- Add tests/unit_tests/test_special_characters.py - Cover IDs, documents, metadata, and collection name validation - Test cases include SQL injection patterns, unicode, emoji, and special chars Resolves: oceanbase#133
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a unit-test module that verifies special-character handling in IDs, documents, metadata, and collection names using a mocked SQL executor. ChangesSpecial-character SQL tests
Estimated code review effort: 2 (Simple) | ~15 minutes 🚥 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
🤖 Fix all issues with AI agents
In `@tests/unit_tests/test_special_characters.py`:
- Around line 2-8: The tests in test_special_characters.py create variables
executed_sql and metadata_key_test but never assert anything; update the two
test methods to assert that the SQL and metadata strings contain properly
escaped fragments by using escape_string from pymysql.converters to compute
expected escaped values and comparing them to executed_sql and metadata_key_test
(e.g., assert expected in executed_sql and assert expected_meta in
metadata_key_test), which will both validate escaping behavior and eliminate the
F841 unused-variable warnings; reference the existing variables executed_sql,
metadata_key_test and the escape_string function to locate where to inject these
assertions.
| # Ensure local src/ is on sys.path | ||
| project_root = Path(__file__).parent.parent.parent | ||
| src_root = project_root / "src" | ||
| sys.path.insert(0, str(src_root)) |
There was a problem hiding this comment.
Usually the project is installed when running the tests, so these are not necessary.
There was a problem hiding this comment.
@frostming 谢谢提醒，已修复。
最新提交 (b1ef617) 删除了 project_root / src_root / sys.path.insert() 这三行，以及随之失去用处的 import sys / from pathlib import Path 导入和 noqa: E402 标记。现在测试完全依赖项目以 pip install -e . 方式安装，pytest 直接 from pyseekdb.client.client_base import BaseClient。
顺手把 CodeRabbit 在另一个线程里指出的 F841 也修了：test_metadata_special_characters 之前构造的 metadata_key_test 在 _collection_add 之前就断言，触发 UnboundLocalError；现在分成 value / key 两组调用，分别对 executed_sql 做了基于 pymysql.converters.escape_string 的转义断言。
本地 pytest tests/unit_tests/test_special_characters.py 4/4 通过。请 @hnwyllmm 复核，CI 也已重新触发。
- Update uv.lock to fix lock file consistency check - Add docstrings to MockClient class methods to improve docstring coverage - This resolves the CI quality check failure Closes oceanbase#155
…e#155 - Move test_long_collection_name.py to tests/integration_tests - Use environment variables for database connection - Add proper cleanup in fixtures - Change pytest.raises(Exception) to pytest.raises(TypeError) - Add assertions to verify special character escaping
- Add assertions to verify SQL escaping of IDs, documents, and metadata
- Use escape_string to compute expected escaped segments
- Fix incorrect column name ('key' -> '_id') and table name mapping in tests
- Remove unused imports and fix F841 linting issues
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
Hi @hnwyllmm! This PR is now MERGEABLE with all CI checks passing. Summary:
Ready for merge! |
- Remove project_root/src_root/sys.path.insert() so the test relies on
the installed package (pyseekdb is installed in the CI environment
via `pip install -e .`).
- Drop now-unused `import sys` / `from pathlib import Path` and the
E402 noqa markers.
- Rework test_metadata_special_characters: the previous version built
a key-shaped metadata dict and asserted on `executed_sql` before
any call had populated it (UnboundLocalError / F841). Now both
{key: special_str} and {special_str: "value"} shapes are passed
through _collection_add and their JSON+SQL escaping is asserted via
pymysql.converters.escape_string.
Co-Authored-By: Claude Code noreply@anthropic.com
2 participants
Summary
Addressing issue #133.
Added a new unit test file
tests/unit_tests/test_special_characters.pyto verify that the client correctly handles special characters in:Tested scenarios include:
Test plan
Ran the new tests locally using
uv run pytest tests/unit_tests/test_special_characters.py -v.All 4 tests passed.
🤖 Generated with Claude Code
Summary by CodeRabbit