Skip to content

backend: config: Refactor TestParse function#3512

Closed
andoriyaprashant wants to merge 9 commits intokubernetes-sigs:mainfrom
andoriyaprashant:config
Closed

backend: config: Refactor TestParse function#3512
andoriyaprashant wants to merge 9 commits intokubernetes-sigs:mainfrom
andoriyaprashant:config

Conversation

@andoriyaprashant
Copy link
Contributor

Summary

This PR refactors the TestParse function in config_test.go to reduce its length and complexity. The function was previously too long and had the linter disabled using //nolint:funlen

Related Issue

Fixes #3287

Changes

  • Split TestParse into smaller, individual test functions.
  • Removed the //nolint:funlen comment.
  • All tests pass successfully.
  • The linter now passes for this file without any issues.

Test

=== RUN   TestParseNoArgsNoEnv
--- PASS: TestParseNoArgsNoEnv (0.00s)
=== RUN   TestParseWithArgs
--- PASS: TestParseWithArgs (0.00s)
=== RUN   TestParseFromEnv
--- PASS: TestParseFromEnv (0.00s)
=== RUN   TestParseBothArgsAndEnv
--- PASS: TestParseBothArgsAndEnv (0.00s)
=== RUN   TestParseOidcSettingsWithoutInCluster
{"level":"error","source":"F:/headlamp/backend/pkg/config/config.go","line":171,"error":"oidc-client-id, oidc-client-secret, oidc-idp-issuer-url, oidc-validator-client-id, \n\t\toidc-validator-idp-issuer-url, flags are only meant to be used in inCluster mode","time":"2025-06-24T04:42:00+05:30","message":"validating config"}
--- PASS: TestParseOidcSettingsWithoutInCluster (0.00s)
=== RUN   TestParseInvalidBaseURL
{"level":"error","source":"F:/headlamp/backend/pkg/config/config.go","line":171,"error":"base-url needs to start with a '/' or be empty","time":"2025-06-24T04:42:00+05:30","message":"validating config"}
--- PASS: TestParseInvalidBaseURL (0.00s)
=== RUN   TestParseKubeconfigFromDefaultEnv
--- PASS: TestParseKubeconfigFromDefaultEnv (0.00s)
=== RUN   TestParseEnableDynamicClusters
--- PASS: TestParseEnableDynamicClusters (0.00s)
PASS
ok      github.com/kubernetes-sigs/headlamp/backend/pkg/config

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 23, 2025
@k8s-ci-robot k8s-ci-robot requested review from skoeva and sniok June 23, 2025 23:21
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: andoriyaprashant
Once this PR has been reviewed and has the lgtm label, please assign joaquimrocha for approval. For more information see the 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

@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 23, 2025
Copy link
Contributor

@skoeva skoeva left a comment

Choose a reason for hiding this comment

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

Thanks for looking into this issue! We can avoid duplicating code by using table-driven tests, I think it's more idiomatic in situations like this

@andoriyaprashant andoriyaprashant requested a review from skoeva July 1, 2025 20:13
Copy link
Contributor

@skoeva skoeva left a comment

Choose a reason for hiding this comment

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

Good work, would just suggest a couple of things before approval:

  • Squash all your changes (and rebases) into one atomic commit
  • Adjust the PR title and commit title to the following format: backend: config: Refactor TestParse function

@andoriyaprashant andoriyaprashant changed the title Refactor Config Parsing Tests to Improve Readability and Meet Linter Standards backend: config: Refactor TestParse function Jul 3, 2025
Copy link
Contributor

@skoeva skoeva left a comment

Choose a reason for hiding this comment

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

Would you be able to squash your changes into an atomic commit? In the same format as before (backend: config: Refactor TestParse function)

@andoriyaprashant
Copy link
Contributor Author

Hello @skoeva I was facing some local issues with this config branch, so I have opened a new pull request with the same changes as suggested. I am closing this PR now. Kindly review Pull request #3610
Thank you

Copy link
Contributor

@skoeva skoeva left a comment

Choose a reason for hiding this comment

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

what issues were you having? it isn't advisable to reopen PRs because of e.g. rebase issues, is this the case? git rebase -i should be good to wrap up the changes on this branch

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

Labels

cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

Development

Successfully merging this pull request may close these issues.

backend: Refactor config_test.go TestParse to be shorter

3 participants