Skip to content

Conversation

@Bizo883
Copy link

@Bizo883 Bizo883 commented Nov 5, 2025

Pull request type

  • Code changes (bugfix, features)
  • Code maintenance (refactoring, formatting, tests)
  • ReadMe, Docs and GitHub updates

Checklist

  • Tests for the changes have been added (if needed)
  • Docs have been reviewed and added / updated
  • Lint (black rocketpy/ tests/) has passed locally
  • All tests (pytest tests -m slow --runslow) have passed locally
  • CHANGELOG.md has been updated (if relevant)

Current behavior

There is no test for the _check_missing_components().

New behavior

This PR adds unit tests to verify the warning behavior of the _check_missing_components() method.
Specifically, it includes three test cases:

  1. All components missing → expects a single warning listing all missing elements.
  2. Some components missing → ensures only the absent components appear in the warning message.
  3. No components missing → ensures no warning is issued.

These tests are defined in tests/unit/test_rocket.py.

Breaking change

  • Yes
  • No

C8H10O2 and others added 3 commits November 4, 2025 16:13
Added a placeholder in the [Unreleased] section for the upcoming feature
to add custom warnings when a rocket has no motors, parachute, or AeroSurface. See RocketPy-Team#285
This enhancement adds a warning when a Rocket object has no motors,
parachutes, or AeroSurface components. It notifies the user so that
they can add missing components before running simulations. See RocketPy-Team#285
@Bizo883 Bizo883 requested a review from a team as a code owner November 5, 2025 15:22
"""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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are you sure?

    # If any warning occurs, pytest will fail automatically

Copy link
Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changelog seems weird @phmbressan

Copy link

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.

Copy link
Member

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changelog seems weird @phmbressan

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?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changelog seems weird @phmbressan

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?

Understood, I’ll take care of it. Thanks for pointing it out.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a rocket without parachute is something quite common.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following up on issue #285 and your comment:

#285 (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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should never raise an warning for "parachuteless rockets", so to say

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Backlog

Development

Successfully merging this pull request may close these issues.

4 participants