Skip to content

Test both endpoints and slices for unit tests#19645

Merged
istio-testing merged 2 commits intoistio:masterfrom
howardjohn:pilot/es-unit-tests
Dec 28, 2019
Merged

Test both endpoints and slices for unit tests#19645
istio-testing merged 2 commits intoistio:masterfrom
howardjohn:pilot/es-unit-tests

Conversation

@howardjohn
Copy link
Copy Markdown
Member

@howardjohn howardjohn commented Dec 17, 2019

This change makes our controller tests run twice, once with endpoints
and once with endpoint slice. A few tests are not relevant to this, so
they are just using Endpoints as before. Additionally, the probing tests
were combined to use a table driven test to make this change simpler.

@howardjohn howardjohn requested a review from a team as a code owner December 17, 2019 20:53
@googlebot googlebot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label Dec 17, 2019
@istio-testing istio-testing added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Dec 17, 2019
@howardjohn
Copy link
Copy Markdown
Member Author

@rshriram @hzxuzhonghu please take a look

Copy link
Copy Markdown
Member

@hzxuzhonghu hzxuzhonghu left a comment

Choose a reason for hiding this comment

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

Overall lgtm

pod2 := generatePod("172.0.1.2", "pod2", "nsA", "", "node2", map[string]string{"app": "prod-app"}, map[string]string{})
for mode, name := range EndpointModeNames {
t.Run(name, func(t *testing.T) {
controller, fx := newFakeControllerWithOptions(fakeControllerOptions{mode: mode})
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this is not right, you need to copy the value of mode. otherwise, if the subtests are run in parrallel, when the function is run, the mode may be the last value of EndpointModeNames

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I think it doesn't apply here since mode isn't a pointer? I'll change it though since I'm not sure

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I put a hold on this to fix it. Once this is in I'll update to beta endpoint slice api

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It doesnot matter whether it is value type or pointer type, as mode is a variable

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I have fixed this, please take a look

@howardjohn howardjohn added the do-not-merge/hold Block automatic merging of a PR. label Dec 24, 2019
@howardjohn howardjohn removed the do-not-merge/hold Block automatic merging of a PR. label Dec 27, 2019
@howardjohn
Copy link
Copy Markdown
Member Author

/retest

@istio-testing istio-testing merged commit 7bcc6d0 into istio:master Dec 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants