Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 50 minutes and 15 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThis pull request updates developer-facing documentation and pytest configuration. It modifies the Docker Compose command in the docs service instructions to explicitly reference compose files. The tests documentation is reorganized to focus on REST API test execution and authoring with new testing conventions, including marker-based test categorization and endpoint testing patterns. Section headings are adjusted in setup documentation, and navigation is updated in mkdocs.yml. Pytest marker declarations in pyproject.toml are updated with revised descriptions and two new markers ( Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The examples using
py_api: httpx.AsyncClientin the docs are written as synchronous test functions and call.get()withoutawait; consider updating them toasync deftests thatawait py_api.get(...)to match actual async usage and avoid confusion. - In the test examples you use
HTTPStatus.SUCCESS, which is not a standardhttp.HTTPStatusmember; it may be clearer to use a real status constant (e.g.HTTPStatus.OK) or explicitly note that it is a placeholder. - The
php_apimarker description inpyproject.tomlcurrently says "requests the py_api fixture"; this likely should say "requests the php_api fixture" to accurately document its behavior.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The examples using `py_api: httpx.AsyncClient` in the docs are written as synchronous test functions and call `.get()` without `await`; consider updating them to `async def` tests that `await py_api.get(...)` to match actual async usage and avoid confusion.
- In the test examples you use `HTTPStatus.SUCCESS`, which is not a standard `http.HTTPStatus` member; it may be clearer to use a real status constant (e.g. `HTTPStatus.OK`) or explicitly note that it is a placeholder.
- The `php_api` marker description in `pyproject.toml` currently says "requests the py_api fixture"; this likely should say "requests the php_api fixture" to accurately document its behavior.
## Individual Comments
### Comment 1
<location path="pyproject.toml" line_range="130-133" />
<code_context>
+ "mut: executes a mutation on the database (even when it is cleaned up).",
+ # *_api markers included for documentation, should not be added manually.
+ "py_api: requests the py_api fixture. Added automatically.",
+ "php_api: requests the py_api fixture. Added automatically.",
]
filterwarnings = [
</code_context>
<issue_to_address>
**issue (typo):** The `php_api` marker description likely refers to the wrong fixture.
The description for `php_api` likely should say it requests the `php_api` fixture rather than `py_api` to avoid confusion. If it intentionally uses `py_api` under the hood, please clarify that in the wording (e.g., that `php_api` is implemented via `py_api`).
```suggestion
# *_api markers included for documentation, should not be added manually.
"py_api: requests the py_api fixture. Added automatically.",
"php_api: requests the php_api fixture (implemented via py_api). Added automatically.",
]
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #314 +/- ##
==========================================
+ Coverage 93.55% 93.58% +0.03%
==========================================
Files 73 73
Lines 3119 3119
Branches 221 221
==========================================
+ Hits 2918 2919 +1
Misses 143 143
+ Partials 58 57 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
docs/contributing/tests.md (1)
95-101: Add the known migration-test auth limitation to this section.Please document that some authentication-failure paths cannot be validated via migration tests when the PHP endpoint returns XML instead of JSON, and that those cases belong in Python-side unit tests.
Based on learnings: Migration tests for OpenML API endpoints that rely on
php_apifor data staging cannot test authentication failure paths (Code 103) when the legacy PHP API returns XML instead of JSON; these paths should be tested in standard Python unit tests after dependent endpoints are migrated.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/contributing/tests.md` around lines 95 - 101, Add a short note to the "migration tests" section explaining the known auth-testing limitation: state that migration tests calling both APIs cannot validate authentication-failure paths (Code 103) for endpoints that stage data via php_api when the legacy PHP endpoint returns XML instead of JSON, and instruct readers to cover those cases in Python-side unit tests after migration; reference "migration tests", "php_api", and "Code 103" so maintainers can locate and understand the constraint.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/contributing/tests.md`:
- Around line 27-29: Add missing language identifiers to the fenced code blocks
in docs/contributing/tests.md: tag the docker-compose command block with "bash"
(the block containing `docker compose exec python-api python -m pytest tests -m
"not php_api and not slow"`) and tag the Python test block with "python" (the
block starting with `async def test_get_dataset(py_api: httpx.AsyncClient,
php_api: httpx.AsyncClient) -> None:`) so they satisfy MD040 and improve
readability.
- Around line 85-90: The test test_get_dataset_private_access_denied sets
owner_of_that_dataset = OWNER_USER but then asserts a FORBIDDEN error, so change
the principal passed to get_dataset to a non-owner user (e.g., use a
NON_OWNER_USER constant or a different user fixture) and ensure the dataset id
(private_dataset / 42) is still owned by OWNER_USER; update the variable name
(e.g., non_owner_user) and pass that to get_dataset(user=...) so the call
correctly triggers DatasetNoAccessError in get_dataset.
- Line 47: The guidance incorrectly states tests can be filtered out with the
"php_api" marker; update the sentence to say these slow tests should be marked
with `slow` and show the correct pytest filter (e.g., `-m "not slow"`),
replacing the reference to `php_api` with `slow` so the line clearly advises
using the `slow` marker to exclude these tests.
- Around line 62-74: The two test functions
test_get_dataset_identifier_validation and test_get_dataset_success are
currently defined with def but call py_api.get() asynchronously; change their
signatures to async def and await the py_api.get(...) calls (e.g., await
py_api.get("/datasets/1")) and keep/assert the expected HTTPStatus
(HTTPStatus.OK) and expected JSON result in test_get_dataset_success; update
both functions accordingly so they run as async tests.
In `@pyproject.toml`:
- Line 132: The php_api marker description incorrectly says it "requests the
py_api fixture"; update the marker text in pyproject.toml so it correctly
references the php_api fixture (matching the actual fixture defined as php_api
in tests/conftest.py) — locate the php_api marker entry and replace the mention
of py_api with php_api to ensure the description and the fixture name (php_api)
are consistent.
---
Nitpick comments:
In `@docs/contributing/tests.md`:
- Around line 95-101: Add a short note to the "migration tests" section
explaining the known auth-testing limitation: state that migration tests calling
both APIs cannot validate authentication-failure paths (Code 103) for endpoints
that stage data via php_api when the legacy PHP endpoint returns XML instead of
JSON, and instruct readers to cover those cases in Python-side unit tests after
migration; reference "migration tests", "php_api", and "Code 103" so maintainers
can locate and understand the constraint.
🪄 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: 8195109b-278e-4c92-9695-234eb85d599a
📒 Files selected for processing (5)
docs/contributing/documentation.mddocs/contributing/setup.mddocs/contributing/tests.mdmkdocs.ymlpyproject.toml
1 participant
Update (parts of) the documentation to reflect current intentions.