Improving codecov

[abpolym]

Disclaimer: I want to improve code coverage in this branch, so it would be nice if you do not merge yet until I add more tests and fix the ones that I skipped (with @testitem ... skip=true) for now.
Maybe you could trigger Codecov manually to update the branches it sees, so I can see how much code coverage improved?

As of now, this PR will change code test coverage by +2.07% (from 76.17% to 78.24%. Previously the 12 month trend was at +4.24%).

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.35%. Comparing base (8a0be85) to head (cfa513e).
⚠️ Report is 31 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #539      +/-   ##
==========================================
+ Coverage   76.17%   80.35%   +4.17%     
==========================================
  Files         207      211       +4     
  Lines        6149     6179      +30     
==========================================
+ Hits         4684     4965     +281     
+ Misses       1465     1214     -251     

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[abpolym]

Alright after the tests have run, coverage should be improved by about 4% or more - which approximately 12 months of test coverage progress according to CodeCov. You can review and merge after the CI tests have gone through and I will add more tests in another branch.

[wouterwln]

In general, I think this is a good effort. I think we should revise the test suite of nodes anyway. Can I ask you @abpolym to take a junior phd student next tuesday and go through this? I think if we're changing the test suite anyway we should also test the rules for numerical stability, and we can maybe remove the tests for constructors and such to see if we can test the functionality and interfaces rather than this specific code (which we will have to change if the interface ever changes)

[wouterwln]

I in general don't like tests like this, because what happens if we change the interface and/or constructor and/or field names of these structs? Let's aim to design our tests in such a way that we can change the underlying structs if we want and we purely test the actual functionality. Of course the tests we actually design should under the hood call the constructor and such, I'm just a bit afraid we will have a lot of legacy code in tests as well if we just design tests to improve the codecov percentage over actual functionality

[abpolym]

I agree we should avoid tests that depend on struct internals or field names due to maintainability risks, but in this case, the tests are asserting public functional behavior — not implementation details.
This testitem verifies that the distribution’s external interface (support, prod rule) behaves as expected, which is precisely what should remain stable even if the internal implementation changes, i.e. is the support property of the distribution in a real interval (0, ∞), and does the default_prod_rule actual works for two GammaShapeLikelihood?
If future refactors break these tests, that would likely indicate a semantic change that should be consciously reviewed, not a superficial one.

What are your suggestions to improve the test? Remove it? Change it - and how?

[bvdmitri]

I think testing that s is Distributions.RealInterval is an implementation detail indeed. It can be MyRealInterval or SomeCoolSophisticatedLibrary.Real_interval and it doesn't really matter what type it is.

is the support property of the distribution in a real interval (0, ∞)

Thats a good test, but its not necessarily testing its type. A better test could be the one that tests this property without relying on the underlying type of the support

I think the support should have a public interface (I don't remember in my head) where you can query its left bound and test that it equals to zero and the right bound equals to typemax

[abpolym]

Alright I think something like

    # support should be (0.0, Inf)
    s = support(d)
    @test minimum(s) == 0.0
    @test maximum(s) == Inf

would be sufficient then - I also remove the Distributions.RealInterval check.

I will quickly check if any of my other tests also checks the same implementation details and then just replace/remove it. Then I commit

[github-actions]

🤖 Code Formatting

Your PR still has some code formatting issues. I've updated PR #543 with the necessary formatting changes.

You can merge that PR into this branch to fix the code style check.

Alternatively, you can run make format locally and push the changes yourself.

[abpolym]

Ok I had a quick look over all the tests and removed tests that test implementation details. I also saw that I had a duplicated testitem in the mixture node test that noone caught 👀

[bvdmitri]

So do you think its is ready now for review/merge @abpolym ?

[abpolym]

Yes @bvdmitri :)