Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix healthz checkerNames test so that it tests against the expected output #70753

Merged
merged 1 commit into from
Nov 9, 2018
Merged

fix healthz checkerNames test so that it tests against the expected output #70753

merged 1 commit into from
Nov 9, 2018

Conversation

logicalhan
Copy link
Member

What type of PR is this?

/kind bug

What this PR does / why we need it:

Fixes a test which is not currently testing expectations correctly. This test should be failing but is not since the assertion logic is flipped. This PR unflips the logic (fixing the test), refactors out some formatting functionality and adds a test for that.

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

NONE

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/bug Categorizes issue or PR as related to a bug. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 7, 2018
@logicalhan
Copy link
Member Author

/sig api-machinery

@k8s-ci-robot k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. area/apiserver and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Nov 7, 2018
@logicalhan logicalhan mentioned this pull request Nov 7, 2018
Merged
justinsb
justinsb reviewed Nov 7, 2018
View reviewed changes
staging/src/k8s.io/apiserver/pkg/server/healthz/healthz_test.go
@@ -148,10 +148,32 @@ func TestCheckerNames(t *testing.T) {
for _, tc := range testCases {
result := checkerNames(tc.have...)
t.Run(tc.desc, func(t *testing.T) {
if reflect.DeepEqual(tc.want, result) {
if !reflect.DeepEqual(tc.want, result) {
Copy link
Member

Choose a reason for hiding this comment

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

Nice spot :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

😃

Copy link
Member

Choose a reason for hiding this comment

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

wow

justinsb
justinsb reviewed Nov 7, 2018
View reviewed changes
staging/src/k8s.io/apiserver/pkg/server/healthz/healthz.go
@@ -187,14 +187,20 @@ func adaptCheckToHandler(c func(r *http.Request) error) http.HandlerFunc {

// checkerNames returns the names of the checks in the same order as passed in.
func checkerNames(checks ...HealthzChecker) []string {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we use checkerNames now. Maybe just rename the function to formatCheckerNames but keep the existing code, and change the comment to say that we are formatting them for printing/debugging...

staging/src/k8s.io/apiserver/pkg/server/healthz/healthz_test.go Outdated

func TestFormatCheckerNames(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

I would just fix the existing test so add the quotes (and so that it actually checks correctly ;-) ). This just looks like a debug helper function, so the tests are already a little disproportionate...

@justinsb
Copy link
Member

justinsb commented Nov 7, 2018

Nice find.

As checkerNames is only used in this one spot, I think it's probably better to enforce that (the rename to formatCheckerNames looks like a good one to help people), and just to fix the test to expect quoted values.

/ok-to-test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Nov 7, 2018
@justinsb
Copy link
Member

justinsb commented Nov 7, 2018

You reminded me that you wanted to use this function elsewhere!

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 7, 2018
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 7, 2018
@justinsb
Copy link
Member

justinsb commented Nov 7, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 7, 2018
@logicalhan
Copy link
Member Author

/test pull-kubernetes-e2e-gce-100-performance
/test pull-kubernetes-integration
/test pull-kubernetes-kubemark-e2e-gce-big
/test pull-kubernetes-e2e-kops-aws

@lavalamp
Copy link
Member

lavalamp commented Nov 9, 2018

/approve

nice catch

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lavalamp, logicalhan

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

The pull request process is described here

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 approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 9, 2018
@k8s-ci-robot k8s-ci-robot merged commit daea190 into kubernetes:master Nov 9, 2018
This was referenced Nov 20, 2018
Merged
Closed
Merged
Merged
k8s-ci-robot added a commit that referenced this pull request Dec 5, 2018
@k8s-ci-robot
Merge pull request #71285 from cheftako/automated-cherry-pick-of-#70753-
ee860a5 
#70676-#70971-upstream-release-1.12

Automated cherry pick of #70753 #70676 #70971 upstream release 1.12
@logicalhan logicalhan mentioned this pull request Dec 8, 2018
Closed
6 tasks
k8s-ci-robot added a commit that referenced this pull request Dec 13, 2018
@k8s-ci-robot
Merge pull request #71334 from cheftako/automated-cherry-pick-of-#713…
15008f0 
…25-upstream-release-1.10

Automated cherry pick of #70753, #70676 and #70971 upstream release 1.10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@justinsb justinsb justinsb left review comments

@lavalamp lavalamp lavalamp left review comments

@caesarxuchao caesarxuchao Awaiting requested review from caesarxuchao

@smarterclayton smarterclayton Awaiting requested review from smarterclayton

Assignees

@justinsb justinsb

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/apiserver cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. release-note-none Denotes a PR that doesn't merit a release note. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

healthz checker test doesn't test what it should
4 participants
@logicalhan @justinsb @lavalamp @k8s-ci-robot