Improve consistency of test variable names and ordering#310
Conversation
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughRefactors many test files: swapped equality assertion operand order (e.g., 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 |
PGijsbers
changed the title
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is ❌ Your patch status has failed because the patch coverage (89.14%) is below the target coverage (100.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #310 +/- ##
==========================================
- Coverage 93.57% 93.57% -0.01%
==========================================
Files 72 72
Lines 3113 3112 -1
Branches 222 220 -2
==========================================
- Hits 2913 2912 -1
Misses 143 143
Partials 57 57 ☔ 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.
🧹 Nitpick comments (2)
tests/routers/openml/migration/setups_migration_test.py (1)
101-105: Avoid order-dependent unpacking fromerror.values().Line 101 relies on JSON key order. Read
codeandmessageby key to make the assertion stable.Suggested patch
- code, message = php_response.json()["error"].values() + error = php_response.json()["error"] + code = error["code"] + message = error["message"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/routers/openml/migration/setups_migration_test.py` around lines 101 - 105, The test unpacks php_response.json()["error"].values() which is order-dependent; instead extract the error dict and read keys explicitly (e.g. error = php_response.json()["error"]; use error["code"] and error["message"]) and then assert py_response.json()["code"] equals that code and message equals "Tag is not owned by you" to make the assertions stable; update references to php_response and py_response accordingly.tests/routers/openml/migration/flows_migration_test.py (1)
69-69: Assert PHP status explicitly before parsing payload.On Line 69 only
py_responseis assertedOK; then Line 92 assumes PHP success shape (["flow"]). Add an explicitassert php_response.status_code == HTTPStatus.OKto fail with a clearer reason.Suggested patch
- assert py_response.status_code == HTTPStatus.OK + assert py_response.status_code == HTTPStatus.OK + assert php_response.status_code == HTTPStatus.OKAlso applies to: 92-92
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/routers/openml/migration/flows_migration_test.py` at line 69, Add an explicit assertion that php_response.status_code == HTTPStatus.OK before any parsing or indexing of php_response.json() (the test currently only asserts py_response.status_code == HTTPStatus.OK), so modify the test around the php_response usage (referencing the php_response variable and HTTPStatus.OK) to fail fast with a clear message if the PHP call did not succeed.
🤖 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/migration/flows_migration_test.py`:
- Line 69: Add an explicit assertion that php_response.status_code ==
HTTPStatus.OK before any parsing or indexing of php_response.json() (the test
currently only asserts py_response.status_code == HTTPStatus.OK), so modify the
test around the php_response usage (referencing the php_response variable and
HTTPStatus.OK) to fail fast with a clear message if the PHP call did not
succeed.
In `@tests/routers/openml/migration/setups_migration_test.py`:
- Around line 101-105: The test unpacks php_response.json()["error"].values()
which is order-dependent; instead extract the error dict and read keys
explicitly (e.g. error = php_response.json()["error"]; use error["code"] and
error["message"]) and then assert py_response.json()["code"] equals that code
and message equals "Tag is not owned by you" to make the assertions stable;
update references to php_response and py_response accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 12c2706c-2c0e-4244-8f3a-741f71cfd7fd
📒 Files selected for processing (19)
tests/config_test.pytests/dependencies/fetch_user_test.pytests/routers/openml/dataset_tag_test.pytests/routers/openml/datasets_list_datasets_test.pytests/routers/openml/datasets_qualities_test.pytests/routers/openml/migration/datasets_migration_test.pytests/routers/openml/migration/evaluations_migration_test.pytests/routers/openml/migration/flows_migration_test.pytests/routers/openml/migration/runs_migration_test.pytests/routers/openml/migration/setups_migration_test.pytests/routers/openml/migration/studies_migration_test.pytests/routers/openml/migration/tasks_migration_test.pytests/routers/openml/qualities_list_test.pytests/routers/openml/setups_tag_test.pytests/routers/openml/setups_untag_test.pytests/routers/openml/study_post_test.pytests/routers/openml/task_list_test.pytests/routers/openml/task_type_get_test.pytests/routers/openml/task_type_list_test.py
1 participant
Update the tests to be more consistent with variable naming and ordering in assertions.