Skip to content

ci: restore test coverage reporting - #3077

Open
HarshPopat23 wants to merge 1 commit into
kubeflow:masterfrom
HarshPopat23:fix/restore-coverage-reporting
Open

HarshPopat23 wants to merge 1 commit into
kubeflow:masterfrom
HarshPopat23:fix/restore-coverage-reporting

Conversation

@HarshPopat23

@HarshPopat23 HarshPopat23 commented Aug 7, 2026 •

Copy link
Copy Markdown

What this PR does / why we need it

Spark Operator already generates cover.out as part of make unit-test, but the coverage profile is currently discarded when CI finishes. This means coverage trends and regressions are not visible during review.

This PR restores coverage reporting following the existing Kubeflow Trainer workflow pattern.

Changes

  • Uploads the existing cover.out profile to Coveralls after Go unit tests.
  • Uses the same pinned actions-goveralls action and configuration as Kubeflow Trainer.
  • Adds a Coveralls coverage badge to the README.
  • Keeps the existing unit-test coverage collection and package exclusions unchanged.

Coverage enforcement

This PR intentionally does not hard-code an arbitrary coverage threshold.

The first successful report on master should establish the repository baseline. Maintainers can then enable Coveralls status checks and configure the agreed coverage/decrease thresholds in the repository's Coveralls settings, as proposed in #3076.

A local Windows run produced an approximate 34.3% profile, but I am not treating that as the repository baseline because the envtest suites fail during AfterSuite teardown on native Windows when stopping kube-apiserver and etcd. The test specs themselves completed successfully; the authoritative baseline should come from the Linux CI run.

Validation

  • git diff --check
  • GitHub Actions workflow YAML parses successfully.
  • Verified the coverage upload configuration against Kubeflow Trainer's current Go test workflow.
  • Locally exercised the existing unit-test coverage path with Kubernetes 1.35 envtest assets; native Windows envtest teardown is unsupported, so the GitHub-hosted Linux run will provide the authoritative full result.

Related to #3076

Scope and operational prerequisite

This PR restores coverage profile reporting only.

Before uploads and the README badge can become functional, an authorized maintainer must enable kubeflow/spark-operator in Coveralls. The badge may display unknown until the first successful upload from master.

Coverage thresholds and reviewed exclusions for generated or non-testable code will be handled separately after the Linux CI baseline is available.

Coveralls repository enablement is tracked in #3089

Signed-off-by: HarshPopat23 <musichk61@gmail.com>
@google-oss-prow

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

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

Details Needs approval from an approver in each of these files:

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

@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown

🎉 Welcome to the Kubeflow Spark Operator! 🎉

Thanks for opening your first PR! We're happy to have you as part of our community 🚀

Here's what happens next:

Join the community:

Feel free to ask questions in the comments if you need any help or clarification!
Thanks again for contributing to Kubeflow! 🙏

@vjanelle

vjanelle commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

@HarshPopat23

Copy link
Copy Markdown
Author

Thanks @vjanelle. I used the same pinned actions-goveralls action and cover.out configuration from the Trainer workflow.

I omitted Trainer's working-directory input because Spark Operator runs make unit-test from the repository root, where cover.out is generated.

Please let me know if there is any additional part of the Trainer configuration you intended me to mirror.

@vikas-saxena02

vikas-saxena02 commented Aug 8, 2026 •

Copy link
Copy Markdown
Contributor

Nice PR — SHA-pinning the action and deferring the threshold rather than inventing one are both the right calls. A few things worth checking:

1. continue-on-error: true — Trainer has this on its Coveralls step ([test-go.yaml#L92(https://github.com/kubeflow/trainer/blob/master/.github/workflows/test-go.yaml)).
integration.yaml sets permissions: contents: read and fork PRs get a read-only token, so without it a Coveralls failure would fail CI on most community PRs. Trainer also guards its whole test job with
if: ${{ github.repository == 'kubeflow/trainer' }}, which this job doesn't have. What did the coverage step do on your own fork run?

2. Coveralls needs enabling first. Someone with admin has to add
kubeflow/spark-operator on coveralls.io before uploads are accepted, and the README badge will render broken until then. Worth noting in the description as a merge-ordering dependency.

3. The baseline may be understated. Makefile:193-194 sets KUBEBUILDER_ASSETS on one recipe line and runs go test on the next — separate shells, so go test never sees it. There are five envtest suites in the unit-tested set. This is pre-existing, but since the PR's purpose is establishing a baseline, it's worth fixing (&& or export) before a threshold is set against the number.

4. Generated code. #3076 asks for explicit exclusions for generated /non-testable code. zz_generated.deepcopy.go and the API packages are currently counted, which shifts the baseline. Probably follow-up, but flagging while the baseline conversation is open.

@HarshPopat23

Copy link
Copy Markdown
Author

@vikas-saxena02

Thanks for the detailed review. continue-on-error: true is already present. The step hasn’t run for my fork yet because the PR workflow is awaiting maintainer approval, and feature-branch pushes aren’t included by the workflow’s push trigger.

I agree that the repository must first be enabled in Coveralls. I’ll document that dependency and ask a maintainer to enable it before relying on the badge.

I also checked the KUBEBUILDER_ASSETS assignment. It does run in a separate shell, but all five envtest suites explicitly set BinaryAssetsDirectory to the downloaded bin/k8s/1.35.0-<os>-<arch> directory, so the suites do not rely on that environment variable. I suggest handling the ineffective assignment as a separate cleanup.

I agree generated-code exclusions need an explicit policy. I’ll clarify that this PR restores reporting only, with exclusions and enforcement remaining follow-up work after the first master baseline.

@HarshPopat23

Copy link
Copy Markdown
Author

cc: @andreyvelich @robert-bell

@tariq-hasan

Copy link
Copy Markdown
Member

/ok-to-test
/retest

@HarshPopat23

Copy link
Copy Markdown
Author

Thanks @tariq-hasan ,
Waiting for further guide.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants