tests: refactor POST /setup/untag tests (API happy path + untag_setup)#304
Open
igennova wants to merge 2 commits intoopenml:mainfrom
Open
tests: refactor POST /setup/untag tests (API happy path + untag_setup)#304igennova wants to merge 2 commits intoopenml:mainfrom
igennova wants to merge 2 commits intoopenml:mainfrom
Conversation
Contributor
WalkthroughThe PR refactors tests for setup untagging: it removes HTTP-layer regex checks and replaces several API-level negative tests with direct unit tests that call Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- Several tests rely on hard-coded IDs and pre-existing DB state (e.g.,
setup_id=1and uploader2without inserting a corresponding setup), which makes them more brittle; consider inserting the required setup rows explicitly in each test (or via a fixture) so they are fully self-contained. - The
test_setup_untag_*file now mixes API-level tests and directuntag_setupunit-style tests; if this pattern grows, consider separating router/HTTP tests from service-level tests into different modules to keep test intent and responsibilities clearer.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Several tests rely on hard-coded IDs and pre-existing DB state (e.g., `setup_id=1` and uploader `2` without inserting a corresponding setup), which makes them more brittle; consider inserting the required setup rows explicitly in each test (or via a fixture) so they are fully self-contained.
- The `test_setup_untag_*` file now mixes API-level tests and direct `untag_setup` unit-style tests; if this pattern grows, consider separating router/HTTP tests from service-level tests into different modules to keep test intent and responsibilities clearer.
## Individual Comments
### Comment 1
<location path="tests/routers/openml/setups_untag_test.py" line_range="19-28" />
<code_context>
-async def test_setup_untag_unknown_setup(py_api: httpx.AsyncClient) -> None:
</code_context>
<issue_to_address>
**suggestion (testing):** Consider keeping at least one error-path test through the HTTP API to verify exception-to-HTTP mapping.
Previously, `test_setup_untag_unknown_setup` and `test_setup_untag_tag_not_found` hit the endpoint via `py_api` and asserted on HTTP status codes and error payloads. The new tests only call `untag_setup` directly and assert on custom exceptions, which no longer validates the HTTP contract (status codes like 404 vs 403, and the `detail` structure). Please reintroduce at least one negative-path test through `py_api` (e.g., unknown setup or tag-not-found) that asserts on the response status and JSON body to ensure routing and exception handling remain correct.
Suggested implementation:
```python
async def test_setup_untag_missing_auth(py_api: httpx.AsyncClient) -> None:
assert response.json()["detail"] == "No API key provided."
@pytest.mark.mut
async def test_setup_untag_unknown_setup_api(
py_api: httpx.AsyncClient,
owner_api_key: ApiKey,
) -> None:
unknown_setup_id = 999999999
response = await py_api.delete(
f"/openml/setups/{unknown_setup_id}/tags/some-tag",
headers={"X-Api-Key": owner_api_key.key},
)
assert response.status_code == 404
body = response.json()
assert body["detail"]["code"] == "setup_not_found"
assert body["detail"]["setup_id"] == unknown_setup_id
@pytest.mark.mut
async def test_setup_untag_api_success(
```
To make this compile and pass in your codebase, you may need to:
1. Ensure there is a fixture `owner_api_key: ApiKey` (or similar) that provides a valid API key for the owner user. If your fixtures differ, adjust the test signature and `headers=...` line accordingly (e.g., use `OWNER_USER.api_key_header` or whatever helper you already have).
2. Confirm the URL pattern for the untag endpoint; update `f"/openml/setups/{unknown_setup_id}/tags/some-tag"` to match your actual router path if it differs.
3. Align the expected error payload with your real exception-to-HTTP mapping. If your error response is shaped differently (e.g., `{"detail": "Setup not found"}` or `{"detail": {"type": "...", "msg": "..."}}`), update the `assert body["detail"][...]` lines to match the real structure.
4. If `httpx` or `pytest` are not already imported in this file, add the appropriate imports at the top: `import httpx` and `import pytest`.
</issue_to_address>
### Comment 2
<location path="tests/routers/openml/setups_untag_test.py" line_range="21-30" />
<code_context>
+ )
@pytest.mark.mut
@pytest.mark.parametrize(
- "api_key",
- [ApiKey.SOME_USER, ApiKey.ADMIN],
+ "user",
+ [SOME_USER, ADMIN_USER],
ids=["Owner", "Administrator"],
)
-async def test_setup_untag_success(
- api_key: str, py_api: httpx.AsyncClient, expdb_test: AsyncConnection
-) -> None:
+async def test_setup_untag_direct_success(user: User, expdb_test: AsyncConnection) -> None:
+ tag = "setup_untag_via_direct"
await expdb_test.execute(
- text("INSERT INTO setup_tag (id, tag, uploader) VALUES (1, 'test_success_tag', 2)")
+ text("INSERT INTO setup_tag (id, tag, uploader) VALUES (1, :tag, 2);"),
+ parameters={"tag": tag},
)
- response = await py_api.post(
- f"/setup/untag?api_key={api_key}",
- json={"setup_id": 1, "tag": "test_success_tag"},
+ result = await untag_setup(
+ setup_id=1,
+ tag=tag,
+ user=user,
+ expdb_db=expdb_test,
</code_context>
<issue_to_address>
**suggestion (testing):** Strengthen the "not owned" test by explicitly asserting that the tag row remains in the database after the forbidden operation.
In `test_setup_untag_not_owned_by_you`, after asserting that `untag_setup` raises `TagNotOwnedError`, also query `setup_tag` and assert that the row `(id=1, tag=tag, uploader=2)` still exists. This ensures the forbidden operation does not delete or mutate the tag record before raising the error.
</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 #304 +/- ##
==========================================
+ Coverage 93.04% 93.07% +0.03%
==========================================
Files 71 71
Lines 2991 3004 +13
Branches 221 221
==========================================
+ Hits 2783 2796 +13
Misses 152 152
Partials 56 56 ☔ 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}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/routers/openml/setups_untag_test.py (2)
25-45: Consider extracting shared arrange/assert DB helpers to reduce repetition.
setup_taginsert + post-condition deletion checks are repeated across multiple tests; a tiny helper would keep these tests shorter and easier to evolve.Also applies to: 73-77, 97-116
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/routers/openml/setups_untag_test.py` around lines 25 - 45, Extract repeated DB arrange/assert into small helper functions to reduce duplication: create helpers like insert_setup_tag(expdb_test, setup_id, tag, uploader=2) that wraps the expdb_test.execute(text("INSERT INTO setup_tag ..."), parameters=...) and assert_setup_tag_absent(expdb_test, setup_id, tag) that runs the SELECT used in the post-condition and asserts rows.all() is empty; then update tests that call expdb_test.execute and the SELECT assert (and the py_api.post usage with ApiKey.SOME_USER) to use these helpers (or place them in conftest or the test module) to keep tests shorter and consistent.
50-58: Keep one API-level negative-path smoke test to protect HTTP contract.These direct-call tests validate domain logic well, but they no longer assert router exception mapping (
status/code/detail) for negative paths. Consider retaining one lightweight API failure case (e.g., tag not found) so endpoint contract regressions are caught.Suggested addition
+@pytest.mark.mut +async def test_setup_untag_api_tag_not_found(py_api: httpx.AsyncClient) -> None: + missing_tag = "missing_tag_for_api_contract" + response = await py_api.post( + f"/setup/untag?api_key={ApiKey.SOME_USER}", + json={"setup_id": 1, "tag": missing_tag}, + ) + # Assert endpoint-level error contract (status + structured payload fields) + assert response.status_code != HTTPStatus.OK + assert "code" in response.json() + assert "detail" in response.json()Also applies to: 60-68, 72-87
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/routers/openml/setups_untag_test.py` around lines 50 - 58, Add one API-level negative-path test alongside the direct domain test: create a new test (e.g., test_api_setup_untag_unknown_setup) that uses the HTTP test client fixture to call the router endpoint that maps to untag_setup (simulate removing tag from setup_id=999999/tag="test_tag"), then assert the HTTP response maps the domain SetupNotFoundError to the expected status/code/detail (e.g., 404 and the "Setup <id> not found." detail) to protect the endpoint contract.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/routers/openml/setups_untag_test.py`:
- Around line 25-45: Extract repeated DB arrange/assert into small helper
functions to reduce duplication: create helpers like
insert_setup_tag(expdb_test, setup_id, tag, uploader=2) that wraps the
expdb_test.execute(text("INSERT INTO setup_tag ..."), parameters=...) and
assert_setup_tag_absent(expdb_test, setup_id, tag) that runs the SELECT used in
the post-condition and asserts rows.all() is empty; then update tests that call
expdb_test.execute and the SELECT assert (and the py_api.post usage with
ApiKey.SOME_USER) to use these helpers (or place them in conftest or the test
module) to keep tests shorter and consistent.
- Around line 50-58: Add one API-level negative-path test alongside the direct
domain test: create a new test (e.g., test_api_setup_untag_unknown_setup) that
uses the HTTP test client fixture to call the router endpoint that maps to
untag_setup (simulate removing tag from setup_id=999999/tag="test_tag"), then
assert the HTTP response maps the domain SetupNotFoundError to the expected
status/code/detail (e.g., 404 and the "Setup <id> not found." detail) to protect
the endpoint contract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6cce0488-28f5-41e0-9124-7db5409189bc
📒 Files selected for processing (1)
tests/routers/openml/setups_untag_test.py
Contributor
Author
|
@PGijsbers This PR is ready for review. Thank you! |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/routers/openml/setups_untag_test.py (1)
71-92: Good test for the authorization error path.The test correctly verifies:
- The exception is raised when a non-owner attempts removal
- The tag remains in the database after the failed operation
Minor observation: using
OWNER_USERfor a test where the user does not own the tag may be confusing to future readers. If this naming follows existing project conventions, consider adding a brief comment clarifying the relationship (e.g.,# OWNER_USER does not own this tag (uploader=2)).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/routers/openml/setups_untag_test.py` around lines 71 - 92, The test test_setup_untag_not_owned_by_you uses OWNER_USER while inserting the setup_tag row with uploader=2 which can be confusing; update the test to clarify that OWNER_USER does not own the tag by either using a different user constant or adding a one-line comment above the INSERT explaining "OWNER_USER does not own this tag (uploader=2)", referencing the test function name test_setup_untag_not_owned_by_you and the untag_setup call so reviewers see the intent and the setup_tag insert parameters clearly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/routers/openml/setups_untag_test.py`:
- Around line 71-92: The test test_setup_untag_not_owned_by_you uses OWNER_USER
while inserting the setup_tag row with uploader=2 which can be confusing; update
the test to clarify that OWNER_USER does not own the tag by either using a
different user constant or adding a one-line comment above the INSERT explaining
"OWNER_USER does not own this tag (uploader=2)", referencing the test function
name test_setup_untag_not_owned_by_you and the untag_setup call so reviewers see
the intent and the setup_tag insert parameters clearly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2098474a-6a85-482e-baf7-7201576b9afd
📒 Files selected for processing (1)
tests/routers/openml/setups_untag_test.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description