Skip to content

chore: add SetupServer unit tests - #4021

Open
neelkumar01 wants to merge 2 commits into
kubeflow:masterfrom
neelkumar01:test/pkg-metrics-setup
Open

chore: add SetupServer unit tests#4021
neelkumar01 wants to merge 2 commits into
kubeflow:masterfrom
neelkumar01:test/pkg-metrics-setup

Conversation

@neelkumar01

@neelkumar01 neelkumar01 commented Sep 1, 2026

Copy link
Copy Markdown

What this PR does / why we need it:

This PR adds unit tests for pkg/metrics/setup.go to verify the behavior of SetupServer

The tests cover:

  • Insecure metrics server setup
  • Secure metrics server setup
  • Behavior when SecureServing is not specified
  • Disabled metrics server when BindAddress is set to "0"
  • Metrics server setup with authentication enabled
  • Error handling when adding the metrics server to the manager fails

These tests improve coverage for the metrics server setup logic and help ensure that future changes do not break its expected behavior

Notes

  • Tests are implemented using a table driven approach
  • A lightweight fake controller manager is created to verify manager interactions without requiring a real manager instance

Which issue(s) this PR fixes:
Fixes #4019

Checklist:

Signed-off-by: Neel Kumar <neelkumar247me@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

@neelkumar01 neelkumar01 changed the title test(metrics): add SetupServer unit tests chore: add SetupServer unit tests Sep 1, 2026
Signed-off-by: Neel Kumar <neelkumar247me@gmail.com>

@HarshPopat23 HarshPopat23 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @neelkumar01 for adding coverage for SetupServer; this area needs tests because it controls metrics exposure, TLS, and authentication.

I think one important issue should be addressed before approval. The insecure, secure, authentication-enabled, and unspecified-SecureServing cases all assert only whether manager.Add was called. Removing the TLS configuration, forcing SecureServing to false, or removing FilterProvider would still leave these tests passing.

Could we extract construction of metricsserver.Options into a small testable helper, or inject/capture the server constructor, and assert SecureServing, BindAddress, CertDir, TLSOpts, and FilterProvider directly? The registration tests can then remain focused on disabled-server behavior and manager error propagation.

Please also assert the identity of the manager.Add error with errors.Is rather than checking only that some error was returned.

One additional question: configuration defaulting sets SecureServing to true, while direct nil input to SetupServer is interpreted as false. The nil test should explicitly document and verify whichever behavior is intended.

With these changes, this would provide meaningful regression protection rather than primarily increasing line coverage.

@ArshVermaGit ArshVermaGit left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the PR adds tests only, but the required lgtm approval is still missing.

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

Labels

Projects

Status: Needs Triage

Development

Successfully merging this pull request may close these issues.

Add unit tests for metrics server setup

3 participants