etcd: upload coverage report even with failures

[ivanvc]

Restore the previous behavior. Always upload the coverage results even if the tests have failures.

I found this while working on etcd-io/etcd#19424, and by the workflow failure on etcd-io/etcd#19450.

/cc @jmhbnz

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ivanvc
Once this PR has been reviewed and has the lgtm label, please assign wenjiaswe for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• config/jobs/etcd/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[ivanvc]

/cc @ahrtr

[ahrtr]

Even make upload-coverage-report fails, the workflow will also be displayed as success?

[ivanvc]

Good point. The original implementation was swallowing that error, too. But let me address that.

[ivanvc]

Ah, hold on. If any command has a fail exit code, kubekins-e2e's runner.sh will make the job fail.

So, no. If the make upload-coverage-repot fails, the job will be marked as failed.

[ahrtr]

If any command has a fail exit code, kubekins-e2e's runner.sh will make the job fail.

If it's true, then can we simplify the script something like below,

            make test-coverage
            make upload-coverage-report

EDIT: or just make upload-coverage-report? (it's just the existing implementation:))

[ahrtr]

The original implementation was swallowing that error

Not sure this statement is true or I followed your point. My understanding is when executing make upload-coverage-report, it will fail directly if test-coverage fails

[ivanvc]

Ah, sorry, I may have forgotten to add more context. In etcd-io/etcd@7d87198, I'm also breaking the target prerequisite.

The old behavior was: If running the tests fails, then still upload the coverage report to Codecov.

After I refactored to import this job into Prow, the new behavior is: If running the tests fails, bail and don't upload the coverage report to Codecov.

Reference: etcd-io/etcd@6c86654#diff-1cdc3aec2106533afd6b4e545676ccbcd206d88ccd390b010d0c4b9f380c434c

With James' failure in the coverage job (etcd-io/etcd#19450) it made me realize that we won't upload the coverage report if the test fails.

So, this will restore the previous behavior (along with PR etcd-io/etcd#19424).

I hope that this clears up the confusion.

[ahrtr]

So,

• You are planning to merge this PR firstly, then github/workflows: remove coverage workflow etcd-io/etcd#19424 later?
• Your goal is to always upload the coverage data no matter the test-coverage is success or not?

[ivanvc]

You are planning to merge this PR firstly, then etcd-io/etcd#19424 later?

Given that the job definition is in one repository and the actual code for the job is in the other, editing them is less than ideal. However, I could also restore the original behavior from the etcd repo side in the Makefile by removing the prerequisite and executing $(MAKE) test-coverage || true.

The job will still work if we apply the change from this pull request. It will call the test-coverage target twice, but it won't make the job fail.

Your goal is to always upload the coverage data no matter the test-coverage is success or not?

Yes. I'm just restoring the original behavior before I refactored the code. For reference, this was the original code:

LOG_FILE=${1:-test-coverage.log}
# We collect the coverage
COVERDIR=covdir PASSES='build cov' ./scripts/test.sh 2>&1 | tee "${LOG_FILE}"
test_success="$?"
# We try to upload whatever we have:
bash <(curl -s https://codecov.io/bash) -f ./covdir/all.coverprofile -cF all || exit 2
# Expose the original status of the test coverage execution.
exit ${test_success}

I believe the idea is that even with the test failure, we get this summary: etcd-io/etcd#19424 (comment).

I don't have a strong opinion. I'm fine dropping this pull request and keeping the behavior I introduced in the refactor.

To summarize:

• Original behavior: Upload the coverage report even if tests failed (https://github.com/etcd-io/etcd/blob/5cd14a603145ed049be97abb3074da2e3497b2b3/scripts/codecov_upload.sh)
• Behavior after my refactor: Don't upload the coverage report if tests fail (https://github.com/etcd-io/etcd/blob/6c866548dc5d319214c428f083ea72902617672d/scripts/codecov_upload.sh)

Please let me know if you think I should close this pull request. Or proceed any other way.