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: increase test coverage #756

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

Gui-FernandesBR
Copy link
Member

@Gui-FernandesBR Gui-FernandesBR commented Dec 15, 2024

Pull request type

  • Code maintenance (refactoring, formatting, tests)

Checklist

  • Tests for the changes have been added (if needed)
  • 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

  • Coverage without slow tests: 76%

New behavior

  • Coverage without slow tests: 80%
  • Coverage with slow tests: 92%

Breaking change

  • No

Additional information

  • I'm not sure if I will have time to add more tests, so I guess the PR is ready for review
  • Please review carefully the logics behind the tests.

@Gui-FernandesBR Gui-FernandesBR added the Tests Regarding Tests label Dec 15, 2024
@Gui-FernandesBR Gui-FernandesBR self-assigned this Dec 15, 2024
@Gui-FernandesBR Gui-FernandesBR requested a review from a team as a code owner December 15, 2024 06:03
Copy link

codecov bot commented Dec 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.32%. Comparing base (83aa20e) to head (88e6025).
Report is 9 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #756      +/-   ##
===========================================
+ Coverage    76.42%   80.32%   +3.90%     
===========================================
  Files           95       95              
  Lines        11090    10968     -122     
===========================================
+ Hits          8475     8810     +335     
+ Misses        2615     2158     -457     

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

@Gui-FernandesBR
Copy link
Member Author

These are the top 10 files we should be worried about and try to increase coverage:

image

@Lucas-Prates
Copy link
Contributor

@Gui-FernandesBR if you do not mind, I can take a closer look at sensitivity_model and sensitivity_prints. I recall that you implemented tests back in the PR, but they were failing for some reason.

@Gui-FernandesBR
Copy link
Member Author

@Gui-FernandesBR if you do not mind, I can take a closer look at sensitivity_model and sensitivity_prints. I recall that you implemented tests back in the PR, but they were failing for some reason.

Yes please, I'd like some help on that

@Lucas-Prates
Copy link
Contributor

Lucas-Prates commented Dec 16, 2024

100% coverage for both sensitivity_model and sensitivity_prints. Also removed a piece of duplicate code inside sensitivity_model.

@Gui-FernandesBR Gui-FernandesBR force-pushed the tst/increase-test-coverage branch from da7c35d to 6863d6e Compare December 16, 2024 02:53
(0, 20, 200, 2, 1, True, True),
],
)
def test_short_time_fft(
Copy link
Member Author

Choose a reason for hiding this comment

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

@Lucas-Prates could yo review this test please? test_short_time_fft

Copy link
Contributor

@Lucas-Prates Lucas-Prates Dec 16, 2024

Choose a reason for hiding this comment

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

Looks okay to me! If you want to include a "numeric assertion" test, since you are using pure signal (i.e. sine function), all the result functions should be a Dirac delta function, i.e. a pulse function, at the given frequency, 5 Hz in this case. Something like this

image

So, if you want to, you could include an extra argument true_frequency, and then

signal = np.sin(2 * np.pi * true_frequency * t)
.
.
.
for f in stft_results:
    assert pytest.approx(1, 0.01) == f[true_frequency - 1][1]

@Gui-FernandesBR Gui-FernandesBR added this to the Release v1.X.0 milestone Dec 16, 2024
@Gui-FernandesBR Gui-FernandesBR linked an issue Dec 16, 2024 that may be closed by this pull request
@Gui-FernandesBR
Copy link
Member Author

Ok, I think this PR is already in a good point.
Of course that we can do even better. Ideally we should see 90% of code coverage without slow tests, and up to 10% of slowed tests.
But for today we are fine already.

@RocketPy-Team/code-owners ready for review! Pleas notice that the code coverage has increased 4 pp, but I didn't spend much time trying to find the best validation possible for each function method. I opted to be simple.

@Gui-FernandesBR
Copy link
Member Author

Once again I am sharing the top 10 worst files in terms of coverage.
Perhaps you could contribute a little to the function.py file, @phmbressan ?

image

MNT: linters

TST: complementing tests for sensitivity analysis and removing duplicate piece of code.

DEV: add pragma comments to exclude specific lines from coverage

MNT: fix pylint error
@Gui-FernandesBR Gui-FernandesBR force-pushed the tst/increase-test-coverage branch from 88e6025 to 9bd4383 Compare December 22, 2024 22:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Tests Regarding Tests
Projects
Status: Backlog
Development

Successfully merging this pull request may close these issues.

TST: Increase code coverage to 90%
2 participants