Skip to content

Improve coverage in multiprocessing cases #1053

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

Merged
merged 13 commits into from
Nov 26, 2024

Conversation

EmmaRenauld
Copy link
Contributor

@EmmaRenauld EmmaRenauld commented Nov 20, 2024

Our coverage with codecov does not read lines that are called through multiprocessing (Pool).

I separated the case nbr_processes=1 :

if nbr_processes == 1:
    call_it_ourselves()
else:
    as_before

All impacted scripts tested locally with both --processes 1 and --processes 4 (I changed manually the values in each script test).

Copy link

codecov bot commented Nov 20, 2024

Codecov Report

Attention: Patch coverage is 44.04762% with 94 lines in your changes missing coverage. Please review.

Project coverage is 69.30%. Comparing base (e7337af) to head (06122c1).
Report is 159 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1053      +/-   ##
==========================================
+ Coverage   68.93%   69.30%   +0.36%     
==========================================
  Files         438      445       +7     
  Lines       22889    23891    +1002     
  Branches     3106     3262     +156     
==========================================
+ Hits        15779    16557     +778     
- Misses       5793     5958     +165     
- Partials     1317     1376      +59     
Components Coverage Δ
Scripts 69.96% <ø> (+0.12%) ⬆️
Library 68.40% <44.04%> (+0.74%) ⬆️

@pep8speaks
Copy link

pep8speaks commented Nov 20, 2024

Hello @EmmaRenauld, Thank you for updating !

Line 344:80: E501 line too long (81 > 79 characters)

Comment last updated at 2024-11-21 17:08:12 UTC

@EmmaRenauld EmmaRenauld changed the title [WIP] Improve coverage in multiprocessing cases Improve coverage in multiprocessing cases Nov 21, 2024
@EmmaRenauld
Copy link
Contributor Author

Not changed: everything in scilpy.tracking.Tracker (too complicated).

I used the keyword search "Pool" to find these methods. There could be others.

@EmmaRenauld
Copy link
Contributor Author

EmmaRenauld commented Nov 21, 2024

That gave us a 0.36% upgrade. I was hoping for more, but still.

File by file:
scilpy.reconst.divide: 29% to 69%
scilpy.reconst.fodf: 66% to 71%
scilpy.segment.voting_scheme: 77% to 81%
scilpy.tractanalysis.fixel_density: 52% to 94%

@arnaudbore arnaudbore self-requested a review November 22, 2024 21:57
Copy link
Contributor

@arnaudbore arnaudbore left a comment

Choose a reason for hiding this comment

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

LGTM !

Copy link
Contributor

@CHrlS98 CHrlS98 left a comment

Choose a reason for hiding this comment

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

I read the code and everything looks good to me. I approve!

It's sad though that we need to add cases to our code to increase our code coverage by 0.36%. Also for my information, is it only multiprocessing that codecov has a problem handling or is it function callbacks in general?

@EmmaRenauld
Copy link
Contributor Author

EmmaRenauld commented Nov 26, 2024

I'm not sure yet. These are a few methods that I found that were not covered when I was investigating which unit test I should do next. If I find others, I will try to manage them too.

It's only 0.36%, sure, but it still helps me a lot when I want to check if some functions are simply never used anywhere.

@arnaudbore arnaudbore merged commit 4d7f52c into scilus:master Nov 26, 2024
2 checks passed
@EmmaRenauld EmmaRenauld deleted the no_multiproc_cov branch November 26, 2024 15:09
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.

4 participants