Re-enable Codecov comments but delay PR notifications

[fingolfin]

Switch Codecov to manual notifications and send them once after the coverage-uploading test matrix finishes. This avoids noisy intermediate PR comments and temporary dropped-coverage reports while uploads are still in flight.

---

Follow-up to PR #6331. To repeat what I wrote there

I disabled Codecov comments 5 years ago in PR #4265, let's try them again. Back then this was done to fix issue #4260, reported by @ThomasBreuer, which was quite annoying.

That issue persists, but I've now added code which hopefully should avoid it. We'll see.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 78.61%. Comparing base (15ae669) to head (9ae4c48).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #6332   +/-   ##
=======================================
  Coverage   78.61%   78.61%           
=======================================
  Files         684      684           
  Lines      292662   292662           
  Branches     8656     8656           
=======================================
  Hits       230067   230067           
  Misses      60786    60786           
  Partials     1809     1809           

☔ 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.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[fingolfin]

Seems to be working as intended

[ThomasBreuer]

As far as I understand, this change solves the problem.
I think the automatic messages are useful, provided that they are telling correct numbers. Thus we can give this proposal a try.
(Shall the proposed solution regarded as a recommended setup also for other repositories?)

[lgoettgens]

I think this is way to complicated for what it tries to achieve. Codecov has a config setting which tells it how many uploads by coverage jobs to expect, and to only notify once all of them are there. Compared to all the changes in this PR, that would be a one-line change to the config. We are using that for years now for other repos, and it works as expected. One only needs to keep the number in the config in sync with the number of CI jobs that create coverage. See e.g. https://github.com/oscar-system/Oscar.jl/blob/cee2e4dd1f83b35e9b7fb233b48ebe99f33934fa/.codecov.yml#L4 for an example config.

[fingolfin]

@lgoettgens thank for pointing out that alternative approach using after_n_builds.

Codecov suggests two ways to deal with the "how to determine when all reports are in" issue: after_n_builds, and the method used in this PR.

Let's summarize what "all the changes" in this PR are:

1. inserting one extra CI step that triggers after all the other steps which upload coverage have completed (with IMHO straightforward and simple code);
2. enabling manual_trigger in the codecov config;
3. and finally the change this is all about, namly removing comment: false in the codecov config.

The alternative you suggest would essentially avoid 1; would still need 3; and would replace 2 with a slightly different change to the codecov config.

So when you write "this is way to complicated" I assumed you refer to part 1 of the change. But what exactly is complicated about it.

Yes, using after_n_builds would allow the patch here to be a bit smaller. But the next time someone adds or removes a CI job that uploads data, there is a good chance nobody remembers that setting and we'll again waste time noticing the resulting problem, debugging and then fixing it.. This may be OK for OSCAR where maybe several (?) people are active and know about the setting (you and perhaps Benjamin...? I certainly am not among them, actively, I have to relearn this each time). But for GAP, it may be that the next time someone touches this in two years and everyone forgot (like I forgot about adding that comment: false line)

So... from my perspective, the tradeoff seems to be between adding a couple more lines to a YAML file once, constituting a conceptual fix (it signals uploads are done when... uploads are done), versus a hardcoding a number of uploads (which strikes me as a hacky workaround, not a fix for the underlying issue), I don't really see any reason to prefer the latter? Am I missing something?

[lgoettgens]

Yeah, with your changes that reduced the diff, it doesn't seem that bad anymore. My preferred solution would be to be able to specify after_n_builds: all. But as long as this needs a hardcoded value, I can follow your reasoning here.
Thus, please don't see my comment above as an objection, but rather as pointing out an alternative, even though it is formulated more like the former. I have no strong preference neither way