Fix incorrect test cases to cover code that needs to be tested in TestValidateIPSet

[mengjiao-liu]

What type of PR is this?

/kind bug

What this PR does / why we need it:

Wrong test cases:

kubernetes/pkg/util/ipset/ipset_test.go

Lines 927 to 948 in 5bafc83

	{ // case[3]
	ipset: &IPSet{
	Name: "bar",
	SetType: BitmapPort,
	HashFamily: ProtocolFamilyIPV6,
	HashSize: 0,
	MaxElem: 2048,
	},
	valid: false,
	desc: "wrong hash size number",
	},
	{ // case[4]
	ipset: &IPSet{
	Name: "baz",
	SetType: BitmapPort,
	HashFamily: ProtocolFamilyIPV6,
	HashSize: 1024,
	MaxElem: -1,
	},
	valid: false,
	desc: "wrong hash max elem number",
	},

According to the testcases desc field, we can see that case[3] is the test wrong hash size number
case[4] is to test wrong hash max elem number
However, when building the test case, the value set by the IPSet.SetType fields is BitmapPort,

kubernetes/pkg/util/ipset/ipset_test.go

Line 930 in 5bafc83

SetType: BitmapPort,

kubernetes/pkg/util/ipset/ipset_test.go

Line 941 in 5bafc83

SetType: BitmapPort,

which does not conform to the description of the desc field.
And this will cause the test to jump directly to verifying the PortRange of BitmapPort. At this time, PortRange == "", so it will definitely return false,

kubernetes/pkg/util/ipset/ipset.go

Lines 107 to 109 in 5bafc83

	if set.SetType == BitmapPort {
	if valid := validatePortRange(set.PortRange); !valid {
	return false

and the verified HashSize and MaxElem part code that needs to be tested is not tested.

kubernetes/pkg/util/ipset/ipset.go

Lines 112 to 121 in 5bafc83

	// check hash size value of ipset
	if set.HashSize <= 0 {
	klog.Errorf("Invalid hashsize value %d, should be >0", set.HashSize)
	return false
	}
	// check max elem value of ipset
	if set.MaxElem <= 0 {
	klog.Errorf("Invalid maxelem value %d, should be >0", set.MaxElem)
	return false
	}

When we modified the code, the test coverage increased from 85.0% to 87.2% and the code that verified the HashSize and MaxElem part code that was not covered previously was covered.

Run command:

go test -cover ./pkg/util/ipset/

Befor：

ok      k8s.io/kubernetes/pkg/util/ipset        0.919s  coverage: 85.0% of statements

After：

ok      k8s.io/kubernetes/pkg/util/ipset        (cached)        coverage: 87.2% of statements

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

/sig testing

[k8s-ci-robot]

@mengjiao-liu: This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[aojea]

great catch
/lgtm
/approve
/assign @thockin

[mengjiao-liu]

ping @thockin

[thockin]

These tests pass today (these cases fail successfully).

Can you explain why this change is "more correclty incorrect" ?

Would it be too much to ask to expand the comments from // case[3] to something meaningful?

[thockin]

I see it's not actually validating the error message. Blech.

This could/should be refactored to return error and the test should verify the error message (strings.Contains or something).

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aojea, mengjiao-liu, thockin

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:

• pkg/util/ipset/OWNERS [thockin]

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