Chore/ctrl tests (#132)

* chore: removed unused errors from controller

The ChecksController had mutliple functions which should return errors
but no errors were returned. This commit fixes that and cleans up the tests
that expected errors to be returned (but were never also used, as no error could
ever be returned).

Signed-off-by: Bruno Bressi <[email protected]>

* chore: removed unused fields and errors

Removed the unsued error field from the error struct and the error check
that is no longer needed.

* chore: add log entry

There was no clear logging of why the error occurred, which made the log entry not that practical.

* test: cleanup on controller_test.go

Added a check for the reconcile method to actually check if the number of check's registered actually matches the expectation and fixed an error in the configuration of a check, which wasn't being registered.

Removed the duplicate check registration case, because this is currently impossible to occur, as the Reconcile function already checks if a check is registered.

* tests: remove test case

Non-existent checks cannot be unregistered, as the function only gets called if an existing check fails to run, if the check is missing from the new config and finally when all the checks are being removed on shutdown.

---------

Signed-off-by: Bruno Bressi <[email protected]>

pkg/sparrow/controller.go

@@ -134,7 +134,7 @@ func (cc *ChecksController) RegisterCheck(ctx context.Context, check checks.Chec
 	// Add prometheus collectors of check to registry
 	for _, collector := range check.GetMetricCollectors() {
 		if err := cc.metrics.GetRegistry().Register(collector); err != nil {
-			log.ErrorContext(ctx, "Could not add metrics collector to registry")
+			log.ErrorContext(ctx, "Could not add metrics collector to registry", "error", err)
 		}
 	}
 

pkg/sparrow/controller_test.go

@@ -115,7 +115,9 @@ func TestChecksController_Reconcile(t *testing.T) {
 			name:   "no checks registered yet but register one",
 			checks: []checks.Check{},
 			newRuntimeConfig: runtime.Config{Health: &health.Config{
-				Targets: []string{"https://gitlab.com"},
+				Targets:  []string{"https://gitlab.com"},
+				Interval: 1 * time.Second,
+				Timeout:  1 * time.Second,
 			}},
 		},
 		{
@@ -211,7 +213,7 @@ func TestChecksController_Reconcile(t *testing.T) {
 
 			cc.Reconcile(ctx, tt.newRuntimeConfig)
 
-			// iterate of the controllers checks and check if they are configured
+			// iterate of the controller's checks and check if they are configured
 			for _, c := range cc.checks.Iter() {
 				cfg := c.GetConfig()
 				assert.NotNil(t, cfg)
@@ -225,6 +227,9 @@ func TestChecksController_Reconcile(t *testing.T) {
 					assert.Equal(t, tt.newRuntimeConfig.Dns, cfg)
 				}
 			}
+
+			// check that the number of registered checks is correct
+			assert.Equal(t, len(tt.newRuntimeConfig.Iter()), len(cc.checks.Iter()))
 		})
 	}
 }
@@ -236,29 +241,21 @@ func TestChecksController_RegisterCheck(t *testing.T) {
 		check checks.Check
 	}{
 		{
-			name: "valid check",
+			name: "register one check",
 			setup: func() *ChecksController {
 				return NewChecksController(db.NewInMemory(), NewMetrics())
 			},
 			check: health.NewCheck(),
 		},
-		{
-			name: "duplicate check registration",
-			setup: func() *ChecksController {
-				cc := NewChecksController(db.NewInMemory(), NewMetrics())
-				check := health.NewCheck()
-				cc.RegisterCheck(context.Background(), check)
-				return cc
-			},
-			check: health.NewCheck(),
-		},
 	}
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			cc := tt.setup()
-
 			cc.RegisterCheck(context.Background(), tt.check)
+			if cc.checks.Iter()[0] != tt.check {
+				t.Errorf("Expected one check to be registered")
+			}
 		})
 	}
 }
@@ -272,10 +269,6 @@ func TestChecksController_UnregisterCheck(t *testing.T) {
 			name:  "valid check",
 			check: health.NewCheck(),
 		},
-		{
-			name:  "unregister non-existent check",
-			check: health.NewCheck(),
-		},
 	}
 
 	for _, tt := range tests {