[Part 1]Add unit tests for reconcile methods #250

bhiravabhatla · 2021-04-19T03:19:30Z

Signed-off-by: santosh [email protected]

Resolves #35

Add unit tests for configmap, daemonset, deployment reconcilers
Fix Patch calls for configmap, daemonset, deployment reconcilers

Raised #261 for service and service account reconcilers

Signed-off-by: santosh <[email protected]>

bhiravabhatla · 2021-04-19T03:24:44Z

@jpkrohling - I am unable to test Patch call (updating existing configmap) with envtest.

		err = expectedConfigMaps(context.Background(), params(), []v1.ConfigMap{configMap()}, true)
		assert.NoError(t, err)

While running expectedConfigMaps with existing configmap, I am seeing below error returned.

failed to apply changes: the name of the object (test-collector based on URL) was undeterminable: name must be provided

While running on debug - i could see the control reaching the Patch statement -

if err := params.Client.Patch(ctx, updated, patch); err != nil {
			return fmt.Errorf("failed to apply changes: %w", err)
		}

And Patch is returning above error. Any clues what might be causing this. Current PR has this particular test commented.

Signed-off-by: santosh <[email protected]>

alolita · 2021-04-22T21:05:14Z

@humivo @IrisTuntun can you take a look at this PR too for the Operator enhancements we're working on?

bhiravabhatla · 2021-04-23T03:32:56Z

I would need help with above blocker. If this is resolved, I can add tests for all the other reconcilers similarly.

Signed-off-by: santosh <[email protected]>

bhiravabhatla · 2021-04-25T08:59:39Z

@jpkrohling - I guess there is a bug in Patch logic -

opentelemetry-operator/pkg/collector/reconcile/configmap.go

Line 123 in db40230

patch := client.MergeFrom(&params.Instance)

We are generating patch from Instance, (not sure why). If I generate the patch from existing configmap [ which should be the case, as we are pacthing configmap - not openTelemetryCollector CR ] , like below -

patch := client.MergeFrom(existing)

Patch Tests are passing. Below is the documentation of mergeFrom function - MergeFrom creates a Patch that patches using the merge-patch strategy with the given object as base.

// MergeFrom creates a Patch that patches using the merge-patch strategy with the given object as base.
// The difference between MergeFrom and StrategicMergeFrom lays in the handling of modified list fields.
// When using MergeFrom, existing lists will be completely replaced by new lists.
// When using StrategicMergeFrom, the list field's `patchStrategy` is respected if specified in the API type,
// e.g. the existing list is not replaced completely but rather merged with the new one using the list's `patchMergeKey`.
// See https://kubernetes.io/docs/tasks/manage-kubernetes-objects/update-api-object-kubectl-patch/ for more details on
// the difference between merge-patch and strategic-merge-patch.
func MergeFrom(obj runtime.Object) Patch {
	return &mergeFromPatch{patchType: types.MergePatchType, createPatch: createMergePatch, from: obj}
}

Same is the case for all the patch calls ( configmap, daemonset, deployment , etc). Kindly confirm.

I ll update this thread after testing the operator using above fix

bhiravabhatla · 2021-04-25T11:23:54Z

I ll update this thread after testing the operator using above fix

Works well. I could see deployment being patched if I manually edit the deployment. I am pushing the above change with this PR if its ok. @jpkrohling - Thoughts?

We were generating patches from params.Instance as base for configmap,deployments & daemonsets. Get the patch from existing resource instead. Signed-off-by: santosh <[email protected]>

Signed-off-by: santosh <[email protected]>

jpkrohling · 2021-04-26T10:17:06Z

Sorry for taking so long to look into this, I'll review this today or, at the latest, tomorrow.

IrisTuntun · 2021-04-26T17:09:44Z

Hi, I'm working on adding StatefulSet to reconcile and have a test added in this PR. The test works without changing the MergeFrom logic. I was wondering if my test strategy makes sense here.

jpkrohling

It's looking great! Would you like to get this one merged first, before working on the next tests? This way, we don't get huge PRs in the queue.

pkg/collector/reconcile/configmap.go

jpkrohling · 2021-04-28T09:25:29Z

pkg/collector/reconcile/configmap_test.go

