Implements flight route tests #41
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces several changes across multiple files, including the removal of the Changes
Possibly related PRs
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (4)
pyproject.toml (1)
46-49: Consider more targeted rule suppressionsWhile some of these new pylint suppressions make sense in a testing context (particularly
redefined-outer-namefor pytest fixtures), others might be masking potential code quality issues:
too-many-argumentsandtoo-many-positional-arguments: These rules typically flag functions that might benefit from refactoring. Consider:
- Using dataclasses or parameter objects for complex parameter sets
- Breaking down complex functions into smaller ones
import-error: This should only be needed if you have conditional imports or complex import paths. Consider:
- Adding proper path configurations
- Using try/except for conditional imports instead of globally disabling the check
Instead of globally disabling these rules, consider:
- Using inline suppressions (
# pylint: disable=rule-name) only where necessary- Creating a separate pylint configuration for test files
- Refactoring complex functions to address the root cause
Would you like assistance in implementing any of these suggestions?
lib/models/rocket.py (1)
21-24: Consider refining the implementation of the__eq__methodThe custom
__eq__method compares the dictionaries of twoRocketinstances usingself.dict() == other.dict(). While this works for simple comparisons, it may not account for differences in fields that contain mutable data types or nested models, which could lead to unexpected behavior.Consider leveraging Pydantic's built-in comparison methods or ensure that all fields are comparable and immutable. Alternatively, you might want to define the
__hash__method if instances ofRocketneed to be used in hashed collections.tests/test_routes/test_rockets_route.py (1)
107-115: Simplify nested context managers for better readabilityYou can improve code readability by combining the nested
withstatements into a single line:with patch.object(RocketController, 'create_rocket', return_value=RocketCreated(rocket_id='123')) as mock_create_rocket, \ patch.object(Motor, 'set_motor_kind', side_effect=None) as mock_set_motor_kind: # Rest of the codeThis reduces indentation and makes the code cleaner.
lib/routes/rocket.py (1)
Documentation update needed for API endpoint change
The route path change is reflected in the test files, but the API documentation in the README.md needs to be updated. The sequence diagram still shows the old pattern
/model/:idinstead of the new/:id/modelpattern.
- Update the sequence diagram in README.md that shows:
to match the new pattern: LoadingUser ->> API: POST /model/:id/rocketpy🔗 Analysis chain
Line range hint
80-89: LGTM: Route path follows RESTful conventionsThe updated route path better follows RESTful conventions by using the resource/id/format pattern. This breaking change is acceptable as per previous team decisions.
Let's verify that all documentation is updated to reflect this change:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any outdated route references rg -i "rocketpy/\{.*id\}" --type md rg -i "rocketpy/.*id" --type rstLength of output: 221
Script:
#!/bin/bash # Search for any documentation files first fd -e md -e rst -e txt # Then search for the old route pattern in all text files rg -t txt -t md -t rst "rocketpy/[^/]+" # Also search for potential API documentation fd -g "*api*" -g "*doc*" -g "*swagger*" -g "*openapi*" -e yaml -e json -e md -e rstLength of output: 440
Script:
#!/bin/bash # Check contents of README and other docs for any API references cat README.md cat CONTRIBUTING.md # Look for API specs or documentation in common locations fd . -t f -e yaml -e json | grep -i "api\|swagger\|openapi" # Search for any test files that might reference the API endpoint fd -e py -e json | xargs rg "rocketpy"Length of output: 22322
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (15)
.github/workflows/pylint.yml(1 hunks)Makefile(2 hunks)README.md(1 hunks)lib/controllers/flight.py(1 hunks)lib/controllers/rocket.py(1 hunks)lib/models/aerosurfaces.py(1 hunks)lib/models/rocket.py(2 hunks)lib/routes/flight.py(2 hunks)lib/routes/rocket.py(1 hunks)pyproject.toml(1 hunks)tests/test_routes/conftest.py(1 hunks)tests/test_routes/test_environments_route.py(0 hunks)tests/test_routes/test_flights_route.py(1 hunks)tests/test_routes/test_motors_route.py(0 hunks)tests/test_routes/test_rockets_route.py(1 hunks)
💤 Files with no reviewable changes (2)
- tests/test_routes/test_environments_route.py
- tests/test_routes/test_motors_route.py
✅ Files skipped from review due to trivial changes (1)
- .github/workflows/pylint.yml
🧰 Additional context used
📓 Learnings (1)
lib/routes/rocket.py (1)
Learnt from: GabrielBarberini
PR: RocketPy-Team/Infinity-API#38
File: lib/routes/motor.py:75-75
Timestamp: 2024-11-15T15:12:21.314Z
Learning: When modifying API route paths in `lib/routes/motor.py`, changing from `/rocketpy/{motor_id}` to `/{motor_id}/rocketpy` is acceptable when no external consumers are affected. It's acceptable to introduce this breaking change if the team has decided to adopt the new approach, provided that the `README` and related documentation are updated accordingly.
🔇 Additional comments (10)
pyproject.toml (1)
Line range hint 2-3: Verify semantic versioning compliance
The version bump to 2.3.0 suggests a minor feature addition. Given this PR is primarily about test refactoring, please verify if this version increment aligns with semantic versioning principles.
Makefile (1)
1-2: LGTM! Good separation of formatting and linting tasks.
The separation of format and lint targets is a good practice as it clearly distinguishes between:
- Formatting tools (
black,ruff) that automatically fix code style - Linting tools (
flake8,pylint) that check for potential errors
tests/test_routes/test_rockets_route.py (2)
174-175: Avoid modifying shared fixtures in place to prevent side effects
In test_create_generic_motor_rocket, you update the stub_rocket fixture in place:
stub_rocket.update({'motor': stub_motor})Modifying the shared stub_rocket fixture can affect other tests that rely on it. To prevent unintended side effects, make a copy of the fixture before updating it.
198-199: Avoid modifying shared fixtures in place to prevent side effects
Similar to previous comments, in test_create_liquid_motor_level_tank_rocket, you're modifying stub_motor and stub_rocket fixtures directly:
stub_motor.update({'tanks': [stub_level_tank]})
stub_rocket.update({'motor': stub_motor})Create copies before making these updates to avoid side effects in other tests.
lib/models/aerosurfaces.py (1)
6-12: Parachute model is well-defined with proper validations
The Parachute class is correctly defined using Pydantic BaseModel, with appropriate use of Field to enforce validations on cd_s, sampling_rate, and lag. The use of type hints and constraints ensures data integrity.
lib/routes/rocket.py (1)
76-76: LGTM: Parameter ordering aligns with controller interface
The updated parameter order in the controller call matches the controller's interface, maintaining consistency across the codebase.
lib/routes/flight.py (2)
Line range hint 62-89: LGTM: Consistent route path structure
The route path change maintains consistency with the rocket routes and follows the same RESTful conventions.
149-149: LGTM: Consistent parameter ordering
The parameter ordering in the controller call maintains consistency with the rocket routes and matches the controller's interface.
lib/controllers/rocket.py (1)
173-173: Parameter reordering improves API consistency
The parameter reordering in update_rocket_by_id makes the method signature more consistent with REST API conventions and other methods in the class where ID is the first parameter.
Let's verify all callers of this method have been updated:
✅ Verification successful
Let me analyze the method signatures more carefully by getting the full context of these calls.
Parameter reordering is correctly implemented across the codebase
The parameter reordering in update_rocket_by_id has been properly handled across all its usages:
- In
lib/routes/rocket.py, the method is called with correct parameter order:RocketController.update_rocket_by_id(rocket_id, rocket) - In
lib/repositories/rocket.py, the repository method has a different signature as it's not the same method, just similar name - In
lib/controllers/rocket.py, the internal call to repository's method is correct as it only passesrocket_id
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for calls to update_rocket_by_id to ensure they match the new signature
rg -A 2 "update_rocket_by_id\(" --type py
Length of output: 852
Script:
#!/bin/bash
# Get more context around the update_rocket_by_id calls
rg -B 5 -A 10 "update_rocket_by_id\(" --type py
Length of output: 3658
lib/controllers/flight.py (1)
186-186: Parameter reordering aligns with class conventions
The parameter reordering in update_flight_by_id improves consistency by:
- Matching the pattern of other update methods in the class (
update_env_by_flight_id,update_rocket_by_flight_id) - Following the same convention as
RocketController
Let's verify all callers of this method have been updated:
✅ Verification successful
Let me verify the implementation details of the repository method to ensure compatibility.
Let me try a different approach to check the repository implementation.
Let me check the route implementation to verify the parameter order in the controller call.
Parameter reordering is correctly implemented across the codebase
The verification shows that:
- The controller's
update_flight_by_idmethod has been updated to accept parameters as(cls, flight_id: str, flight: Flight). - The route handler in
lib/routes/flight.pycorrectly calls the controller with the new parameter order:FlightController.update_flight_by_id(flight_id, flight). - The repository's
update_flight_by_idmethod has a compatible signature that acceptsflight_idas its parameter, with the flight data being accessed through the instance property.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for calls to update_flight_by_id to ensure they match the new signature
rg -A 2 "update_flight_by_id\(" --type py
Length of output: 852
Script:
#!/bin/bash
# Check the implementation of update_flight_by_id in the repository
ast-grep --pattern 'async def update_flight_by_id($$$)'
Length of output: 57
Script:
#!/bin/bash
# Get the content of the repository file to check the method signature
cat lib/repositories/flight.py
Length of output: 6605
Script:
#!/bin/bash
# Get the content of the routes file to check how the controller is called
cat lib/routes/flight.py
Length of output: 4256
GabrielBarberini
force-pushed
the
refactor_tests
branch
from
89d0c3c to
1e58b4a
Compare
lib/models/rocket.py
Outdated
| def __eq__(self, other): | ||
| if not isinstance(other, Rocket): | ||
| return False | ||
| return self.dict() == other.dict() |
There was a problem hiding this comment.
Just to clear some things up for me: Why the override of the equality operator? In which situations will this equality check be used?
There was a problem hiding this comment.
Truth be said this is actually a hack to make this pass:
mock_update_flight.assert_called_once_with(
'123', rocket=Rocket(**stub_rocket)
)
But definitely should be refactored.
I'll try to think in a non-verbose yet functional way to overcome that, thanks for bringing this.
There was a problem hiding this comment.
patched here 8a8e680
Implements flight route tests
Summary by CodeRabbit