Repository: kubernetes/kubernetes
File: staging/src/k8s.io/endpointslice/reconciler_test.go (Go)

Code before:
	client := newClientset()
	setupMetrics()
	namespace := "test"
	svc, _ := newServiceAndEndpointMeta("foo", namespace)

	// start with 20 pods
	pods := []*corev1.Pod{}
	for i := 0; i < 20; i++ {
		pods = append(pods, newPod(i, namespace, true, 1, false))
	}

	r := newReconciler(client, []*corev1.Node{{ObjectMeta: metav1.ObjectMeta{Name: "node-1"}}}, defaultMaxEndpointsPerSlice)
	reconcileHelper(t, r, &svc, pods, []*discovery.EndpointSlice{}, time.Now())

	actions := client.Actions()
	assert.Len(t, actions, 1, "Expected 1 additional client actions as part of reconcile")
	assert.True(t, actions[0].Matches("create", "endpointslices"), "First action should be create endpoint slice")

	expectMetrics(t, expectedMetrics{desiredSlices: 1, actualSlices: 1, desiredEndpoints: 20, addedPerSync: 20, removedPerSync: 0, numCreated: 1, numUpdated: 0, numDeleted: 0, slicesChangedPerSync: 1})

	fetchedSlices := fetchEndpointSlices(t, client, namespace)
	reconcileHelper(t, r, &svc, pods[0:10], []*discovery.EndpointSlice{&fetchedSlices[0]}, time.Now())
	expectMetrics(t, expectedMetrics{desiredSlices: 1, actualSlices: 1, desiredEndpoints: 10, addedPerSync: 20, removedPerSync: 10, numCreated: 1, numUpdated: 1, numDeleted: 0, slicesChangedPerSync: 2})
}

