Skip to content
This repository has been archived by the owner on Apr 25, 2023. It is now read-only.

test: add test to validate the controller actions to keep the cluster data #1212

Conversation

hectorj2f
Copy link
Contributor

What this PR does / why we need it:

It is a continuation of #1201 which has been stale for a long period. I cherry picked their changes and added some tests.

This test also cover other functions of the kubefedcluster controller.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

the judgement of clusterChanged is wrong
@hectorj2f hectorj2f self-assigned this Apr 13, 2020
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 13, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hectorj2f

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 approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Apr 13, 2020
@hectorj2f hectorj2f force-pushed the hectorj2f/add_test_ctrl_cluster_data branch 2 times, most recently from e49807d to bba163b Compare April 13, 2020 22:55
Eventually(func() bool {
_, found = cc.clusterDataMap[kc.Name]
return found
}, 30*time.Second).Should(BeTrue())
Copy link
Contributor

Choose a reason for hiding this comment

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

Recommend: Verify that the data updated (API Endpoint) equals what is updated above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@irfanurrehman
Copy link
Contributor

@hectorj2f Thanks for doing this. The test itself looks quite allright. A few nits (see comments).

Additionally little bit of history for your reference:
We had the integration tests (the kind that your wrote here) earlier, where a local etcd and api server were used for individual tests; however we saw that the overall time taken to complete all tests against the PRs was crossing 1 hour. At some point we did fold all those integration tests into e2e to save time (currently the test run still takes close to 40+ mins).
The difference I am talking about is:
Test run with etcd and api-server in this PR: 7.507s
Pure unit test from an older build: .021s

Having said that, I do agree that the current test is not verifying the cluster state change and I think its ok to introduce the new test you have added, covering the missing piece as here (It would be worse in terms of time consumed to verify this scenario in e2e).
Can I also recommend that the current unit test be left behind as it is, and add the test that you have introduced separately under test/integration (If you think its easy enough to do)?
Again, really appreciate you doing this.

@hectorj2f
Copy link
Contributor Author

@irfanurrehman I'd check how this can all be introduced without causing a lot of confusion or messing up the code structure. I certainly realized that there are many pieces of the kubefed's code that are not tested either because it escapes from the e2e radar or it is too complex to create mocks for all these tess.

@hectorj2f hectorj2f force-pushed the hectorj2f/add_test_ctrl_cluster_data branch from bba163b to 2fbc981 Compare April 20, 2020 11:10
@hectorj2f
Copy link
Contributor Author

@irfanurrehman I addressed your comments. I didn't move the new test to test/integration due to the amount of additional changes that will imply.

@irfanurrehman
Copy link
Contributor

Thanks @hectorj2f
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 21, 2020
@k8s-ci-robot k8s-ci-robot merged commit 37e3e51 into kubernetes-retired:master Apr 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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 Indicates that a PR is ready to be merged. 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.

4 participants