HDDS-3783. Upload coverage even if tests failed

[adoroszlai]

What changes were proposed in this pull request?

PRs get code coverage feedback from Codecov thanks to changes implemented in HDDS-3726. Codecov needs coverage data for each new revision on master. However, coverage check is currently skipped if any tests fail. This PR proposes to always execute coverage if basic compilation is OK. It still waits for various test runs before execution.

https://issues.apache.org/jira/browse/HDDS-3783

How was this patch tested?

Tested conditions on simplified workflow file with both:

• successful "compilation": code, run
• and failed "compilation": code, run

Real CI ("unfortunately" all tests passed):
https://github.com/adoroszlai/hadoop-ozone/runs/761992786

[elek]

I was thinking about the same, but I am afraid if we enable it, we got invalid coverage data. Let's say it-client is failing, at least the failing tests will be missing. For me it seems to be more reliable to upload only the full builds.

[adoroszlai]

I was thinking about the same, but I am afraid if we enable it, we got invalid coverage data. Let's say it-client is failing, at least the failing tests will be missing. For me it seems to be more reliable to upload only the full builds.

I think we get invalid coverage data with the current setup, too. For example #1055 only changes some CSI yaml files, but coverage is said to decrease. I think that's due to using wrong base coverage data.

[codecov-commenter]

Codecov Report

Merging #1062 into master will decrease coverage by 0.08%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master    #1062      +/-   ##
============================================
- Coverage     69.48%   69.40%   -0.09%     
+ Complexity     9110     9109       -1     
============================================
  Files           961      961              
  Lines         48132    48127       -5     
  Branches       4672     4676       +4     
============================================
- Hits          33446    33403      -43     
- Misses        12468    12508      +40     
+ Partials       2218     2216       -2     

Impacted Files	Coverage Δ	Complexity Δ
.../org/apache/hadoop/hdds/scm/pipeline/Pipeline.java	85.71% <100.00%> (+0.20%)	44.00 <0.00> (+1.00)
.../org/apache/hadoop/ozone/om/helpers/OmKeyInfo.java	86.25% <100.00%> (+0.33%)	42.00 <0.00> (+2.00)
...adoop/ozone/om/helpers/OmKeyLocationInfoGroup.java	75.60% <100.00%> (+1.25%)	12.00 <2.00> (+1.00)
...g/apache/hadoop/ozone/om/helpers/OzoneAclUtil.java	81.91% <100.00%> (+1.23%)	39.00 <4.00> (+2.00)
...adoop/ozone/om/request/key/OMKeyCommitRequest.java	97.00% <100.00%> (ø)	18.00 <0.00> (+1.00)
...p/ozone/om/ratis/utils/OzoneManagerRatisUtils.java	67.44% <0.00%> (-19.13%)	39.00% <0.00%> (ø%)
...che/hadoop/ozone/om/ratis/OMRatisSnapshotInfo.java	83.33% <0.00%> (-10.67%)	7.00% <0.00%> (-5.00%)
...ache/hadoop/ozone/om/codec/S3SecretValueCodec.java	90.90% <0.00%> (-9.10%)	3.00% <0.00%> (-1.00%)
...va/org/apache/hadoop/hdds/utils/db/RDBMetrics.java	85.71% <0.00%> (-7.15%)	13.00% <0.00%> (-1.00%)
.../transport/server/ratis/ContainerStateMachine.java	69.36% <0.00%> (-6.76%)	59.00% <0.00%> (-5.00%)
... and 28 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 53395a0...26f94fa. Read the comment docs.

[elek]

I think that's due to using wrong base coverage data.

I am not sure. I thought codecov compares the branch with the master instead of git merge-base. Seems to be better to have less frequent but good coverage data on the master. With uploading failing test data I can't be sure that the coverage on the sonarcloud is valid or not.

It would be better to understand the problem of codecov and adjust it. Are you sure that this is the missing piece for the codecov?

[elek]

BTW we can have different rules for codecov / sonarcloud AFAIK.

[adoroszlai]

Codecov seems to agree with you. ;) It will not compare against commits with failed CI (if it know that CI passed or failed). From the doc:

If CI fails, one or more of the following assumptions can be made:

1. Not all tests were executed; therefore coverage is incomplete.
2. Exceptions may call new execution paths, resulting in different coverage metrics.
3. A failed test could produce different coverage than the same test ran successfully.

I think for Ozone:

1. is the worst problem, can happen if dependent modules are skipped by Maven due to earlier failure (eg. in common).
2. is true even for successful CI (example, where only javadoc was updated).
3. is something we should try to fix (eg. split test cases that perform a long series of steps, making assertions all the way).