Refactor setups_tag tests to use direct method calls for non-happy-path tests#298
Open
igennova wants to merge 2 commits intoopenml:mainfrom
Open
Refactor setups_tag tests to use direct method calls for non-happy-path tests#298igennova wants to merge 2 commits intoopenml:mainfrom
igennova wants to merge 2 commits intoopenml:mainfrom
Conversation
Contributor
WalkthroughTests in 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 1 issue, and left some high level feedback:
- In the success tests you assert
"my_*_tag" in ...["tag"]; if the tag is expected to match exactly, consider changing these to strict equality to catch unexpected formatting or concatenation issues.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the success tests you assert `"my_*_tag" in ...["tag"]`; if the tag is expected to match exactly, consider changing these to strict equality to catch unexpected formatting or concatenation issues.
## Individual Comments
### Comment 1
<location path="tests/routers/openml/setups_tag_test.py" line_range="29-30" />
<code_context>
- assert re.match(r"Setup \d+ not found.", response.json()["detail"])
+
+ assert response.status_code == HTTPStatus.OK
+ assert "my_new_success_api_tag" in response.json()["setup_tag"]["tag"]
+
+ rows = await expdb_test.execute(
</code_context>
<issue_to_address>
**suggestion (testing):** Use stricter equality assertions for tags instead of substring checks.
Both the API success test and the direct success test use `in` to check the tag value. If the API doesn’t intentionally add extra content around the tag, please assert the exact value (or full structure) instead, e.g.:
```python
assert response.json()["setup_tag"]["tag"] == "my_new_success_api_tag"
```
This makes the tests stricter and prevents false positives if additional text is ever added to the tag.
```suggestion
assert response.status_code == HTTPStatus.OK
assert response.json()["setup_tag"]["tag"] == "my_new_success_api_tag"
```
</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 #298 +/- ##
=======================================
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.
🧹 Nitpick comments (2)
tests/routers/openml/setups_tag_test.py (2)
42-42: Tighten exception regex to avoid permissive matches.Both patterns currently end with
.(any char in regex) and are unanchored, so they can pass on unintended messages. Use anchored patterns and escape the final period.Proposed assertion pattern fix
- with pytest.raises(SetupNotFoundError, match=r"Setup \d+ not found."): + with pytest.raises(SetupNotFoundError, match=r"^Setup \d+ not found\.$"): ... - with pytest.raises(TagAlreadyExistsError, match=r"Setup 1 already has tag 'existing_tag_123'."): + with pytest.raises( + TagAlreadyExistsError, + match=r"^Setup 1 already has tag 'existing_tag_123'\.$", + ):Also applies to: 56-56
🤖 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` at line 42, The regex in the pytest.raises calls for SetupNotFoundError is too permissive — update the match patterns used in the pytest.raises(SetupNotFoundError, match=...) assertions to anchor the start and end and escape the final period (e.g., change to a pattern equivalent to "^Setup \\d+ not found\\.$"); apply the same anchored/escaped fix to the other similar assertion referenced (the second pytest.raises at the other location) so the tests only match the exact expected messages.
32-35: Prefer bound SQL parameters in DB assertions.Inline SQL string literals are brittle for tags with quotes and harder to reuse; parameterized
text(...)keeps the assertions safer and clearer.Proposed query refactor
- rows = await expdb_test.execute( - text("SELECT * FROM setup_tag WHERE id = 1 AND tag = 'my_new_success_api_tag'") - ) + rows = await expdb_test.execute( + text("SELECT * FROM setup_tag WHERE id = :setup_id AND tag = :tag"), + {"setup_id": 1, "tag": "my_new_success_api_tag"}, + ) ... - rows = await expdb_test.execute( - text("SELECT * FROM setup_tag WHERE id = 1 AND tag = 'my_direct_success_tag'") - ) + rows = await expdb_test.execute( + text("SELECT * FROM setup_tag WHERE id = :setup_id AND tag = :tag"), + {"setup_id": 1, "tag": "my_direct_success_tag"}, + )Also applies to: 75-78
🤖 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 - 35, Replace the inline SQL literals in the test DB assertions with parameterized SQL using text(...) and bound parameters: update the expdb_test.execute calls that query the setup_tag table (currently using "id = 1 AND tag = 'my_new_success_api_tag'") to use named bind params (e.g., :id, :tag) and pass the values in the execute call (e.g., {"id": 1, "tag": "my_new_success_api_tag"}); make the same change for the other assertion that queries setup_tag (the second expdb_test.execute around the later assertion) so both queries use bound parameters instead of inline string literals.
🤖 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_tag_test.py`:
- Line 42: The regex in the pytest.raises calls for SetupNotFoundError is too
permissive — update the match patterns used in the
pytest.raises(SetupNotFoundError, match=...) assertions to anchor the start and
end and escape the final period (e.g., change to a pattern equivalent to "^Setup
\\d+ not found\\.$"); apply the same anchored/escaped fix to the other similar
assertion referenced (the second pytest.raises at the other location) so the
tests only match the exact expected messages.
- Around line 32-35: Replace the inline SQL literals in the test DB assertions
with parameterized SQL using text(...) and bound parameters: update the
expdb_test.execute calls that query the setup_tag table (currently using "id = 1
AND tag = 'my_new_success_api_tag'") to use named bind params (e.g., :id, :tag)
and pass the values in the execute call (e.g., {"id": 1, "tag":
"my_new_success_api_tag"}); make the same change for the other assertion that
queries setup_tag (the second expdb_test.execute around the later assertion) so
both queries use bound parameters instead of inline string literals.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: bd2301c9-25fe-43e9-abd7-cc6ab9b01255
📒 Files selected for processing (1)
tests/routers/openml/setups_tag_test.py
Contributor
Author
|
@PGijsbers This PR refactors setups_tag tests per the post-#295 structure (single HTTP happy path + direct handler calls). I plan to open additional small PRs for other endpoints the same way unless you’d prefer a different split. Thanks |
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.
Summary:
Following the testing guidelines from #295:
py_api.