func TestReconcileEndpointSlicesMetrics_HistogramDeltas(t *testing.T) {
	client := newClientset()
	namespace := "test"
	svc, endpointMeta := newServiceAndEndpointMeta("foo", namespace)

	// existing slice has one stale endpoint and one endpoint that should remain.
	stalePod := newPod(0, namespace, true, 1, false)
	keepPod := newPod(1, namespace, true, 1, false)
	newDesiredPod := newPod(2, namespace, true, 1, false)

	existingSlice := newEmptyEndpointSlice(1, namespace, endpointMeta, svc)
	existingSlice.Endpoints = []discovery.Endpoint{
		podToEndpoint(stalePod, &corev1.Node{}, &svc, discovery.AddressTypeIPv4),
		podToEndpoint(keepPod, &corev1.Node{}, &svc, discovery.AddressTypeIPv4),
	}
	createEndpointSlices(t, client, namespace, []*discovery.EndpointSlice{existingSlice})
	fetchedSlices := fetchEndpointSlices(t, client, namespace)

	r := newReconciler(client, []*corev1.Node{{ObjectMeta: metav1.ObjectMeta{Name: "node-1"}}}, defaultMaxEndpointsPerSlice)

	addedCountBefore, err := testutil.GetHistogramMetricCount(metrics.EndpointsAddedPerSync.WithLabelValues())
	handleErr(t, err, "endpointsAddedPerSyncCountBefore")
	addedSumBefore, err := testutil.GetHistogramMetricValue(metrics.EndpointsAddedPerSync.WithLabelValues())
	handleErr(t, err, "endpointsAddedPerSyncSumBefore")

	removedCountBefore, err := testutil.GetHistogramMetricCount(metrics.EndpointsRemovedPerSync.WithLabelValues())

Review comment (bug):
hmm this test is much complicated from what I had imagined when writing the metric testing guidelines [here](https://github.com/richabanker/community/blob/e87ec83a2740d74b95bd6adabfbf99d2ae8f22b3/contributors/devel/sig-instrumentation/metric-instrumentation.md#adding-a-new-metric) (check the "Add tests for the metric that validate its behavior under known conditions" section)

Its hard to tell from a first glance that this test named `TestReconcileEndpointSlicesMetrics_HistogramDeltas` is actually testing metrics, since it seems like we are simulating various multi-step scenarios making it hard to follow what we are trying to validate here... Creating elaborate scenarios in tests also make it difficult to be maintained in future, esp if the underlying logic changed leading to this test failing, one would need to understand what was the logic change, how this metric was working before based on this multi-step scenario, and then adjust each scenario accordingly. In general, we should prefer simple to the point tests, they should be treated as documentation for code, super easy to read and comprehend.

I was thinking maybe we could have a simpler setup, just enough for registering the metric, creating the pods, creating the reconciler, calling the `Reconcile()` method and verifying that the metric changes as expected (for this, use `testutil.CollectAndCompare()` so you can also verify the metric format, not just its values.)

Rough example:

```
func TestEndpointsDesiredMetric(t *testing.T) {
    // setup
	setupMetrics()
	client := newClientset()
	namespace := "test"
	svc, _ := newServiceAndEndpointMeta("foo", namespace)

	pods := []*corev1.Pod{
		newPod(0, namespace, true, 1, false),
		newPod(1, namespace, true, 1, false),
		newPod(2, namespace, true, 1, false),
		newPod(3, namespace, true, 1, false),
		newPod(4, namespace, true, 1, false),
	}

    // call reconcile()
	r := newReconciler(client, []*corev1.Node{{ObjectMeta: metav1.ObjectMeta{Name: "node-1"}}}, defaultMaxEndpointsPerSlice)
	reconcileHelper(t, r, &svc, pods, []*discovery.EndpointSlice{}, time.Now())

    // verify metric changed along with verifying its format (help text, labels, type)
	expectedMetric := `
		# HELP endpoint_slice_controller_endpoints_desired [ALPHA] Number of endpoints desired
		# TYPE endpoint_slice_controller_endpoints_desired gauge
		endpoint_slice_controller_endpoints_desired 5
	`
	if err := testutil.CollectAndCompare(metrics.EndpointsDesired, strings.NewReader(expectedMetric), "endpoint_slice_controller_endpoints_desired"); err != nil {
		t.Errorf("unexpected endpoints_desired metric: %v", err)
	}
}
```

@aojea for whether you have a preference over the test's simplicity here. 

Code after:
	client := newClientset()
	resetMetrics()
	namespace := "test"
	svc, _ := newServiceAndEndpointMeta("foo", namespace)

	// start with 20 pods
	pods := []*corev1.Pod{}
	for i := 0; i < 20; i++ {
		pods = append(pods, newPod(i, namespace, true, 1, false))
	}

	r := newReconciler(client, []*corev1.Node{{ObjectMeta: metav1.ObjectMeta{Name: "node-1"}}}, defaultMaxEndpointsPerSlice)
	reconcileHelper(t, r, &svc, pods, []*discovery.EndpointSlice{}, time.Now())

	actions := client.Actions()
	assert.Len(t, actions, 1, "Expected 1 additional client actions as part of reconcile")
	assert.True(t, actions[0].Matches("create", "endpointslices"), "First action should be create endpoint slice")

	expectMetrics(t, expectedMetrics{desiredSlices: 1, actualSlices: 1, desiredEndpoints: 20, addedPerSync: 20, removedPerSync: 0, numCreated: 1, numUpdated: 0, numDeleted: 0, slicesChangedPerSync: 1})

	fetchedSlices := fetchEndpointSlices(t, client, namespace)
	reconcileHelper(t, r, &svc, pods[0:10], []*discovery.EndpointSlice{&fetchedSlices[0]}, time.Now())
	expectMetrics(t, expectedMetrics{desiredSlices: 1, actualSlices: 1, desiredEndpoints: 10, addedPerSync: 20, removedPerSync: 10, numCreated: 1, numUpdated: 1, numDeleted: 0, slicesChangedPerSync: 2})
}

func TestReconcileEndpointSlicesMetrics_EndpointsAddedPerSync_CollectAndCompare(t *testing.T) {
	resetMetrics()
	client := newClientset()
	namespace := "test"
	svc, endpointMeta := newServiceAndEndpointMeta("foo", namespace)

	keepPod := newPod(1, namespace, true, 1, false)
	newDesiredPod := newPod(2, namespace, true, 1, false)

	// Existing slice has one endpoint that should remain. Reconcile should add exactly 1 endpoint.
	existingSlice := newEmptyEndpointSlice(1, namespace, endpointMeta, svc)
	existingSlice.Endpoints = []discovery.Endpoint{
		podToEndpoint(keepPod, &corev1.Node{}, &svc, discovery.AddressTypeIPv4),
	}
	createdSlice, err := client.DiscoveryV1().EndpointSlices(namespace).Create(context.TODO(), existingSlice, metav1.CreateOptions{})
	if err != nil {
		t.Fatalf("Expected no error creating EndpointSlice, got: %v", err)
	}

	r := newReconciler(client, []*corev1.Node{{ObjectMeta: metav1.ObjectMeta{Name: "node-1"}}}, defaultMaxEndpointsPerSlice)
	reconcileHelper(t, r, &svc, []*corev1.Pod{keepPod, newDesiredPod}, []*discovery.EndpointSlice{createdSlice}, time.Now())

	want := `
# HELP endpoint_slice_controller_endpoints_added_per_sync [BETA] Number of endpoints added on each Service sync
# TYPE endpoint_slice_controller_endpoints_added_per_sync histogram
endpoint_slice_controller_endpoints_added_per_sync_bucket{le="2"} 1