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 54 minutes and 20 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 contains updates to two documentation files. The first adds a new section to the testing documentation explaining how database fixtures manage transactions during test execution and provides guidance for migration tests involving multiple database interactions. The second updates migration documentation to describe RFC 9457-compliant error response formats with standardized fields and corrects HTTP status code mappings for specific error scenarios. Both changes are documentation-only with no modifications to code. Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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 2 issues, and left some high level feedback:
- In
migration.md, under 'Appropriate HTTP Status Codes', fix minor wording issues like 'There are several cases were the PHP server...' → 'where' and consider tightening phrases like 'aims to correct that' to something more precise (e.g., 'corrects this'). - In
tests.md, the sentence 'We do not want our tests to side effects' is missing 'have' and should be clarified (e.g., 'We do not want our tests to have side effects, as they might lead to inconsistent behavior').
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `migration.md`, under 'Appropriate HTTP Status Codes', fix minor wording issues like 'There are several cases were the PHP server...' → 'where' and consider tightening phrases like 'aims to correct that' to something more precise (e.g., 'corrects this').
- In `tests.md`, the sentence 'We do not want our tests to side effects' is missing 'have' and should be clarified (e.g., 'We do not want our tests to have side effects, as they might lead to inconsistent behavior').
## Individual Comments
### Comment 1
<location path="docs/migration.md" line_range="37" />
<code_context>
+You will notice that the response still contains a "code" of "111" (though as a top level property not embedded in the "error" scope).
+This field is included to support the migration of clients, but should be considered deprecated.
+As per the RFC9457 standard, the "type" field now includes the unique code for the error.
+The "title" field is a human readable summary of the general issue and the "detail" case may provide additional information for the specific request.
+They _will_ be resolvable URIs in the future, providing a page with more information.
+
</code_context>
<issue_to_address>
**issue (typo):** Consider changing "detail" case" to "detail" field" for grammatical correctness.
Using "field" for both "title" and "detail" will keep the terminology consistent and clearer, e.g. "the \"detail\" field may provide additional information for the specific request."
```suggestion
As per the RFC9457 standard, the "type" field now includes the unique code for the error.
The "title" field is a human readable summary of the general issue and the "detail" field may provide additional information for the specific request.
```
</issue_to_address>
### Comment 2
<location path="docs/migration.md" line_range="44" />
<code_context>
+In some cases the JSON endpoints previously returned XML ([example](https://github.com/openml/OpenML/issues/1200)), the new API always returns JSON.
+
+# Appropriate HTTP Status Codes
+There are several cases were the PHP server did not provide semantically correct status codes.
+The Python server aims to correct that.
+The errors that changed which are most likely to occur are probably errors when there is no result, or when the input is incorrect.
</code_context>
<issue_to_address>
**issue (typo):** Typo: use "where" instead of "were".
Use: "There are several cases where the PHP server did not provide semantically correct status codes."
```suggestion
There are several cases where the PHP server did not provide semantically correct status codes.
```
</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 #316 +/- ##
=======================================
Coverage 93.53% 93.53%
=======================================
Files 67 67
Lines 3109 3109
Branches 221 221
=======================================
Hits 2908 2908
Misses 143 143
Partials 58 58 ☔ 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: 4
🤖 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`:
- Line 143: Fix the grammar in the sentence that currently reads "do not want
our tests to side effects" by changing it to "do not want our tests to have side
effects" wherever that exact phrasing occurs (the sentence discussing tests and
side effects in the contributing/tests.md content).
- Around line 145-148: Update the "migration tests" section to document the
known limitation: state that migration tests for OpenML API endpoints that use
the PHP API for staging cannot assert certain authentication-failure paths
(specifically Code 103) when the legacy PHP API returns XML instead of JSON, and
add guidance to instead cover those scenarios in standard Python unit tests
after migration; reference the terms "migration tests", "php_api", "Code 103",
and "authentication-failure" so readers can find and implement the appropriate
post-migration Python unit tests.
In `@docs/migration.md`:
- Around line 38-44: Replace the wording typos in the migration doc: change
"detail case" to "detail field" and "cases were" to "cases where" in the section
that includes the sentences "The 'title' field..." and "# Appropriate HTTP
Status Codes" so the phrasing reads correctly and clearly.
- Line 32: The example response JSON in docs/migration.md has a mismatched
dataset id: the curl request references dataset 1000000 but the JSON "detail"
says 10000; update the JSON "detail" (and "code" if it encodes the id) so the
dataset id matches the curl (e.g., change "No dataset with id 10000 found." to
"No dataset with id 1000000 found.") and ensure any other example fields
referencing the id are consistent (check the JSON object containing
"type","title","status","detail","code").
🪄 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: b7a8d94e-fdb5-473c-852f-b695c12960c8
📒 Files selected for processing (3)
docs/contributing/tests.mddocs/migration.mdtests/routers/openml/estimation_procedure_test.py
| There is one situation where you may need to commit to the database: migration tests. | ||
| Since the PHP API communicates to the database in a separate transaction, changes made within the transaction in "Python land" are not visible to PHP. | ||
| In this case, be extremely careful! You *must* write the test so that even if things fail unexpectedly, there is no data left behind. | ||
| Generally speaking, you want to use a context manager that cleans up after you. In some cases you may need to clean up after yourself during the test. |
There was a problem hiding this comment.
Add the known migration-test limitation for auth failure paths.
This section should mention that some authentication-failure scenarios cannot be asserted through migration tests when the PHP side returns XML instead of JSON, and should be covered in Python unit tests post-migration.
Suggested doc addition
There is one situation where you may need to commit to the database: migration tests.
Since the PHP API communicates to the database in a separate transaction, changes made within the transaction in "Python land" are not visible to PHP.
In this case, be extremely careful! You *must* write the test so that even if things fail unexpectedly, there is no data left behind.
Generally speaking, you want to use a context manager that cleans up after you. In some cases you may need to clean up after yourself during the test.
+
+Also note: for some legacy endpoints, authentication failure paths are not suitable for migration comparison tests if the PHP API returns XML error payloads. Cover those in Python unit tests once the dependent endpoint is migrated.Based on learnings: Migration tests for OpenML API endpoints that rely on php_api for 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.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| There is one situation where you may need to commit to the database: migration tests. | |
| Since the PHP API communicates to the database in a separate transaction, changes made within the transaction in "Python land" are not visible to PHP. | |
| In this case, be extremely careful! You *must* write the test so that even if things fail unexpectedly, there is no data left behind. | |
| Generally speaking, you want to use a context manager that cleans up after you. In some cases you may need to clean up after yourself during the test. | |
| There is one situation where you may need to commit to the database: migration tests. | |
| Since the PHP API communicates to the database in a separate transaction, changes made within the transaction in "Python land" are not visible to PHP. | |
| In this case, be extremely careful! You *must* write the test so that even if things fail unexpectedly, there is no data left behind. | |
| Generally speaking, you want to use a context manager that cleans up after you. In some cases you may need to clean up after yourself during the test. | |
| Also note: for some legacy endpoints, authentication failure paths are not suitable for migration comparison tests if the PHP API returns XML error payloads. Cover those in Python unit tests once the dependent endpoint is migrated. |
🧰 Tools
🪛 LanguageTool
[style] ~147-~147: As an alternative to the over-used intensifier ‘extremely’, consider replacing this phrase.
Context: ...re not visible to PHP. In this case, be extremely careful! You must write the test so that even...
(EN_WEAK_ADJECTIVE)
[style] ~148-~148: Consider using “who” when you are referring to a person instead of an object.
Context: ...king, you want to use a context manager that cleans up after you. In some cases you ...
(THAT_WHO)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/contributing/tests.md` around lines 145 - 148, Update the "migration
tests" section to document the known limitation: state that migration tests for
OpenML API endpoints that use the PHP API for staging cannot assert certain
authentication-failure paths (specifically Code 103) when the legacy PHP API
returns XML instead of JSON, and add guidance to instead cover those scenarios
in standard Python unit tests after migration; reference the terms "migration
tests", "php_api", "Code 103", and "authentication-failure" so readers can find
and implement the appropriate post-migration Python unit tests.
1 participant
No description provided.