Separate out test files to one file per endpoint#295
Merged
Conversation
Contributor
|
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 as they are similar to previous changes (1)
WalkthroughThis PR reorganizes OpenML router tests: it removes several existing tests (qualities, some setup/tag, task-type list, and flows_get tests), and adds new dedicated test modules covering flows existence, qualities listing, setup retrieval and tagging, study attachment and creation, and task-type list comparisons. Some raw-SQL helper functions and duplicated HTTP-level comparison tests were removed; new tests use async DB fixtures and API clients. Net change: multiple test files added and removed across the suite. 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 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 |
![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 1 issue, and left some high level feedback:
- In
study_attach_test._attach_tasks_to_study, theUPDATE study SET status = 'in_preparation' WHERE id = 1statement is hard-coded toid = 1and ignores thestudy_idargument, which will cause incorrect behavior if the helper is reused for other study IDs—consider parameterizing the query withstudy_id.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `study_attach_test._attach_tasks_to_study`, the `UPDATE study SET status = 'in_preparation' WHERE id = 1` statement is hard-coded to `id = 1` and ignores the `study_id` argument, which will cause incorrect behavior if the helper is reused for other study IDs—consider parameterizing the query with `study_id`.
## Individual Comments
### Comment 1
<location path="tests/routers/openml/study_attach_test.py" line_range="20-23" />
<code_context>
+from tests.users import ApiKey
+
+
+async def _attach_tasks_to_study(
+ study_id: int,
+ task_ids: list[int],
+ api_key: str,
+ py_api: httpx.AsyncClient,
+ expdb_test: AsyncConnection,
+) -> httpx.Response:
+ # Adding requires the study to be in preparation,
+ # but the current snapshot has no in-preparation studies.
+ await expdb_test.execute(text("UPDATE study SET status = 'in_preparation' WHERE id = 1"))
+ return await py_api.post(
+ f"/studies/attach?api_key={api_key}",
</code_context>
<issue_to_address>
**suggestion:** Use `study_id` parameter instead of hard-coded `id = 1` to make the helper more robust and self-consistent
The helper takes a `study_id` argument but the SQL still hard-codes `WHERE id = 1`, which couples it to current usages and makes future calls with other `study_id` values brittle. Please change the query to `WHERE id = :study_id` and pass `study_id` as a parameter so the test behavior matches the function signature.
```suggestion
# Adding requires the study to be in preparation,
# but the current snapshot has no in-preparation studies.
await expdb_test.execute(
text("UPDATE study SET status = 'in_preparation' WHERE id = :study_id"),
{"study_id": study_id},
)
return await py_api.post(
```
</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 #295 +/- ##
=======================================
Coverage 54.20% 54.20%
=======================================
Files 38 38
Lines 1581 1581
Branches 137 137
=======================================
Hits 857 857
Misses 722 722
Partials 2 2 ☔ 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.
Actionable comments posted: 2
🧹 Nitpick comments (4)
tests/routers/openml/qualities_list_test.py (1)
162-167: Avoid mutating the baseline expected payload in-place.At Line 162, mutating
expectedwith.pop()makes the intent a bit harder to read. Keeping the original expected response immutable improves clarity.♻️ Suggested refactor
- deleted = expected["data_qualities_list"]["quality"].pop() + deleted = expected["data_qualities_list"]["quality"][-1] + expected_after_delete = { + "data_qualities_list": { + "quality": expected["data_qualities_list"]["quality"][:-1], + }, + } await _remove_quality_from_database(quality_name=deleted, expdb_test=expdb_test) response = await py_api.get("/datasets/qualities/list") assert response.status_code == HTTPStatus.OK - assert expected == response.json() + assert expected_after_delete == response.json()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/routers/openml/qualities_list_test.py` around lines 162 - 167, The test mutates the baseline `expected` payload by doing `deleted = expected["data_qualities_list"]["quality"].pop()`, which makes the expected response harder to reason about; instead, read the value without modifying `expected` (e.g., pick the last element from `expected["data_qualities_list"]["quality"]` or make a shallow copy of that list) and pass that value to `_remove_quality_from_database(quality_name=..., expdb_test=expdb_test)` so the subsequent `response.json()` can be compared against the original immutable `expected`. Ensure you keep the identifier `deleted` (or a similarly named variable) and leave `expected` unchanged before the GET and assert.tests/routers/openml/setups_tag_test.py (1)
32-34: Consider using parameterized SQL for consistency.While this is test code and the values are hardcoded strings (not user input), using parameterized queries is a good habit that maintains consistency with production code patterns.
♻️ Optional: Use parameterized SQL
await expdb_test.execute( - text("INSERT INTO setup_tag (id, tag, uploader) VALUES (1, 'existing_tag_123', 2);") + text("INSERT INTO setup_tag (id, tag, uploader) VALUES (:id, :tag, :uploader)"), + {"id": 1, "tag": "existing_tag_123", "uploader": 2}, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/routers/openml/setups_tag_test.py` around lines 32 - 34, Replace the hardcoded SQL string in the test by using a parameterized query: change the text("INSERT INTO setup_tag (id, tag, uploader) VALUES (1, 'existing_tag_123', 2);") passed to expdb_test.execute into a text() with named placeholders (e.g., :id, :tag, :uploader) and supply the parameter dict to expdb_test.execute so the INSERT uses bound parameters instead of inlined values.tests/routers/openml/study_post_test.py (1)
31-45: Consider extracting expected values that depend on test fixtures.The hardcoded
"creator": 2ties the test to the assumption thatApiKey.SOME_USERmaps to user ID 2. If the test user configuration changes, this test will fail with a confusing error.♻️ Optional: Make creator expectation more explicit
Consider adding a comment or extracting to a constant:
# ApiKey.SOME_USER corresponds to user_id=2 in test fixtures SOME_USER_ID = 2 # ... "creator": SOME_USER_ID,Or import/reference from the test user configuration if available.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/routers/openml/study_post_test.py` around lines 31 - 45, The expected dict hardcodes "creator": 2 which couples the test to a specific fixture mapping; change the test to derive the creator id from the test fixture or ApiKey mapping instead of using 2. Locate the expected dict named expected and replace the literal for the "creator" key with a reference to the actual test user id (e.g., a fixture value, a constant like SOME_USER_ID imported from your test user config, or by resolving ApiKey.SOME_USER to its id) so the test remains stable if fixture IDs change.tests/routers/openml/study_attach_test.py (1)
46-46: Redundant database UPDATE before calling helper.The
_attach_tasks_to_studyhelper already executes the UPDATE statement to set the study status. The additional UPDATE on line 46 (and similarly on lines 62, 82) is redundant.♻️ Remove redundant UPDATE calls
async def test_attach_task_to_study_needs_owner( py_api: httpx.AsyncClient, expdb_test: AsyncConnection ) -> None: - await expdb_test.execute(text("UPDATE study SET status = 'in_preparation' WHERE id = 1")) response = await _attach_tasks_to_study(Apply similar removal to lines 62 and 82.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/routers/openml/study_attach_test.py` at line 46, Remove the redundant direct UPDATE calls that set study.status to 'in_preparation' before invoking the helper; the _attach_tasks_to_study test helper already performs that UPDATE, so delete the await expdb_test.execute(text("UPDATE study SET status = 'in_preparation' WHERE id = 1")) occurrences (and the similar two duplicates) in tests/routers/openml/study_attach_test.py so the test relies only on _attach_tasks_to_study to change the study status.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/routers/openml/flows_exists_test.py`:
- Around line 43-46: The tests patch the wrong import path for get_by_name;
update the mock.patch targets in all three tests
(test_flow_exists_calls_db_correctly, test_flow_exists_processes_found,
test_flow_exists_handles_flow_not_found) so they patch where
routers.openml.flows looks up the name (i.e., replace
"database.flows.get_by_name" with
"routers.openml.flows.database.flows.get_by_name" for the mocked_db patch in
each test) so the AsyncMock actually intercepts calls inside the flows.exists
handler.
In `@tests/routers/openml/study_attach_test.py`:
- Line 22: The UPDATE in tests/routers/openml/study_attach_test.py currently
hardcodes "WHERE id = 1" and ignores the helper's study_id parameter; change the
SQL to use a parameterized WHERE clause (e.g. "WHERE id = :study_id") and pass
the study_id when calling expdb_test.execute, so the UPDATE targets the provided
study_id (use a bound param dict like {"study_id": study_id} or equivalent with
text()).
---
Nitpick comments:
In `@tests/routers/openml/qualities_list_test.py`:
- Around line 162-167: The test mutates the baseline `expected` payload by doing
`deleted = expected["data_qualities_list"]["quality"].pop()`, which makes the
expected response harder to reason about; instead, read the value without
modifying `expected` (e.g., pick the last element from
`expected["data_qualities_list"]["quality"]` or make a shallow copy of that
list) and pass that value to `_remove_quality_from_database(quality_name=...,
expdb_test=expdb_test)` so the subsequent `response.json()` can be compared
against the original immutable `expected`. Ensure you keep the identifier
`deleted` (or a similarly named variable) and leave `expected` unchanged before
the GET and assert.
In `@tests/routers/openml/setups_tag_test.py`:
- Around line 32-34: Replace the hardcoded SQL string in the test by using a
parameterized query: change the text("INSERT INTO setup_tag (id, tag, uploader)
VALUES (1, 'existing_tag_123', 2);") passed to expdb_test.execute into a text()
with named placeholders (e.g., :id, :tag, :uploader) and supply the parameter
dict to expdb_test.execute so the INSERT uses bound parameters instead of
inlined values.
In `@tests/routers/openml/study_attach_test.py`:
- Line 46: Remove the redundant direct UPDATE calls that set study.status to
'in_preparation' before invoking the helper; the _attach_tasks_to_study test
helper already performs that UPDATE, so delete the await
expdb_test.execute(text("UPDATE study SET status = 'in_preparation' WHERE id =
1")) occurrences (and the similar two duplicates) in
tests/routers/openml/study_attach_test.py so the test relies only on
_attach_tasks_to_study to change the study status.
In `@tests/routers/openml/study_post_test.py`:
- Around line 31-45: The expected dict hardcodes "creator": 2 which couples the
test to a specific fixture mapping; change the test to derive the creator id
from the test fixture or ApiKey mapping instead of using 2. Locate the expected
dict named expected and replace the literal for the "creator" key with a
reference to the actual test user id (e.g., a fixture value, a constant like
SOME_USER_ID imported from your test user config, or by resolving
ApiKey.SOME_USER to its id) so the test remains stable if fixture IDs change.
🪄 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: 5b30d9d7-dba2-4a84-9d45-4fe73bc45928
📒 Files selected for processing (14)
tests/routers/openml/datasets_qualities_test.pytests/routers/openml/flows_exists_test.pytests/routers/openml/flows_get_test.pytests/routers/openml/qualities_list_test.pytests/routers/openml/runs_trace_test.pytests/routers/openml/setups_get_test.pytests/routers/openml/setups_tag_test.pytests/routers/openml/setups_untag_test.pytests/routers/openml/study_attach_test.pytests/routers/openml/study_get_test.pytests/routers/openml/study_post_test.pytests/routers/openml/task_get_test.pytests/routers/openml/task_type_get_test.pytests/routers/openml/task_type_list_test.py
💤 Files with no reviewable changes (4)
- tests/routers/openml/task_type_get_test.py
- tests/routers/openml/setups_untag_test.py
- tests/routers/openml/flows_get_test.py
- tests/routers/openml/datasets_qualities_test.py
PGijsbers
added a commit
that referenced
this pull request
Mar 27, 2026
I accidentally only copied over some tests, instead of moving them in #295. This fixes that.
4 tasks
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.
Reorganizing the tests to one file per endpoint, because:
Tests under
migrationare left out for now, but will be moved as well.