-
-
Notifications
You must be signed in to change notification settings - Fork 208
Enh/custom warning no motor parachute aerosurface #871
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from 3 commits
37dbbf8
c0f17a5
db5aa9a
1af1e40
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. a rocket without parachute is something quite common. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Following up on issue #285 and your comment: I wanted to clarify: for rockets without parachutes, should the custom warning be changed to a notice/info message instead, or should it only trigger under specific conditions?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we should never raise an warning for "parachuteless rockets", so to say |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,7 +10,9 @@ | |
|
|
||
|
|
||
| @patch("matplotlib.pyplot.show") | ||
| def test_elliptical_fins(mock_show, calisto_robust, calisto_trapezoidal_fins): # pylint: disable=unused-argument | ||
| def test_elliptical_fins( | ||
| mock_show, calisto_robust, calisto_trapezoidal_fins | ||
| ): # pylint: disable=unused-argument | ||
| test_rocket = calisto_robust | ||
| calisto_robust.aerodynamic_surfaces.remove(calisto_trapezoidal_fins) | ||
| test_rocket.add_elliptical_fins(4, span=0.100, root_chord=0.120, position=-1.168) | ||
|
|
@@ -370,6 +372,40 @@ def test_add_motor(calisto_motorless, cesaroni_m1670): | |
| assert center_of_mass_motorless is not center_of_mass_with_motor | ||
|
|
||
|
|
||
| def test_check_missing_all_components(calisto_motorless): | ||
| """Tests the _check_missing_components method for a Rocket with no components.""" | ||
| with pytest.warns(UserWarning) as record: | ||
| calisto_motorless._check_missing_components() | ||
|
|
||
| assert len(record) == 1 | ||
| msg = str(record[0].message) | ||
| assert "motor" in msg | ||
| assert "parachutes" in msg | ||
| assert "aerodynamic surfaces" in msg | ||
|
|
||
|
|
||
| def test_check_missing_some_components(calisto): | ||
| """Tests the _check_missing_components method for a Rocket missing some components.""" | ||
| calisto.parachutes = [] | ||
| calisto.aerodynamic_surfaces = [] | ||
|
|
||
| with pytest.warns(UserWarning) as record: | ||
| calisto._check_missing_components() | ||
|
|
||
| assert len(record) == 1 | ||
| msg = str(record[0].message) | ||
| assert "parachutes" in msg | ||
| assert "aerodynamic surfaces" in msg | ||
|
|
||
|
|
||
| def test_check_missing_no_components_missing(calisto_robust): | ||
| """Tests the _check_missing_components method for a complete Rocket.""" | ||
| # Call directly — no warnings expected | ||
| calisto_robust._check_missing_components() | ||
| # If any warning occurs, pytest will fail automatically | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. are you sure?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you for the review!You're correct.By default, pytest only displays warnings but doesn't fail.I will updated the test. |
||
| assert True | ||
|
|
||
|
|
||
| def test_set_rail_button(calisto): | ||
| rail_buttons = calisto.set_rail_buttons(0.2, -0.5, 30) | ||
| # assert buttons_distance | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changelog seems weird @phmbressan
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your feedback. Could you please clarify which part of the changelog seems weird? I want to make sure my addition follows the proper format.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
your added line is listed along together other changes that have already been deployed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can double-check later, but this branch might not be up to date with develop (v1.11), which is why the CHANGELOG seems outdated. Could you @C8H10O2 try rebasing this branch with the current develop so that all conflicts are sorted out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood, I’ll take care of it. Thanks for pointing it out.