Proposal: ci codecov/patch reports but not fail

[Kwankyu Lee]

Hi,

Currently ci is in good shape except codecov/patch. 

codecov/patch works well: it reports the coverage change, and fails for a PR that does not test every code path. However, it is a problem that codecov/patch failure make the whole check for the PR fail.

It is not our coding standard that a doctest is added for every code path. A common example: we add "try except: ..." clause, but we do not add a doctest for the ... code path. 

Hence we learned to ignore check failure due to codecov/patch failure. It is annoying to "check and then ignore".

Proposal: have codecode/patch report but not fail.

Kwankyu

[Kwankyu Lee]

No comment will be construed as no objection.

[kcrisman]

On Saturday, October 5, 2024 at 7:47:33 AM UTC-4 Kwankyu Lee wrote:

No comment will be construed as no objection.

I guess I'm not actively developing at this point, and doubtless there are many untested code paths existing.  I do think that on the whole I've found it pretty useful in the past to doctest as many edge cases (including try/except) as possible.  But I don't know how big a problem the codecov issue is, since that didn't exist when I was more active in reviewing/writing :-)  Still, "untested is broken", right?

[Kwankyu Lee]

....  But I don't know how big a problem the codecov issue is ...

We want to regard the check failure as there is a problem with the PR that the author should resolve. 

Currently the codecov failure triggers the check failure, but no reviewer and no author regard the codecov failure as a problem with the PR (this is the practice that you are used to)

The check failure by the codecov failure is just annoying.

 

Still, "untested is broken", right?

This is still a good maxim. But our practice is "broken is then tested". I think our practice is not bad. Testing every code path would bloat our set of doctests.

[Dima Pasechnik]

"normal" codepaths ought to be tested. Error processing with try/except blocks might be hard to test in the 1st place, and less crucial.

[John H Palmieri]

There are ways to "bloat" the set of doctests with minimal impact. For example, we could create a file "TESTS.py" (for example) in a Sage module, consisting only of doctests. It would not be included in the reference manual, not visible when someone does "X.my_favorite_method?" or "X.my_favorite_method??", and since it's a separate file, many developers wouldn't interact with it at all. There may already be some files like that in the Sage library.

I don't know if this approach is worth it, but it does provide a way to add more doctests with minimal impact on most users and developers.

  John

 

[Kwankyu Lee]

There are ways to "bloat" the set of doctests with minimal impact. For example, we could create a file "TESTS.py" (for example) in a Sage module, consisting only of doctests. It would not be included in the reference manual, not visible when someone does "X.my_favorite_method?" or "X.my_favorite_method??", and since it's a separate file, many developers wouldn't interact with it at all. There may already be some files like that in the Sage library.

Yes. Those ways may have their own advantages. But more doctests in whatever ways will lead to more time to test sage.

 

I don't know if this approach is worth it, but it does provide a way to add more doctests with minimal impact on most users and developers.

"more time testing sage" leads to "more developer time to test PRs". I think adding doctests just to test all code paths is wasting developer time.

It seems we have to choose between:

(1) We keep the status quo: not testing every code path created in the PR results in the PR check failure.

(2) we keep codecov/patch  as it is, but require to add doctests for every code path created in the PR. 

(3) We keep our current practice (add doctests for major functionality, and later doctests are added for broken cases). We change codecov/path to report but not fail.

I propose to take (3) .

[Kwankyu Lee]

(3) We keep our current practice (add doctests for major functionality, and later doctests are added for broken cases). We change codecov/path to report but not fail.

(3) is implemented in the PR https://github.com/sagemath/sage/pull/38812.  

[Eric Gourgoulhon]

Le dimanche 6 octobre 2024 à 01:07:06 UTC+2, Kwankyu Lee a écrit :

It seems we have to choose between:

(1) We keep the status quo: not testing every code path created in the PR results in the PR check failure.

(2) we keep codecov/patch  as it is, but require to add doctests for every code path created in the PR. 

(3) We keep our current practice (add doctests for major functionality, and later doctests are added for broken cases). We change codecov/path to report but not fail.

I propose to take (3) .

Agreed. 

Eric. 

[Dima Pasechnik]

It can be doctests, or other kinds of tests - all driven by pytest, as
done in e.g.
src/sage/manifolds/differentiable/symplectic_form_test.py


But yes, I'm for (3), i.e. for stopping the codecov from triggering a failure.

Dima

>
> John
>
>
>
> --
> You received this message because you are subscribed to the Google Groups "sage-devel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to sage-devel+...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/sage-devel/67c8b9ae-593f-4f43-bb4a-2cf10c241e02n%40googlegroups.com.

[Martin R]

I would like to report from a personal experiment.  Nudged towards looking at the codecov warnings by tscrim, I learned that several of them actually uncovered truly (non-obviously) dead code in https://github.com/sagemath/sage/pull/38446.  Making sure that the code code indeed not be reached by creating corresponding tests was a bit of work, but I think in the long run it pays.

(I completely support the current decision, though.)

Best wishes,

Martin