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: Environment unit tests refactoring #571

Merged
merged 4 commits into from
Mar 9, 2024
Merged

Conversation

GabrielBarberini
Copy link
Collaborator

@GabrielBarberini GabrielBarberini commented Mar 7, 2024

Pull request type

  • Code maintenance (refactoring, formatting, tests)

Checklist

  • Docs have been reviewed and added / updated
  • Lint (black rocketpy/ tests/) has passed locally
  • All tests (pytest tests -m slow --runslow) have passed locally --> All tests that were previously passing continued to pass

Breaking change

  • No

Additional information

@GabrielBarberini GabrielBarberini requested a review from a team as a code owner March 7, 2024 21:27
@GabrielBarberini GabrielBarberini changed the base branch from master to develop March 7, 2024 21:28
@Gui-FernandesBR Gui-FernandesBR added the Tests Regarding Tests label Mar 7, 2024
@Gui-FernandesBR Gui-FernandesBR added this to the Release v1.X.0 milestone Mar 7, 2024
@Gui-FernandesBR
Copy link
Member

Good PR.

Tests not passing? That catches my attention. Investigating it right now.

image

@GabrielBarberini
Copy link
Collaborator Author

Good PR.

Tests not passing? That catches my attention. Investigating it right now.

image

I summon: works on my machine 😂

tests/unit/test_environment.py .................. [100%]

Let me know if you find any issues

@Gui-FernandesBR
Copy link
Member

Good PR.
Tests not passing? That catches my attention. Investigating it right now.
image

I summon: works on my machine 😂

tests/unit/test_environment.py .................. [100%]

Let me know if you find any issues

Fair enough lol,

Are you using MacOS? Which python version?

@GabrielBarberini
Copy link
Collaborator Author

GabrielBarberini commented Mar 7, 2024

Good PR.
Tests not passing? That catches my attention. Investigating it right now.
image

I summon: works on my machine 😂
tests/unit/test_environment.py .................. [100%]
Let me know if you find any issues

Fair enough lol,

Are you using MacOS? Which python version?

MacOS Sonoma 14.2 Beta (23C5041e) under Python 3.11.5

@Gui-FernandesBR
Copy link
Member

@GabrielBarberini a problem is that you renamed the variable example_env_robust but a lot of tests used it.
Try to run all the tests at once, it doesn't take that long to be executed. (I usually run make pytest, see the Makefile in the root directory).

Try replacing example_env_robust to example_spaceport_env.

More replacements will be necessary I think.

@GabrielBarberini
Copy link
Collaborator Author

GabrielBarberini commented Mar 7, 2024

@GabrielBarberini a problem is that you renamed the variable example_env_robust but a lot of tests used it. Try to run all the tests at once, it doesn't take that long to be executed. (I usually run make pytest, see the Makefile in the root directory).

Try replacing example_env_robust to example_spaceport_env.

More replacements will be necessary I think.

Hmmm! You are right. Didn't notice this dependency.

Fixed :)

Meanwhile I'll run all the others just in case

@GabrielBarberini GabrielBarberini force-pushed the tests_refactoring branch 2 times, most recently from cf85070 to 13629ed Compare March 7, 2024 22:15
@GabrielBarberini
Copy link
Collaborator Author

GabrielBarberini commented Mar 7, 2024

All good now @Gui-FernandesBR

Sorry for that, for some reason I assumed the env fixtures were used only by the env tests.

Lastly, I tried to use a fixture teardown for env fixtures, but since other modules use env fixtures differently, it ended up conflicting with my change. So I had to set a request.addfinalizer() inside the unit env test that requires teardown (for deleting the environment.json file).

For review convenience, I added it as a separate commit.

Edit: I'll have to find another way to tear this down as the way I attempted is not passing internal pytest validations, maybe I'll just rollback to the way it was, one sec.

@GabrielBarberini
Copy link
Collaborator Author

Now all good.

Copy link

codecov bot commented Mar 7, 2024

Codecov Report

Attention: Patch coverage is 55.55556% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 72.62%. Comparing base (fda6b33) to head (f4f658c).
Report is 18 commits behind head on develop.

Files Patch % Lines
rocketpy/environment/environment.py 55.55% 4 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #571      +/-   ##
===========================================
- Coverage    72.62%   72.62%   -0.01%     
===========================================
  Files           59       59              
  Lines         9598     9607       +9     
===========================================
+ Hits          6971     6977       +6     
- Misses        2627     2630       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

TST: refactors tests/unit/test_environment
TST: refactors environment_fixtures
DOC: fix doc typo in decimal_degrees_to_arc_seconds
TST: fixes example_spaceport_env legacy references
TST: fixes example_plain_env legacy references
TST: Rollback teardown logic in unit env test
Copy link
Member

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

Easy to review a PR when it is well constructed and fixes one problem at time.

My comments are only related to small convention details.
Code works, PR is clear, that's what really matters.

tests/fixtures/environment/environment_fixtures.py Outdated Show resolved Hide resolved
tests/test_plots.py Show resolved Hide resolved
tests/unit/test_environment.py Show resolved Hide resolved
tests/unit/test_environment.py Show resolved Hide resolved
tests/unit/test_environment.py Outdated Show resolved Hide resolved
tests/unit/test_environment.py Outdated Show resolved Hide resolved
tests/unit/test_environment.py Outdated Show resolved Hide resolved
@Gui-FernandesBR Gui-FernandesBR changed the title Environment unit tests refactoring TST: Environment unit tests refactoring Mar 8, 2024
@GabrielBarberini
Copy link
Collaborator Author

Easy to review a PR when it is well constructed and fixes one problem at time.

My comments are only related to small convention details. Code works, PR is clear, that's what really matters.

Thanks for the feedback! I'll address your requests soon

@Gui-FernandesBR
Copy link
Member

This PR contributes to #480

@Gui-FernandesBR
Copy link
Member

Thanks again for the contribution, @GabrielBarberini ,
I commented on the conversations, you can mark them as resolved once you feel they are done.

I believe we are close to approve/merge

Copy link
Member

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

LGTM

@GabrielBarberini GabrielBarberini merged commit 6096bfd into develop Mar 9, 2024
9 of 11 checks passed
@GabrielBarberini GabrielBarberini deleted the tests_refactoring branch March 9, 2024 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Tests Regarding Tests
Projects
Status: Closed
Development

Successfully merging this pull request may close these issues.

2 participants