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 unit tests for autoregister_controller.go reliable #45864

Merged
merged 1 commit into from
May 22, 2017
Merged

Fix unit tests for autoregister_controller.go reliable #45864

merged 1 commit into from
May 22, 2017

Conversation

shiywang
Copy link
Contributor

@shiywang shiywang commented May 16, 2017 •
edited

Fixes #45538
Still wip, and just have some questions which I left some comments in original issue above

@k8s-github-robot k8s-github-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 16, 2017
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 16, 2017
@shiywang shiywang mentioned this pull request May 16, 2017
Closed
@deads2k
Copy link
Contributor

deads2k commented May 16, 2017

o I using a cache.NewIndexer to replace previous fakeWatch and informers.NewSharedInformerFactory, and using indexers.Add() to manipulate Workqueue ?

Yeah, that's what we want.

I wrote a basic framwork of new unit test file, is this right ?

Choosing steps versus complete setup ahead of time is up to you, but using steps they'll need access to the workqeuue. I think I'd suggest having more test cases without steps. The idea is that you would set up all the private variables (indexers, list of APIServices to maintain, etc), then make a single call to the sync function and check the response. It takes a little more set up, but the individual tests are easy to reason about.

@shiywang shiywang force-pushed the autoregister_controller branch 2 times, most recently from 63b1e6e to f500c17 Compare May 21, 2017 15:08
@shiywang shiywang changed the title [WIP] Fix unit tests for autoregister_controller.go reliable Fix unit tests for autoregister_controller.go reliable May 21, 2017
@k8s-github-robot k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 21, 2017
@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels May 21, 2017
@shiywang
Copy link
Contributor Author

/release-note-none

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels May 21, 2017
@shiywang
Copy link
Contributor Author

shiywang commented May 22, 2017 •
edited

@deads2k could you have an early review ? not sure if I covered all scenarios

@deads2k
Copy link
Contributor

deads2k commented May 22, 2017

@deads2k could you have an early review ? not sure if I covered all scenarios

Nice job. lgtm

@deads2k
Copy link
Contributor

deads2k commented May 22, 2017

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 22, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, shiywang

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 22, 2017
@shiywang shiywang mentioned this pull request May 22, 2017
Closed
@shiywang
Copy link
Contributor Author

@k8s-bot pull-kubernetes-federation-e2e-gce test this

@shiywang
Copy link
Contributor Author

@k8s-bot kubemark e2e test this

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 45864, 46169)

@k8s-github-robot k8s-github-robot merged commit 83b49b5 into kubernetes:master May 22, 2017
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented May 22, 2017 •
edited

@shiywang: The following test(s) failed:

Test name Commit Details Rerun command
Jenkins verification f9137a23f85b33aa932f094c0c21da2b8fb77739 link @k8s-bot verify test this
Jenkins kops AWS e2e f9137a23f85b33aa932f094c0c21da2b8fb77739 link @k8s-bot kops aws e2e test this
Jenkins GCE etcd3 e2e f9137a23f85b33aa932f094c0c21da2b8fb77739 link @k8s-bot gce etcd3 e2e test this
pull-kubernetes-cross bc7f8f91fc76f2c58fa01b349acdea6a0b870586 link @k8s-bot pull-kubernetes-cross test this
pull-kubernetes-federation-e2e-gce 8fe3db7 link @k8s-bot pull-kubernetes-federation-e2e-gce test this

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

@shiywang shiywang deleted the autoregister_controller branch May 25, 2017 14:14
@deads2k deads2k mentioned this pull request May 26, 2017
Merged
k8s-github-robot pushed a commit that referenced this pull request May 26, 2017
Merge pull request #46510 from deads2k/agg-31-stable
3912675 
Automatic merge from submit-queue

remove duplicate, flaky tests

These tests were replaced in #45864, but we forgot to remove the originals.  This just removes the originals, but all the test cases were covered in that other pull.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@smarterclayton smarterclayton

@deads2k deads2k

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. release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@shiywang @deads2k @k8s-github-robot @k8s-ci-robot @smarterclayton