Skip to content

Conversation

@AlexVCaron
Copy link
Collaborator

Quick description

The regex for the library component is not working, the resulting coverage component is just empty.
...

Type of change

Check the relevant options.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Provide data, screenshots, command line to test (if relevant)

We'll see on the coverage report that will be uploaded for this PR if it works or not

Checklist

  • My code follows the style guidelines of this project (run autopep8)
  • I added relevant citations to scripts, modules and functions docstrings and descriptions
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I moved all functions from the script file (except the argparser and main) to scilpy modules
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@codecov
Copy link

codecov bot commented Sep 30, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.45%. Comparing base (14cb12f) to head (0718118).
⚠️ Report is 69 commits behind head on master.

❌ Your project status has failed because the head coverage (72.45%) is below the target coverage (75.00%). You can increase the head coverage or adjust the target coverage.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1245      +/-   ##
==========================================
+ Coverage   71.96%   72.45%   +0.49%     
==========================================
  Files         293      293              
  Lines       25189    25185       -4     
  Branches     3528     3527       -1     
==========================================
+ Hits        18127    18249     +122     
+ Misses       5575     5444     -131     
- Partials     1487     1492       +5     
Flag Coverage Δ
smoketests 69.63% <ø> (?)
unittests 13.11% <ø> (-58.85%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
Scripts 75.28% <ø> (+<0.01%) ⬆️
Library 69.10% <ø> (+1.07%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@AlexVCaron
Copy link
Collaborator Author

Coverage split is working ! Go here and use the components dropdown to filter by them.

@AlexVCaron AlexVCaron force-pushed the fix/codecov_split branch 2 times, most recently from 9f14d87 to 8d3ebbc Compare October 1, 2025 02:34
@EmmaRenauld
Copy link
Contributor

EmmaRenauld commented Oct 1, 2025

It's nearly there!

When I select unittest, then I get a 0% coverage on src/cli, which overall decreases the percentage to 13%. It think that without this, we would see ~30%. Can we tell it to ignore the cli path for unittest ? Or is it too complicated?

image

@AlexVCaron
Copy link
Collaborator Author

I tried to have it only with the flags, not sure if it's possible though. I need to look further in the documentation of coverage, it's a bit of a pain.

But right now, you can get the percentages you want by using both flags and components to filter the coverage report. Here, I selected the unittest flag and the Library component :
image

@EmmaRenauld
Copy link
Contributor

EmmaRenauld commented Oct 1, 2025

Also I just noticed that the numbers for the unittest in other files are good, but the numbers for the smoke test are the same as the total coverage from both.

If it can help you: Here are the numbers that I noted a few months ago for:

  • dwi.operations: 79 with smoke, 72 with unit, 89 total
  • dwi.utils: 55 with smoke, 91 with unit, 91 total
  • reconst.bingham: 43 with smoke, 88 with unit, 94 total

- name: Run unit tests
run: |
uv run --active pytest src/scilpy --cov-report term-missing:skip-covered --dist=loadgroup --ignore=src/scilpy/cli
mv .test_reports unit_test_reports
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe the reason that smoke tests have the total coverage is that here you don't do mv .coverage .coverage.unit?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually, the coverage report is also written in the .test_reports folder, so we only need to upload its content to coverage and it's sufficient.

I'm looking into the reports right now to find why we get those new numbers

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Percentages seems good now ! I removed --cov-append in pytest.ini and it fixed it. It was using the default .coverage file that's created at the root of the repo by the unit tests run to seed the coverage report for smoke tests.

@EmmaRenauld
Copy link
Contributor

I tried to have it only with the flags, not sure if it's possible though. I need to look further in the documentation of coverage, it's a bit of a pain.

But right now, you can get the percentages you want by using both flags and components to filter the coverage report. Here, I selected the unittest flag and the Library component : image

Oh! That's what you told us yesterday, sorry!. That's ok for me, then!

@AlexVCaron
Copy link
Collaborator Author

I tried to have it only with the flags, not sure if it's possible though. I need to look further in the documentation of coverage, it's a bit of a pain.
But right now, you can get the percentages you want by using both flags and components to filter the coverage report. Here, I selected the unittest flag and the Library component : image

Oh! That's what you told us yesterday, sorry!. That's ok for me, then!

Great, because it seems to be the only way to do it, beside duplicating coverage and pytest configurations. I don't think we want that 😅

Copy link
Contributor

@EmmaRenauld EmmaRenauld left a comment

Choose a reason for hiding this comment

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

I'm really happy that this is done!

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.

GTG

@arnaudbore arnaudbore merged commit 9e21af8 into scilus:master Oct 9, 2025
2 checks passed
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