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

Follow up on Readiness Reflector #774

Merged
merged 3 commits into from Jun 14, 2019
Merged

Follow up on Readiness Reflector #774

merged 3 commits into from Jun 14, 2019

Conversation

freehan
Copy link
Contributor

@freehan freehan commented Jun 14, 2019

  • minor bug fixes
  • add tons of unit tests.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 14, 2019
@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jun 14, 2019
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 14, 2019
@rramkumar1
Copy link
Contributor

/lgtm

@rramkumar1 rramkumar1 closed this Jun 14, 2019
@rramkumar1 rramkumar1 reopened this Jun 14, 2019
rramkumar1
rramkumar1 reviewed Jun 14, 2019
View reviewed changes
pkg/neg/manager_test.go
)

const (
ClusterID = "clusterid"

namespace1 = "ns1"
Copy link
Contributor

Choose a reason for hiding this comment

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

This is personal preference but it may be better to just inline these values per test. It makes it slightly easier so that you don't have to go back to the top of the file to figure out what the value is.

If its too much work to inline, don't worry about it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It changes too much stuff. Probably I will just do it next time.

@rramkumar1
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 14, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: freehan, rramkumar1

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 merged commit 5b2fd1d into kubernetes:master Jun 14, 2019
bowei
bowei reviewed Jun 14, 2019
View reviewed changes
pkg/neg/manager.go
// This function effectively removes duplicate port with different readiness gate flag if the rest of the field in port info is the same.
func filterPort(p1, p2 negtypes.PortInfoMap) {
for port, portInfo1 := range p1 {
if portInfo2, ok := p2[port]; ok {
Copy link
Member

Choose a reason for hiding this comment

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

portInfo2, ok := p2[port]
if !ok { continue }

pkg/neg/manager.go

// filterPort removes duplicate ports in p1 and p2 if the corresponding port info has the same target port and neg name.
// This function effectively removes duplicate port with different readiness gate flag if the rest of the field in port info is the same.
func filterPort(p1, p2 negtypes.PortInfoMap) {
Copy link
Member

Choose a reason for hiding this comment

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

This is very non-descriptive function name.

removeCommonPorts

Copy link
Member

Choose a reason for hiding this comment

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

where is the unit test for this

Copy link
Contributor Author

@freehan freehan Jun 17, 2019 •
edited

Choose a reason for hiding this comment

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

hmmmmmmm. forgot to write a test for this. adding

pkg/neg/manager_test.go
},
},
}
manager := NewTestSyncerManager(fake.NewSimpleClientset())
namer := manager.namer

for _, tc := range testCases {
if tc.stop {
Copy link
Member

Choose a reason for hiding this comment

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

use t.Run so you don't have to include tc.desc in all of your errors

pkg/neg/manager_test.go
}

for _, tc := range testCases {
ret := manager.ReadinessGateEnabledNegs(tc.namespace, tc.labels)
Copy link
Member

Choose a reason for hiding this comment

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

t.Run

pkg/neg/manager_test.go
}

for _, tc := range testCases {
if manager.ReadinessGateEnabled(tc.key) != tc.expect {
Copy link
Member

Choose a reason for hiding this comment

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

t.Run

pkg/neg/readiness/poller_test.go
}

func TestPollerEndpointRegistrationAndScanForWork(t *testing.T) {
poller := newFakePoller()
Copy link
Member

Choose a reason for hiding this comment

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

t.Parallel

pkg/neg/readiness/poller_test.go
expectEndpointCount: 0,
},
} {
tc.mutateState()
Copy link
Member

Choose a reason for hiding this comment

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

t.Run

pkg/neg/readiness/reflector_test.go
}

func TestSyncPod(t *testing.T) {
fakeContext := fakeContext()
Copy link
Member

Choose a reason for hiding this comment

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

t.Parallel()

pkg/neg/readiness/reflector_test.go
},
},
} {
tc.mutateState()
Copy link
Member

Choose a reason for hiding this comment

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

t.Run()

@bowei
Copy link
Member

bowei commented Jun 14, 2019

What are we getting for test coverage with these tests?

@freehan
Copy link
Contributor Author

freehan commented Jun 17, 2019

What are we getting for test coverage with these tests?

$ go test -coverprofile=coverage.out ./...
ok  	k8s.io/ingress-gce/pkg/neg	2.460s	coverage: 63.9% of statements
?   	k8s.io/ingress-gce/pkg/neg/metrics	[no test files]
ok  	k8s.io/ingress-gce/pkg/neg/readiness	0.314s	coverage: 56.2% of statements
ok  	k8s.io/ingress-gce/pkg/neg/syncers	2.323s	coverage: 72.2% of statements
ok  	k8s.io/ingress-gce/pkg/neg/types	0.276s	coverage: 28.9% of statements
?   	k8s.io/ingress-gce/pkg/neg/types/shared	[no test files]

@freehan freehan mentioned this pull request Jun 26, 2019
Merged
k8s-ci-robot added a commit that referenced this pull request Jun 26, 2019
@k8s-ci-robot
Merge pull request #786 from freehan/unit-test
1a4ff08 
Follow up on #774
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bowei bowei bowei left review comments

@rramkumar1 rramkumar1 rramkumar1 left review comments

@thockin thockin Awaiting requested review from thockin

Assignees

@rramkumar1 rramkumar1

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@freehan @rramkumar1 @k8s-ci-robot @bowei