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

Improve the coverage of the tests and fix some minor bugs #64

Merged
merged 28 commits into from
Jan 21, 2025

Conversation

lionelkusch
Copy link
Collaborator

I added in this pull request some simple tests (raise exceptions, used some options ...)

The aim of this pull request is to increase the coverage before starting to make a lot of modifications to the library.
The new tests are simple due to my lack of knowledge of the details of the different methods.

I don't take the time to format and comment on the additional tests; I will do it when I will reformat each method.

@lionelkusch
Copy link
Collaborator Author

Tis pull request is complementary to the pull request on the estimators #58 and #59 for having coverage close to 99%.
There still have a few lines of the code, which have not been tested.
The most important part will be in hidimstat/stat_coef_diff.py because there weren't any tests associated with it. If you have a suggestion, please share it with us.

Copy link

codecov bot commented Dec 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.23%. Comparing base (19e6cf7) to head (85a6026).
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #64      +/-   ##
==========================================
+ Coverage   81.70%   84.23%   +2.53%     
==========================================
  Files          43       43              
  Lines        2312     2449     +137     
==========================================
+ Hits         1889     2063     +174     
+ Misses        423      386      -37     

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

Copy link
Contributor

@bthirion bthirion left a comment

Choose a reason for hiding this comment

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

Thx for taking care of that, this is super important.
Please take care of the syntax aspects.
How much does test time increase with this PR ? We need to be careful to keep it limited.

@lionelkusch
Copy link
Collaborator Author

How much does test time increase with this PR ? We need to be careful to keep it limited.

The time for the tests is still less than 5 min, as you can see in the report below.
The longest tests will be for the estimator (#57), going until 8 min for all tests. I suppose that is still good and the tests for the estimators can be run only when the estimators files are modified.

@bthirion
Copy link
Contributor

8min is still fine, but we must be super careful here, because these numbers increase steadily.
The best way to do it is to identify which tests take the largest amount of time and see how to lower it.
But this is obviously not a priority for now.

Copy link
Contributor

@bthirion bthirion left a comment

Choose a reason for hiding this comment

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

LGTM otherwise. Thx.

@lionelkusch
Copy link
Collaborator Author

@jpaillard

Copy link
Contributor

@bthirion bthirion left a comment

Choose a reason for hiding this comment

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

The test part looks OK to me.

lionelkusch and others added 2 commits January 7, 2025 15:05
improvement

Co-authored-by: bthirion <[email protected]>
Co-authored-by: Joseph Paillard <[email protected]>
Copy link
Collaborator

@jpaillard jpaillard left a comment

Choose a reason for hiding this comment

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

All good on my side

@lionelkusch
Copy link
Collaborator Author

I finally decided to remove the option for groups and I created the group by hand.

Copy link
Contributor

@bthirion bthirion left a comment

Choose a reason for hiding this comment

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

LGTM overall. One more line needs coverage.

@lionelkusch
Copy link
Collaborator Author

I removed the line not covered because it will be modified in the PR #132.

Copy link
Contributor

@bthirion bthirion left a comment

Choose a reason for hiding this comment

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

LGTM, thx.

@lionelkusch lionelkusch merged commit eccd3d8 into mind-inria:main Jan 21, 2025
8 checks passed
@lionelkusch lionelkusch deleted the PR_test_increase branch January 21, 2025 08:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants