ci: disable codecov blocking PRs until it understands coverage

[moul]

Alternative option to #3088 to unlock #3003.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 63.34%. Comparing base (7ef606c) to head (69fffc6).
Report is 1 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #3090   +/-   ##
=======================================
  Coverage   63.34%   63.34%           
=======================================
  Files         548      548           
  Lines       78680    78680           
=======================================
  Hits        49836    49836           
+ Misses      25482    25481    -1     
- Partials     3362     3363    +1     

Flag	Coverage Δ
contribs/gnodev	61.16% <ø> (ø)
contribs/gnofaucet	14.82% <ø> (ø)
gno.land	67.13% <ø> (ø)
gnovm	67.90% <ø> (ø)
misc/genstd	79.72% <ø> (ø)
tm2	62.41% <ø> (+0.06%)	⬆️

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

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

[thehowl]

I think this is also related to the comment I posted: #3088 (comment)

IMO:

1. We should have codecov complain and fail the CI when there is a coverage drop, but we should ignore it when human review understands it not to be necessary. With ci: add a github bot to support advanced PR review workflows #3037, we can now make CI passing a requirement for merging, but potentially put exclusion rules like on codecov where succeeding is not required for merge.
2. For txtar tests in general, they shouldn't count towards coverage, because I think that incentivises just adding txtar tests and calling it a day. Integration tests are slow and they are decoupled from the code they're testing; I think they're necessary both because they fully test all of our systems together and allow us to test for really complex bugs, but that really testing code paths should happen as unit tests for the most part.

[thehowl]

Signal discussion:

morgan: 3003: I think we should simply add a few table tests for gno.land/pkg/gnoland/param.go, trying out all parameters and ensuring identity when going from string -> Param -> Param.String(), and error cases; add a test for LoadGenesisParamsFile on a specific test file. This should satisfy the codecov coverage requirements.

manfred: Making Codecov happy while testing fewer aspects than this readable end-to-end test: https://github.com/gnolang/gno/pull/3003/files#diff-1b2287b983182dfeeafd315a44921869f7c7545b1e0a883be82a737415cbc13c
If the goal is to write code not for testing but to satisfy Codecov, what is the true value of Codecov?
This simple txtar file tests not only the two aspects you mentioned but also the vmkeeper.Handler component, and it ensures that LoadGenesisParamsFile is calling the gno.land/genesis/params.toml file and that this file is free of formatting errors. This test is not designed to satisfy codecov; rather, it is crucial to maintain because it indicates that something—likely well unit-tested but not integration-tested—may be misconfigured.

guillhem: The thing is that we can say the same thing in the opposite way, changing go default behaviors to make Codecov happy

Morgan: I would argue the table test is better than this end-to-end test. The test might verify the full functioning on the node; but it does it in a few seconds. (No, "just making them parallel" is not a solution; there is still considerable more computation to be done and if we were to test everything as a txtar tests our CI would be immensely slow). The end-to-end test is in a completely different location compared to the code that it's testing; which has adjacent to it no code that actually show the cases and scenarios where it's supposed to fail and succeed.
And note I'm not saying unit tests are better; just that we need both.

Manfred: I disagree that WE need both in this case, we need both in general, but in this test we need txtar, and codecov needs the other one.

Morgan: Do you agree that when I test something in gno.land/ it shouldn't count as coverage for tm2/pkg/bft?

manfred: tm2 needs to be independent from gno.land. It should be self-sustainable in terms of coverage, with its own unit and integration tests.

morgan: I don't want to put a global -coverpkg ./... as that will just mark immediatelly 99% of the test in our codebase as ✨ magically covered ✨
4:37 PM

manfred: gno.land/... relies on gno.land/... Therefore, it is essential to conduct end-to-end tests, particularly to evaluate the default behaviors.
3088 isn't a global cover package; it's a gno.land specific cover package.
I'm fine with each top-level folder having the same rule. Right now, I have only added it to gno.land, which is the only thing we test end-to-end more extensively.

morgan: Ok, I was making an extreme example to make a point. I still don't think we should deviate from the notion of "we should count coverage in the package only relating to that package's tests."
4:39 PM

manfred: You can also run -cover locally if you want to ensure that we have unit tests in addition to integration tests.
However, my PR is marked red because it hasn't been tested, even though it is tested.
The two levels of coverage make sense to me: the per-package coverage and the per-main-component coverage. I believe Codecov should focus on the global coverage, while those considering employment at Codecov can simply use -cover on their computers.

I disagree that 3003 should be red in the current state.
Imagine my PR were green. When you review it, do you think it lacks tests? Would you follow our usual practice when reviewing a PR from anyone and say, "Please test this part; it's sensitive"?
When I review a PR and notice it lacks tests, even if the coverage is green, I will ask about it because I care about testing what matters.
Codecov is my friend because it helps me identify "never-tested code."

But basically here it's a false positive, and when I request tests, it's usually a false-negative.

Global -coverpkg for gno.land should IMO make codecov more a friend than a stupid dictator.
Making it actually a review companion.

guillhem: To me, we simply agree that we enforce that the default GO coverage should be respected. Again, it has nothing to do with Codecov, which simply forwards coverage from what we send to it.

Morgan: I don't want the default behaviour of contributors to be "Oh, I'll just test this with txtar" and then their coverage is green and I don't see warnings, because I think there's benefit in making small, targeted, fast tests, especially for checking different expected error conditions.

And another thing worth noting is that if we were actually testing with -coverpkg gno.land/..., then your full LoadGenesisParamsFile is considered automatically tested, just because it's run by default when you start a node in an integ test.

In the last point, I mean it's tested even without the integ test you added.

manfred: Yes, it's tested, I agree with this statement.

My txtar is adding another condition of testing.
My txtar isn't testing the initchainer globally, but specifically if the params are setup as expected.
If someone writes a txtar instead of a unit test, I'll ask them to write unit tests either instead or in addition.

My PR is an example where I wouldn't make this request because txtar is simply the more efficient, end-to-end, and readable option.
Codecov should be red when there is a real real problem. Our engineer eyes are still reviewing things, and we should review variable names and lack of tests where it makes sense.

Morgan: OK, whatever, let's go with it. Can we restrict it to gno.land/cmd/testdata calculating coverage for gno.land/... for the meantime?

If I have an example of somewhere where codecov would have warned us about not testing something, I'll bring this discussion up again..

---

Closing this.