Update tests to reduce amount of times py_api fixture is used#293
Update tests to reduce amount of times py_api fixture is used#293
Conversation
for more information, see https://pre-commit.ci
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughTests for OpenML dataset endpoints were reorganized: many HTTP-level integration tests were converted to direct calls against service functions (tag_dataset, list_datasets, get_dataset, get_dataset_features, update_dataset_status), with assertions moved from HTTP responses to return values and raised exceptions. New test modules were added for datasets_get, datasets_features, and datasets_status; the previous HTTP-focused datasets_test module was removed. A few API-route tests remain. No exported/public application symbols were changed. Possibly related PRs
🚥 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}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In tests like
test_list_uploaderyou rely on object identity (user is SOME_USER); ifSOME_USERis not guaranteed to be a singleton instance this can behave unexpectedly, so consider comparing a stable attribute (e.g.user.id == SOME_USER.id) instead. - The new
test_rfc9457_error_formatonly exercises the error handler viaDatasetNotFoundError; if the intent is to cover the generic RFC9457 mapping for all dataset-related errors, consider adding at least one route-level example for a different error type (e.g.DatasetNoAccessError) to catch mis-wiring between exception types and the handler.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In tests like `test_list_uploader` you rely on object identity (`user is SOME_USER`); if `SOME_USER` is not guaranteed to be a singleton instance this can behave unexpectedly, so consider comparing a stable attribute (e.g. `user.id == SOME_USER.id`) instead.
- The new `test_rfc9457_error_format` only exercises the error handler via `DatasetNotFoundError`; if the intent is to cover the generic RFC9457 mapping for all dataset-related errors, consider adding at least one route-level example for a different error type (e.g. `DatasetNoAccessError`) to catch mis-wiring between exception types and the handler.
## Individual Comments
### Comment 1
<location path="tests/routers/openml/dataset_tag_test.py" line_range="26" />
<code_context>
async def test_dataset_tag_rejects_unauthorized(key: ApiKey, py_api: httpx.AsyncClient) -> None:
</code_context>
<issue_to_address>
**suggestion (testing):** Route-level unauthorized tag test no longer asserts the RFC 9457 error payload
Previously, this test verified the RFC 9457 error payload (`content-type`, `type`, and `code` = `AuthenticationFailedError.uri`, `103`); now it only checks the `401` status. Since other tests depend on consistent RFC 9457 formatting, consider keeping at least a minimal assertion on the error `type` and/or `code` so this route still has coverage for the expected problem details without reintroducing heavy `py_api` usage.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/routers/openml/datasets_get_test.py (1)
130-142: Consider separating thexfailcase into its own test.Using
pytest.mark.xfailforSOME_USERwithin a test that expects success for other users conflates two different behaviors (authorized access vs. denied access). If the access control logic changes to allowSOME_USER, this test would silently pass instead of alerting you.The denied-access scenario is already covered by
test_private_dataset_no_access. Consider removing thexfailparameter to keep this test focused on authorized access only.♻️ Suggested simplification
`@pytest.mark.parametrize`( - "user", [DATASET_130_OWNER, ADMIN_USER, pytest.param(SOME_USER, marks=pytest.mark.xfail)] + "user", [DATASET_130_OWNER, ADMIN_USER] ) async def test_private_dataset_access( user: User, expdb_test: AsyncConnection, user_test: AsyncConnection ) -> None:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/routers/openml/datasets_get_test.py` around lines 130 - 142, The parameterized test test_private_dataset_access mixes authorized-success cases and an xfail for SOME_USER; remove the pytest.param(SOME_USER, marks=pytest.mark.xfail) entry so the test only asserts successful access for DATASET_130_OWNER and ADMIN_USER, and if you still need to assert denied access for SOME_USER keep or extend the separate test_private_dataset_no_access test to cover that scenario instead.
🤖 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/datasets_get_test.py`:
- Around line 130-142: The parameterized test test_private_dataset_access mixes
authorized-success cases and an xfail for SOME_USER; remove the
pytest.param(SOME_USER, marks=pytest.mark.xfail) entry so the test only asserts
successful access for DATASET_130_OWNER and ADMIN_USER, and if you still need to
assert denied access for SOME_USER keep or extend the separate
test_private_dataset_no_access test to cover that scenario instead.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3af1919e-f663-4f31-a747-c2927b777e83
📒 Files selected for processing (5)
tests/routers/openml/datasets_features_test.pytests/routers/openml/datasets_get_test.pytests/routers/openml/datasets_list_datasets_test.pytests/routers/openml/datasets_status_test.pytests/routers/openml/datasets_test.py
💤 Files with no reviewable changes (1)
- tests/routers/openml/datasets_test.py
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #293 +/- ##
=======================================
Coverage 54.15% 54.15%
=======================================
Files 37 37
Lines 1553 1553
Branches 135 135
=======================================
Hits 841 841
Misses 710 710
Partials 2 2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1 participant
Working towards new guidelines for writing tests that provides more structure around when to use the
py_apifixture versus calling the methods directly.