Skip to content

Add test coverage to git actions#71

Open
matschreiner wants to merge 11 commits intomllam:mainfrom
matschreiner:coverage
Open

Add test coverage to git actions#71
matschreiner wants to merge 11 commits intomllam:mainfrom
matschreiner:coverage

Conversation

@matschreiner
Copy link
Copy Markdown
Contributor

@matschreiner matschreiner commented Feb 14, 2025

This PR introduces test coverage enforcement and improves test reliability:

I have added pytest-cov to the install in the workflows file to ensure coverage measurement is available.
I set a 100% coverage requirement (--cov-fail-under=100) to enforce full test coverage for new code.
Applied # pragma: no cover to all pre-existing, untested code to avoid unnecessary coverage failures.
These changes help maintain strict coverage standards while allowing legacy code to remain untouched. 🚀

Type of change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 💥 Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • 📖 Documentation (Addition or improvements to documentation)

Checklist before requesting a review

  • My branch is up-to-date with the target branch - if not update your fork with the changes from the target branch (use pull with --rebase option if possible).
  • I have performed a self-review of my code
  • For any new/modified functions/classes I have added docstrings that clearly describe its purpose, expected inputs and returned values
  • I have placed in-line comments to clarify the intent of any hard-to-understand passages of my code
  • I have updated the documentation to cover introduced code changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have given the PR a name that clearly describes the change, written in imperative form (context).
  • I have requested a reviewer and an assignee (assignee is responsible for merging)

Checklist for reviewers

Each PR comes with its own improvements and flaws. The reviewer should check the following:

  • the code is readable
  • the code is well tested
  • the code is documented (including return types and parameters)
  • the code is easy to maintain

Author checklist after completed review

  • I have added a line to the CHANGELOG describing this change, in a section
    reflecting type of change (add section where missing):
    • added: when you have added new functionality
    • changed: when default behaviour of the code has been changed
    • fixes: when your contribution fixes a bug

Checklist for assignee

  • PR is up to date with the base branch
  • the tests pass
  • author has added an entry to the changelog (and designated the change as added, changed or fixed)
  • Once the PR is ready to be merged, squash commits and merge the PR.

@matschreiner matschreiner changed the title enforce full coverage Add test coverage to git actions Feb 14, 2025
@observingClouds
Copy link
Copy Markdown
Contributor

I know you didn't ask for a review from my side, but here is it anyway 😝
If the coverage is deactivated for the current/old functions, does that mean, whenever I would now touch a new function, I have to implement the ENTIRE test suite for that function? I wouldn't want that. I would suggest to either implement all test with this PR or set the coverage level to what we currently have, so that it does not drop below that.

Also, I am curious to hear your experiences with this as mine weren't so useful so far.

@matschreiner
Copy link
Copy Markdown
Contributor Author

@observingClouds
It means that if you add new code to the codebase, you will be responsible for making sure to write tests that touches all lines of that code. This is common practice in many places and a good first line of defense against technical debt in the codebase.

You will not be responsible for writing tests for code that is already ignored, even if you touch those functions by calling them from your own new code. You also don't have to write tests for everything, you can ignore lines from coverage. But then you have to consciously choose so. :)

Copy link
Copy Markdown
Contributor

@mfroelund mfroelund left a comment

Choose a reason for hiding this comment

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

Like it! Ambitious with 100% coverage. I'm ok with it, but maybe we should discuss it on a Tuesday meeting. I somewhat like the "# pragma" comments, since it makes it very clear what parts of the code that are uncovered, although it can seem a lot with all those comments at a first glance.

@leifdenby
Copy link
Copy Markdown
Member

leifdenby commented Feb 17, 2025

Thanks for setting this up @matschreiner! I agree with @mafdmi that we should maybe discuss what this will practical mean for development? I am not very familiar with coverage and I think it would be good to for whole team to understand this before we adopt a particular way of working. Let's discuss tomorrow :)

Things I would like to understand is:

  • how is "coverage" calculated? What does it mean technically?
  • what does "writing tests" mean in this context? Can I just write a test that calls a function for example, but doesn't look a the result? What do we expect of each others contributions in terms of testing?

@matschreiner
Copy link
Copy Markdown
Contributor Author

matschreiner commented Feb 17, 2025

@leifdenby @mafdmi
Sure, lets discuss it tomorrow :)

A line is covered if it has been executed while running the test suite. So in the following example the lines until the first return out will be covererd. This means that we don't know if we have some logic that might be failing.

def fun(i):
    if i > 0: 
        out = ...
        return out
    out = logic that maybe is failing ... 
    return out

def test_fun():
    out = fun(1) 
    ### PASSED ### 

Of course it is not a guarantee that our testing logic is solid, we have to test the edgecases ourselves.
Yes, you can write a test that doesn't test anything other than execute the code and it will be enough to get coverage.

To get full coverage of the above code we would write another test

def test_fun():
    out = fun(-1)
    ... 

I think it's quite the task for anyone to write tests for every single uncovered line in the entire repo, so in this PR I have simply ignored all uncovered lines. But going forward I think it is fair that each person is responsible for testing their own code.

If parts of the code is too hard to test or if you for some other reason actively don't want to test it, you can ignore it with

# pragma: no cover

And this is serves as a useful warning to have in the codebase for the reviewer or other users that that this part of the code has not been consciously not been tested.

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