Add testing cases for new motor classes: LiquidMotor and HybridMotor

[phmbressan]

Pull request type

Please check the type of change your PR introduces:

• Code base additions (bugfix, features)
• Code maintenance (refactoring, formatting, renaming, tests)
• ReadMe, Docs and GitHub maintenance
• Other (please describe):

Pull request checklist

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

• Code base maintenance (refactoring, formatting, renaming):

  • 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 code was lacking proper testing for LiquidMotor and HybridMotor classes.

What is the new behavior?

All the implemented tests are passing and they are checks for basic properties of Tank, LiquidMotor and HybridMotor classes, such as masses, volumes, center of masses and inertias.

Does this introduce a breaking change?

• Yes
• No

[Gui-FernandesBR]

Good job here, we were really missing these tests since our last release. I'm approving your code, however, I'd like to invite you to think about some points:

1. You are testing the construction of Hybrid and Liquid classes. But you're not testing the Flight class behavior when using these motors. Should we add a test where the rocket flies with a non-solid motor?

2. In the test_hybrid_motor_info method (same for liquid) you tested the all_info(). Mty suggestion is to also test the info() method, in the ame test. This way we would gain a few lines in our code coverage

3. Speaking of code coverage, I've ran a report here, and the overall coverage of motors files was higher than 90%. Amazing job here!
   

4. I imagine you would agree if i say that the test_tank.py file is the hardest one to understand (and therefore to maintain) of all the test files. I loved how you created the auxiliary functions by the end of the file, however, I still think the code structure is a bit far from the other files. For example, the test_tank_bounds method is testing the same thing for 3 different types of tanks. Did you consider parametrizing (@parametrize decorator) this kind of test so we could easily know what is the tank raising error in case of bug?

All in all the code is great and it does solve the problem, it deserves to be approved and merge. I will be happy if you find my comments helpful somehow.

Thanks,
Guilherme

[phmbressan]

Good job here, we were really missing these tests since our last release. I'm approving your code, however, I'd like to invite you to think about some points:

Thank you for your review. Regarding your questions/suggestions:

1. I implemented very brief tests that simply add the motors to a rocket and verify if an simulation can be completed. This must be expanded with more test cases and real data validation if the future;
2. Good suggestion, done;
3. Glad to hear that!
4. I absolutely agree that the test_tank file is the hardest to grasp so, in the most recent commits, I have done some refactoring:
   • Yes, I have tried the first time to parametrize the tests, however I had some trouble manipulating the fixtures. Thanks to your comment, I excavated a bit further and found a way to make it work, which simplified it quite a bit. Let me know what you think of the current version.
   • I reordered some testing cases, so this might make the code diff a bit cumbersome;
   • Nonetheless, it is still a complex file that can be improved, specially the huge testing cases for different tank types when comparing to real data, such as test_mfr_tank_basic.

[Gui-FernandesBR]

Good job here, we were really missing these tests since our last release. I'm approving your code, however, I'd like to invite you to think about some points:

Thank you for your review. Regarding your questions/suggestions:

1. I implemented very brief tests that simply add the motors to a rocket and verify if an simulation can be completed. This must be expanded with more test cases and real data validation if the future;

2. Good suggestion, done;

3. Glad to hear that!

4. I absolutely agree that the test_tank file is the hardest to grasp so, in the most recent commits, I have done some refactoring:

   • Yes, I have tried the first time to parametrize the tests, however I had some trouble manipulating the fixtures. Thanks to your comment, I excavated a bit further and found a way to make it work, which simplified it quite a bit. Let me know what you think of the current version.
   • I reordered some testing cases, so this might make the code diff a bit cumbersome;
   • Nonetheless, it is still a complex file that can be improved, specially the huge testing cases for different tank types when comparing to real data, such as test_mfr_tank_basic.

Very well done!! The implementations made the functions way more readable now, this was a significant improvement.
I confirm that the code works in my computer.

In the future, having different ways to validade these classes will be important, especially through experimental data comparison.

[Gui-FernandesBR]

Good improvements, @phmbressan , well done!
Tests are passing, I don't think there's much more to do in this PR.

Please feel free to merge whenever you want ;)