+
+func TestDesiredConfigMap(t *testing.T) {
+	t.Run("should return expected config map", func(t *testing.T) {
+		expected := configMap("test-collector")


I would skip the local configMap, and make explicit assertions about certain aspects of the returned config map, like checking the returned labels and the name, and possibly the config.

jpkrohling · 2021-04-28T09:25:58Z

pkg/collector/reconcile/configmap_test.go

+
+func TestExpectedConfigMap(t *testing.T) {
+
+	cm := configMap("test-collector")


You can call the desiredConfigMap here, as you are testing its correctness in another test.

jpkrohling · 2021-04-28T09:28:32Z

pkg/collector/reconcile/daemonset_test.go

+	})
+}
+
+func daemonset(name string) v1.DaemonSet {


Same as the configmap: if you have a test for the desiredDaemonSet, this one here could probably be removed in favor of the official one.

Yep makes sense.

Same as below

jpkrohling · 2021-04-28T09:28:56Z

pkg/collector/reconcile/deployment_test.go

+	})
+}
+
+func deployment(name string) v1.Deployment {


For delete test cases - I cant use desiredDaemonSet/desiredDeployment - As I would need to a resource with same labels as the one in Params but with different name.

i.e.

Lables should be

Labels: map[string]string{
"app.kubernetes.io/managed-by": "opentelemetry-operator",
"app.kubernetes.io/instance": fmt.Sprintf("%s.%s", params.Instance.Namespace, params().Instance.Name),
},

but name should be different than params.Instance.Name. Its not possible using desiredDaemonSet/desiredDeployment functions. So I would have to retain the daemonset and deployment helper functions.

I think all you need for those cases is a deployment/daemonset with one or two labels (managed-by, I believe). This is what we have in the jaeger-operator:

https://github.com/jaegertracing/jaeger-operator/blob/4693d119aabb550a7a651315cbfce1833bb786fc/pkg/controller/jaeger/daemonset_test.go#L104-L114

The good thing about doing it like this is that we see exactly what has influence on the decision to remove the CR.

In our case - we are actually creating the object. So would need the entire spec, only namespacedName and labels wont suffice. Basically have to move everything in daemonset function to actual test. Will do the same then

bhiravabhatla · 2021-04-28T10:49:12Z

Would you like to get this one merged first, before working on the next tests?

Makes sense. I am half way through with service tests - I ll raise another PR for it.

Signed-off-by: santosh <[email protected]>

jpkrohling · 2021-05-03T11:44:48Z

Let me know once this is ready for another review.

Signed-off-by: santosh <[email protected]>

bhiravabhatla · 2021-05-04T13:26:04Z

Let me know once this is ready for another review.

Done. Also added one more test case each for daemonset and deployment to make sure we dont delete resources which are not owned by operator. Kindly review @jpkrohling

bhiravabhatla · 2021-05-04T13:28:05Z

Would you like to get this one merged first, before working on the next tests?

Makes sense. I am half way through with service tests - I ll raise another PR for it.

Raised second PR for this. Could you also take a look at - #261 @jpkrohling

jpkrohling · 2021-05-04T15:26:11Z

CodeQL seems stuck. Would you be able to do a "git commit --amend" and push it again?

Signed-off-by: santosh <[email protected]>

[WIP] Add unit tests for reconcile methods

db385ce

Signed-off-by: santosh <[email protected]>

bhiravabhatla requested review from a team April 19, 2021 03:19

bhiravabhatla added 2 commits April 19, 2021 09:45

[WIP] Fix linting errors

c06a901

Signed-off-by: santosh <[email protected]>

[WIP] Add test case for delete configmap

bf5f3d6

Signed-off-by: santosh <[email protected]>

alolita mentioned this pull request Apr 22, 2021

Add unit tests for OTel Operator StatefulSet enhancements open-telemetry/prometheus-interoperability-spec#47

Closed

bhiravabhatla added 3 commits April 24, 2021 10:05

[WIP] Add test cases for daemonset reconciler

85c91e5

Signed-off-by: santosh <[email protected]>

[WIP] Fix lint issues

9c13098

Signed-off-by: santosh <[email protected]>

[WIP] Add tests for deployment reconcile

c591104

Signed-off-by: santosh <[email protected]>

bhiravabhatla added 2 commits April 25, 2021 17:05

[WIP] Fix issue with patch calls

7c2539e

We were generating patches from params.Instance as base for configmap,deployments & daemonsets. Get the patch from existing resource instead. Signed-off-by: santosh <[email protected]>

[WIP] Add tests for opentelelmetry CR reconciler

e04fc80

Signed-off-by: santosh <[email protected]>

IrisTuntun mentioned this pull request Apr 26, 2021

Add ExpectedStatefulSets in reconcile pkg open-o11y/opentelemetry-operator#1

Closed

jpkrohling reviewed Apr 28, 2021

View reviewed changes

[WIP] Implement feedback on tests

a8fcc95

Signed-off-by: santosh <[email protected]>

bhiravabhatla mentioned this pull request May 2, 2021

[Part 2]Add unit tests for reconcile methods #261

Merged

bhiravabhatla changed the title ~~[WIP] Add unit tests for reconcile methods~~ [Part 1]Add unit tests for reconcile methods May 2, 2021

Add test to cover delete scenario completly

bef5453

Signed-off-by: santosh <[email protected]>

jpkrohling approved these changes May 4, 2021

View reviewed changes

jpkrohling enabled auto-merge (squash) May 4, 2021 15:26

jpkrohling merged commit 70d7920 into open-telemetry:main May 4, 2021

bhiravabhatla deleted the reconcile_tests branch May 5, 2021 02:33

shree007 pushed a commit to shree007/opentelemetry-operator that referenced this pull request Dec 12, 2021

Add unit tests for reconcile methods - part 1 (open-telemetry#250)

27b8207

Signed-off-by: santosh <[email protected]>

ItielOlenick pushed a commit to ItielOlenick/opentelemetry-operator that referenced this pull request May 1, 2024

Add unit tests for reconcile methods - part 1 (open-telemetry#250)

f40cc2c

Signed-off-by: santosh <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Part 1]Add unit tests for reconcile methods #250

[Part 1]Add unit tests for reconcile methods #250

bhiravabhatla commented Apr 19, 2021 •

edited

Loading

bhiravabhatla commented Apr 19, 2021 •

edited

Loading

alolita commented Apr 22, 2021

bhiravabhatla commented Apr 23, 2021

bhiravabhatla commented Apr 25, 2021 •

edited

Loading

bhiravabhatla commented Apr 25, 2021

jpkrohling commented Apr 26, 2021

IrisTuntun commented Apr 26, 2021

jpkrohling left a comment

jpkrohling Apr 28, 2021

bhiravabhatla May 2, 2021

jpkrohling Apr 28, 2021

bhiravabhatla May 2, 2021

jpkrohling Apr 28, 2021

bhiravabhatla Apr 28, 2021

bhiravabhatla May 2, 2021

jpkrohling Apr 28, 2021

bhiravabhatla Apr 28, 2021

bhiravabhatla May 2, 2021

jpkrohling May 3, 2021

bhiravabhatla May 4, 2021

bhiravabhatla commented Apr 28, 2021

jpkrohling commented May 3, 2021

bhiravabhatla commented May 4, 2021

bhiravabhatla commented May 4, 2021

jpkrohling commented May 4, 2021


		func TestExpectedConfigMap(t *testing.T) {

		cm := configMap("test-collector")

[Part 1]Add unit tests for reconcile methods #250

[Part 1]Add unit tests for reconcile methods #250

Conversation

bhiravabhatla commented Apr 19, 2021 • edited Loading

bhiravabhatla commented Apr 19, 2021 • edited Loading

alolita commented Apr 22, 2021

bhiravabhatla commented Apr 23, 2021

bhiravabhatla commented Apr 25, 2021 • edited Loading

bhiravabhatla commented Apr 25, 2021

jpkrohling commented Apr 26, 2021

IrisTuntun commented Apr 26, 2021

jpkrohling left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bhiravabhatla commented Apr 28, 2021

jpkrohling commented May 3, 2021

bhiravabhatla commented May 4, 2021

bhiravabhatla commented May 4, 2021

jpkrohling commented May 4, 2021

bhiravabhatla commented Apr 19, 2021 •

edited

Loading

bhiravabhatla commented Apr 19, 2021 •

edited

Loading

bhiravabhatla commented Apr 25, 2021 •

edited

Loading