Skip to content
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

TST: new set of tests to the Flight class #408

Merged
merged 12 commits into from
Nov 3, 2023

Conversation

Gui-FernandesBR
Copy link
Member

Pull request type

Please check the type of change your PR introduces:

  • Other (please describe): Unit tests

Pull request checklist

Please check if your PR fulfills the following requirements, depending on the type of PR:

  • Code base additions (for bug fixes / features):

    • Tests for the changes have been added
    • Docs have been reviewed and added / updated if needed
    • Lint (black rocketpy) has passed locally and any fixes were made
    • All tests (pytest --runslow) have passed locally

What is the current behavior?

  • The current tests covering the Flight class barely tests the results of the class. Instead, it only check if the class is running without errors. This is a kind of poor testing as it does not give us enough faith on the results if we modify the Flight Class.
  • What I propose is for us to set tests checking numeric outputs from the class, thus ensuring that we will always have a way to copare the new flight simulations against the ones

What is the new behavior?

  • (1) For some specific methods, now we have tests checking if the class is returning the expected values, with those being the results extracted from the simulation as it is today.
  • (2) On other hand, some tests are checking if the class is returning the correct value compared against the exact expected value (calculated by hand).
  • I documented every test so we can easily differentiate (1) and (2)
  • I separated the tests by logic, this way I believe is easier for us to debug the code in the future. Specifically, you're gonna see some very similar methods at the bottom of the test_flight.py file. ButI throught it would be worse to have everything being test under the same function.
  • As a painpoint of making more narrow tests like this, we probably will need to change te expected values of several method if we find a serious bug in the code. (but we don't think there's space for such error in the current state of the code, right?)

Does this introduce a breaking change?

  • No

Other information

  • Please review the BUG: absolute value of the rail button forces #407 first, and then we can change the head branch to beta/v1.0.0
  • Being 100% transparent here: I created all these tests because I want to change the way we calculate R1, R2, R3, M1, M2 and M3 arguments, and therefore I'd love to have better tests to support my changes in a future PR. The Flight class needs an overhaul, so I think more robust tests are coming one way or another

@Gui-FernandesBR Gui-FernandesBR added the Tests Regarding Tests label Sep 9, 2023
@Gui-FernandesBR Gui-FernandesBR added this to the Release v1.1.0 milestone Sep 9, 2023
@Gui-FernandesBR Gui-FernandesBR self-assigned this Sep 9, 2023
@Gui-FernandesBR Gui-FernandesBR marked this pull request as draft September 9, 2023 02:47
@Gui-FernandesBR
Copy link
Member Author

And of course it didn't work out of windows system (great!!)

I'm converting this to draft, temporarily

@Gui-FernandesBR Gui-FernandesBR marked this pull request as ready for review September 9, 2023 04:31
tests/test_flight.py Show resolved Hide resolved
tests/test_flight.py Outdated Show resolved Hide resolved
tests/test_flight.py Outdated Show resolved Hide resolved
tests/test_flight.py Show resolved Hide resolved
tests/test_flight.py Outdated Show resolved Hide resolved
tests/conftest.py Outdated Show resolved Hide resolved
Base automatically changed from bug/max-button-forces-values to beta/v1.0.0 September 15, 2023 23:38
@Gui-FernandesBR Gui-FernandesBR changed the base branch from beta/v1.0.0 to develop October 9, 2023 04:38
Copy link
Member Author

@Gui-FernandesBR Gui-FernandesBR left a comment

Choose a reason for hiding this comment

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

Merging the develop branch into this one actually solved the problem that was blocking this PR.
So I believe we are good to go!

I can't merge it bc we need an approval first. Asking for help here.

Copy link
Collaborator

@phmbressan phmbressan left a comment

Choose a reason for hiding this comment

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

Good. I have updated some old docstrings in my latest commits.

Before merging, I would only double check if the numbers of the new tests are still valid (the ones that come from sim results) since there was a gap of time between this PR being implmented and now.

@Gui-FernandesBR Gui-FernandesBR merged commit d36947c into develop Nov 3, 2023
9 checks passed
@Gui-FernandesBR Gui-FernandesBR deleted the tst/flight-class-protection branch November 3, 2023 21:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Tests Regarding Tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants