Adding more unit tests for remove-clusters

[nikhiljindal]

Ref #58 and #145

Fixing TODOs from #151 and #149

Adding more unit tests for remove-clusters command.
Bulk of the PR is unit test changes, with minor bug fixes.

cc @csbell @G-Harmon @madhusudancs @perotinus

---

This change is 

[G-Harmon]

I looked at the non *_test.go files so far have a few questions about that.

---

Reviewed 5 of 8 files at r1.
Review status: 5 of 8 files reviewed at latest revision, all discussions resolved.

---

app/kubemci/pkg/gcp/backendservice/fake_backendservicesyncer.go, line 85 at r1 (raw file):

			} else {
				// Mark the link as removed.
				removeLinks[ig] = false

Can v.IGLinks have duplicates? If not, then this code doesn't look like it does anything.

---

app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer.go, line 361 at r1 (raw file):

sd != nil
why the check here?
Overall, this logic seems complex/fragile. partially, I don't quite follow what you're doing here.

---

Comments from Reviewable

[nikhiljindal]

Review status: 5 of 8 files reviewed at latest revision, 2 unresolved discussions.

---

app/kubemci/pkg/gcp/backendservice/fake_backendservicesyncer.go, line 85 at r1 (raw file):

Previously, G-Harmon wrote…

Can v.IGLinks have duplicates? If not, then this code doesn't look like it does anything.

Yes our tests have duplicates. Added a comment to make that explicit

---

app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer.go, line 361 at r1 (raw file):

Previously, G-Harmon wrote…

sd != nil
why the check here?
Overall, this logic seems complex/fragile. partially, I don't quite follow what you're doing here.

Updated the variable names to clarify this as discussed.

---

Comments from Reviewable

[nikhiljindal]

Thanks for the review @G-Harmon
Updated as per comments. PTAL

[G-Harmon]

I didn't quite follow the changes in loadbalancersyncer_test.go. I put questions there in that part of the patch.

---

Reviewed 3 of 8 files at r1, 2 of 2 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.

---

app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer.go, line 352 at r2 (raw file):

Sd
Why is this called sd? It returns a LoadBalancerStatus...

---

app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer_test.go, line 369 at r2 (raw file):

Quoted 4 lines of code…

	fhc := lbc.hcs.(*healthcheck.FakeHealthCheckSyncer)
	if len(fhc.EnsuredHealthChecks) == 0 {
		t.Errorf("unexpected health checks not created")
	}

Why do you remove some checks? like health checks, url maps, target proxies, forwarding rules?

---

app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer_test.go, line 426 at r2 (raw file):

	if !reflect.DeepEqual(fw.IGLinks, expectedFWIGLinks) {
		t.Errorf("unexpected IG links on firewall rule, expected: %v, got: %v", expectedFWIGLinks, fw.IGLinks)
	}

(Why don't you check as many things as before? like forwarding rules)

---

Comments from Reviewable

[nikhiljindal]

Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

---

app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer.go, line 352 at r2 (raw file):

Previously, G-Harmon wrote…

Sd
Why is this called sd? It returns a LoadBalancerStatus...

sd for status description :)
Renamed to status.

---

app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer_test.go, line 369 at r2 (raw file):

Previously, G-Harmon wrote…

	fhc := lbc.hcs.(*healthcheck.FakeHealthCheckSyncer)
	if len(fhc.EnsuredHealthChecks) == 0 {
		t.Errorf("unexpected health checks not created")
	}

Why do you remove some checks? like health checks, url maps, target proxies, forwarding rules?

They are not affected by RemoveFromClusters.
Those were essentially testing that CreateLoadBalancer is working as expected, which is duplicated in the relevant test already.

---

app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer_test.go, line 426 at r2 (raw file):

Previously, G-Harmon wrote…

	if !reflect.DeepEqual(fw.IGLinks, expectedFWIGLinks) {
		t.Errorf("unexpected IG links on firewall rule, expected: %v, got: %v", expectedFWIGLinks, fw.IGLinks)
	}

(Why don't you check as many things as before? like forwarding rules)

Same as above. Here am limiting the checks to resources that are affected by RemoveFromClusters.

---

Comments from Reviewable

[nikhiljindal]

Updated code as per comments.
@G-Harmon PTAL

[G-Harmon]

Feel free to merge as is, or add the one more tiny check.

---

Reviewed 2 of 2 files at r3.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer_test.go, line 369 at r2 (raw file):

Previously, nikhiljindal (Nikhil Jindal) wrote…

They are not affected by RemoveFromClusters.
Those were essentially testing that CreateLoadBalancer is working as expected, which is duplicated in the relevant test already.

ah, okay, sounds good.

---

app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer_test.go, line 452 at r3 (raw file):

be := fbs.EnsuredBackendServices[0]

Do we care about verifying both? I think I submitted this shortcoming, so totally optional.

---

Comments from Reviewable

[nikhiljindal]

Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer_test.go, line 452 at r3 (raw file):

Previously, G-Harmon wrote…

be := fbs.EnsuredBackendServices[0]

Do we care about verifying both? I think I submitted this shortcoming, so totally optional.

Good catch. Updated to verify both

---

Comments from Reviewable

[nikhiljindal]

Thanks for the review @G-Harmon
Updated code as per comments.

Will merge once tests pass.

[G-Harmon]

---

Reviewed 2 of 2 files at r4.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer_test.go, line 452 at r3 (raw file):

Previously, nikhiljindal (Nikhil Jindal) wrote…

Good catch. Updated to verify both

thanks!

---

Comments from Reviewable