Repository: clouditor/clouditor
Pull request #1124 (2023-06-01 17:36:05)

Update UpdateMetricConfiguration()
- Delete 2 unnecessary assignments

anatheka: @oxisto @lebogg after an UpdateMetricConfiguration() the is_default field is no longer available but the timestamp field is when you call GetMetricConfiguration(). Is that the way it's supposed to be?
codecov[bot]: ## [Codecov](https://app.codecov.io/gh/clouditor/clouditor/pull/1124?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=clouditor) Report
> Merging [#1124](https://app.codecov.io/gh/clouditor/clouditor/pull/1124?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=clouditor) (1501fdd) into [main](https://app.codecov.io/gh/clouditor/clouditor/commit/cb43506777fc225ebb87fe9f73071bda57b62254?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=clouditor) (cb43506) will **decrease** coverage by `0.05%`.
> The diff coverage is `100.00%`.

```diff
@@            Coverage Diff             @@
##             main    #1124      +/-   ##
==========================================
- Coverage   83.69%   83.64%   -0.05%     
==========================================
  Files          93       93              
  Lines        8588     8587       -1     
==========================================
- Hits         7188     7183       -5     
- Misses       1060     1063       +3     
- Partials      340      341       +1     
```

| Flag | Coverage Δ | |
|---|---|---|
| unittests | `83.64% <100.00%> (-0.05%)` | :arrow_down: |

Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=clouditor#carryforward-flags-in-the-pull-request-comment) to find out more.

| [Impacted Files](https://app.codecov.io/gh/clouditor/clouditor/pull/1124?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=clouditor) | Coverage Δ | |
|---|---|---|
| [service/orchestrator/metrics.go](https://app.codecov.io/gh/clouditor/clouditor/pull/1124?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=clouditor#diff-c2VydmljZS9vcmNoZXN0cmF0b3IvbWV0cmljcy5nbw==) | `79.17% <100.00%> (-0.07%)` | :arrow_down: |

... and [1 file with indirect coverage changes](https://app.codecov.io/gh/clouditor/clouditor/pull/1124/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=clouditor)


oxisto: > @oxisto @lebogg after an UpdateMetricConfiguration() the is_default field is no longer available but the timestamp field is when you call GetMetricConfiguration() via grpcurl. Is that the way it's supposed to be?
> 
> With curl all fields are available. Or should we ignore that?

What do you mean with "is available"? It could be that grpcurl hides boolean fields with the "false" value, this is basically just a display issue. The important thing is that isDefault is really set to false.
anatheka: > > @oxisto @lebogg after an UpdateMetricConfiguration() the is_default field is no longer available but the timestamp field is when you call GetMetricConfiguration() via grpcurl. Is that the way it's supposed to be?
> > With curl all fields are available. Or should we ignore that?
> 
> What do you mean with "is available"? It could be that grpcurl hides boolean fields with the "false" value, this is basically just a display issue. The important thing is that isDefault is really set to false.

Ah, I found it. The JSON tag includes an omitempty qualifier, so it is not shown. Thanks! 

Patch:
diff --git a/service/orchestrator/metrics.go b/service/orchestrator/metrics.go
index 43317cf9..b627f4ce 100644
--- a/service/orchestrator/metrics.go
+++ b/service/orchestrator/metrics.go
@@ -220,6 +220,8 @@ func (svc *Service) UpdateMetric(_ context.Context, req *orchestrator.UpdateMetr
 		return nil, err
 	}
 
+	metric = new(assessment.Metric)
+
 	// Check, if metric exists according to req.Metric.Id
 	err = svc.storage.Get(&metric, "id = ?", req.Metric.Id)
 	if errors.Is(err, persistence.ErrRecordNotFound) {
@@ -381,21 +383,14 @@ func (svc *Service) UpdateMetricConfiguration(ctx context.Context, req *orchestr
 		return nil, err
 	}
 
-	// Validate request configuration
-	err = req.Configuration.Validate()
-	if err != nil {
-		return nil, status.Errorf(codes.InvalidArgument, "%s", err)
-	}
-
 	// Check, if this request has access to the cloud service according to our authorization strategy.
 	if !svc.authz.CheckAccess(ctx, service.AccessRead, req) {
 		return nil, service.ErrPermissionDenied
 	}
 
-	// Make sure that the configuration also has metric/service ID and updated at set
-	req.Configuration.CloudServiceId = req.CloudServiceId
-	req.Configuration.MetricId = req.MetricId
+	// Make sure that the configuration also has updatedAt and isDefault set
 	req.Configuration.UpdatedAt = timestamppb.Now()
+	req.Configuration.IsDefault = false
 
 	err = svc.storage.Save(&req.Configuration)
 	if err != nil && errors.Is(err, persistence.ErrConstraintFailed) {


Test patch:
diff --git a/internal/testdata/testdata.go b/internal/testdata/testdata.go
index 6ba2143f..4d8cce6a 100644
--- a/internal/testdata/testdata.go
+++ b/internal/testdata/testdata.go
@@ -52,6 +52,7 @@ const (
 	MockMetricID1          = "Mock Metric 1"
 	MockMetricName1        = "Mock Metric Name"
 	MockMetricDescription1 = "This is a mock metric"
+	MockMetricCategory1    = "Mock Category 1"
 	MockMetricID2          = "Another Mock Metric"
 
 	// Assessment result
diff --git a/service/orchestrator/metrics_test.go b/service/orchestrator/metrics_test.go
index 1c4d1fcb..7c7068ba 100644
--- a/service/orchestrator/metrics_test.go
+++ b/service/orchestrator/metrics_test.go
@@ -33,12 +33,7 @@ import (
 	"testing"
 	"time"
 
-	"github.com/stretchr/testify/assert"
-	"google.golang.org/grpc/codes"
-	"google.golang.org/grpc/status"
-	"google.golang.org/protobuf/proto"
-	"google.golang.org/protobuf/types/known/structpb"
-
+	"clouditor.io/clouditor/api"
 	"clouditor.io/clouditor/api/assessment"
 	"clouditor.io/clouditor/api/orchestrator"
 	"clouditor.io/clouditor/internal/testdata"
@@ -47,9 +42,41 @@ import (
 	"clouditor.io/clouditor/persistence"
 	"clouditor.io/clouditor/persistence/gorm"
 	"clouditor.io/clouditor/service"
+
+	"github.com/stretchr/testify/assert"
+	"google.golang.org/grpc/codes"
+	"google.golang.org/grpc/status"
+	"google.golang.org/protobuf/proto"
+	"google.golang.org/protobuf/types/known/structpb"
 )
 
-var ErrSomeError = errors.New("some error")
+var (
+	ErrSomeError = errors.New("some error")
+
+	// We need to define the following vars here because we could get import cycle errors in ./internal/testdata/testdata.go
+	MockMetricConfiguration1 = &assessment.MetricConfiguration{
+		MetricId:       testdata.MockMetricID1,
+		CloudServiceId: testdata.MockCloudServiceID1,
+		Operator:       "==",
+		TargetValue:    testdata.MockMetricConfigurationTargetValueString,
+	}
+
+	MockMetricRange1 = &assessment.Range{
+		Range: &assessment.Range_AllowedValues{
+			AllowedValues: &assessment.AllowedValues{
+				Values: []*structpb.Value{
+					structpb.NewBoolValue(false),
+					structpb.NewBoolValue(true),
+				}}}}
+
+	MockMetric1 = &assessment.Metric{
+		Id:          testdata.MockMetricID1,
+		Name:        testdata.MockMetricName1,
+		Description: testdata.MockMetricDescription1,
+		Scale:       assessment.Metric_ORDINAL,
+		Range:       MockMetricRange1,
+	}
+)
 
 func TestService_loadMetrics(t *testing.T) {
 	type fields struct {
@@ -373,6 +400,46 @@ func TestService_UpdateMetric(t *testing.T) {
 			},
 			wantErr: assert.NoError,
 		},
+		{
+			name: "storage error: Get",
+			fields: fields{
+				storage: &testutil.StorageWithError{GetErr: ErrSomeError},
+			},
+			args: args{
+				context.TODO(),
+				&orchestrator.UpdateMetricRequest{
+					Metric: &assessment.Metric{
+						Id:    "TransportEncryptionEnabled",
+						Name:  "A slightly updated metric",
+						Scale: assessment.Metric_NOMINAL,
+						Range: &assessment.Range{Range: &assessment.Range_AllowedValues{}},
+					},
+				},
+			},
+			wantMetric: nil,
+			wantErr:    wantStatusCode(codes.Internal),
+		},
+		{
+			name: "storage error: Save",
+			fields: fields{
+				storage: &testutil.StorageWithError{SaveErr: ErrSomeError},
+			},
+			args: args{
+				context.TODO(),
+				&orchestrator.UpdateMetricRequest{
+					Metric: &assessment.Metric{
+						Id:          "TransportEncryptionEnabled",
+						Name:        "TransportEncryptionEnabled",
+						Description: testdata.MockMetricDescription1,
+						Category:    testdata.MockMetricCategory1,
+						Scale:       assessment.Metric_NOMINAL,
+						Range:       &assessment.Range{Range: &assessment.Range_AllowedValues{}},
+					},
+				},
+			},
+			wantMetric: nil,
+			wantErr:    wantStatusCode(codes.Internal),
+		},
 		{
 			name: "Update non-existing metric",
 			fields: fields{
@@ -441,8 +508,32 @@ func TestService_GetMetric(t *testing.T) {
 		fields     fields
 		args       args
 		wantMetric *assessment.Metric
-		wantErr    bool
+		wantErr    assert.ErrorAssertionFunc
 	}{
+		{
+			name:       "Invalid request",
+			args:       args{},
+			wantMetric: nil,
+			wantErr: func(tt assert.TestingT, err error, i ...interface{}) bool {
+				return assert.ErrorContains(t, err, api.ErrEmptyRequest.Error())
+			},
+		},
+		{
+			name: "storage error",
+			fields: fields{
+				storage: &testutil.StorageWithError{GetErr: ErrSomeError},
+			},
+			args: args{
+				context.TODO(),
+				&orchestrator.GetMetricRequest{
+					MetricId: "TransportEncryptionEnabled",
+				},
+			},
+			wantMetric: nil,
+			wantErr: func(tt assert.TestingT, err error, i ...interface{}) bool {
+				return assert.ErrorContains(t, err, ErrSomeError.Error())
+			},
+		},
 		{
 			name: "Get existing metric",
 			fields: fields{
@@ -479,7 +570,7 @@ func TestService_GetMetric(t *testing.T) {
 							structpb.NewBoolValue(true),
 						}}}},
 			},
-			wantErr: false,
+			wantErr: assert.NoError,
 		},
 		{
 			name:   "Get non-existing metric",
@@ -491,7 +582,9 @@ func TestService_GetMetric(t *testing.T) {
 				},
 			},
 			wantMetric: nil,
-			wantErr:    true,
+			wantErr: func(tt assert.TestingT, err error, i ...interface{}) bool {
+				return assert.ErrorContains(t, err, "metric not found")
+			},
 		},
 	}
 
@@ -503,11 +596,7 @@ func TestService_GetMetric(t *testing.T) {
 			gotMetric, err := svc.GetMetric(tt.args.in0, tt.args.req)
 
 			assert.NoError(t, gotMetric.Validate())
-
-			if (err != nil) != tt.wantErr {
-				t.Errorf("Service.GetMetric() error = %v, wantErrMessage %v", err, tt.wantErr)
-				return
-			}
+			tt.wantErr(t, err)
 			if !proto.Equal(gotMetric, tt.wantMetric) {
 				t.Errorf("Service.GetMetric() = %v, want %v", gotMetric, tt.wantMetric)
 			}
@@ -516,18 +605,77 @@ func TestService_GetMetric(t *testing.T) {
 }
 
 func TestService_ListMetrics(t *testing.T) {
-	var (
-		response *orchestrator.ListMetricsResponse
-		err      error
-	)
-	service := NewService()
-
-	response, err = service.ListMetrics(context.TODO(), &orchestrator.ListMetricsRequest{})
-
-	assert.NoError(t, err)
-	assert.NotEmpty(t, response.Metrics)
-	// TODO(anatheka): Test failes because of incorrect metrics.json file
-	// assert.NoError(t, response.Validate())
+	type fields struct {
+		cloudServiceHooks     []orchestrator.CloudServiceHookFunc
+		toeHooks              []orchestrator.TargetOfEvaluationHookFunc
+		AssessmentResultHooks []func(result *assessment.AssessmentResult, err error)
+		storage               persistence.Storage
+		metricsFile           string
+		loadMetricsFunc       func() ([]*assessment.Metric, error)
+		catalogsFolder        string
+		loadCatalogsFunc      func() ([]*orchestrator.Catalog, error)
+		events                chan *orchestrator.MetricChangeEvent
+		authz                 service.AuthorizationStrategy
+	}
+	type args struct {
+		in0 context.Context
+		req *orchestrator.ListMetricsRequest
+	}
+	tests := []struct {
+		name    string
+		fields  fields
+		args    args
+		wantRes *orchestrator.ListMetricsResponse
+		wantErr assert.ErrorAssertionFunc
+	}{
+		{
+			name: "Invalid input",
+			args: args{},
+			wantErr: func(tt assert.TestingT, err error, i ...interface{}) bool {
+				return assert.ErrorContains(t, err, api.ErrEmptyRequest.Error())
+			},
+		},
+		{
+			name: "Happy path",
+			fields: fields{
+				storage: testutil.NewInMemoryStorage(t, func(s persistence.Storage) {
+					_ = s.Create(MockMetric1)
+				}),
+			},
+			args: args{
+				req: &orchestrator.ListMetricsRequest{},
+			},
+			wantRes: &orchestrator.ListMetricsResponse{
+				Metrics: []*assessment.Metric{MockMetric1},
+			},
+			wantErr: assert.NoError,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			svc := &Service{
+				cloudServiceHooks:     tt.fields.cloudServiceHooks,
+				toeHooks:              tt.fields.toeHooks,
+				AssessmentResultHooks: tt.fields.AssessmentResultHooks,
+				storage:               tt.fields.storage,
+				metricsFile:           tt.fields.metricsFile,
+				loadMetricsFunc:       tt.fields.loadMetricsFunc,
+				catalogsFolder:        tt.fields.catalogsFolder,
+				loadCatalogsFunc:      tt.fields.loadCatalogsFunc,
+				events:                tt.fields.events,
+				authz:                 tt.fields.authz,
+			}
+			gotRes, err := svc.ListMetrics(tt.args.in0, tt.args.req)
+
+			assert.NoError(t, gotRes.Validate())
+
+			tt.wantErr(t, err)
+
+			if !reflect.DeepEqual(gotRes, tt.wantRes) {
+				t.Errorf("Service.ListMetrics() = %v, want %v", gotRes, tt.wantRes)
+			}
+		})
+	}
 }
 
 func TestService_GetMetricImplementation(t *testing.T) {
@@ -547,8 +695,13 @@ func TestService_GetMetricImplementation(t *testing.T) {
 		fields  fields
 		args    args
 		wantRes *assessment.MetricImplementation
-		wantErr bool
+		wantErr assert.ErrorAssertionFunc
 	}{
+		{
+			name:    "Invalid input",
+			wantRes: nil,
+			wantErr: wantStatusCode(codes.InvalidArgument),
+		},
 		{
 			name: "metric not found",
 			fields: fields{
@@ -559,7 +712,7 @@ func TestService_GetMetricImplementation(t *testing.T) {
 					MetricId: testdata.MockMetricID1,
 				},
 			},
-			wantErr: true,
+			wantErr: wantStatusCode(codes.NotFound),
 		},
 		{
 			name: "storage error",
@@ -571,7 +724,7 @@ func TestService_GetMetricImplementation(t *testing.T) {
 					MetricId: testdata.MockMetricID1,
 				},
 			},
-			wantErr: true,
+			wantErr: wantStatusCode(codes.Internal),
 		},
 		{
 			name: "metric found",
@@ -597,6 +750,7 @@ func TestService_GetMetricImplementation(t *testing.T) {
 				Lang:     assessment.MetricImplementation_LANGUAGE_REGO,
 				Code:     "package test",
 			},
+			wantErr: assert.NoError,
 		},
 	}
 
@@ -613,10 +767,7 @@ func TestService_GetMetricImplementation(t *testing.T) {
 			gotRes, err := svc.GetMetricImplementation(tt.args.ctx, tt.args.req)
 			assert.NoError(t, gotRes.Validate())
 
-			if (err != nil) != tt.wantErr {
-				t.Errorf("Service.GetMetricImplementation() error = %v, wantErrMessage %v", err, tt.wantErr)
-				return
-			}
+			tt.wantErr(t, err)
 			if !reflect.DeepEqual(gotRes, tt.wantRes) {
 				t.Errorf("Service.GetMetricImplementation() = %v, want %v", gotRes, tt.wantRes)
 			}
@@ -641,10 +792,10 @@ func TestService_UpdateMetricImplementation(t *testing.T) {
 		fields   fields
 		args     args
 		wantImpl assert.ValueAssertionFunc
-		wantErr  bool
+		wantErr  assert.ErrorAssertionFunc
 	}{
 		{
-			name: "metric not found",
+			name: "Invalid input",
 			fields: fields{
 				storage:     testutil.NewInMemoryStorage(t),
 				metricsFile: "metrics.json",
@@ -655,10 +806,44 @@ func TestService_UpdateMetricImplementation(t *testing.T) {
 				},
 			},
 			wantImpl: assert.Empty,
-			wantErr:  true,
+			wantErr:  wantStatusCode(codes.InvalidArgument),
 		},
 		{
-			name: "storage error",
+			name: "Metric not found",
+			fields: fields{
+				storage: testutil.NewInMemoryStorage(t),
+			},
+			args: args{
+				req: &orchestrator.UpdateMetricImplementationRequest{
+					Implementation: &assessment.MetricImplementation{
+						MetricId: "TransportEncryptionEnabled",
+						Lang:     assessment.MetricImplementation_LANGUAGE_REGO,
+						Code:     "package example",
+					},
+				},
+			},
+			wantImpl: assert.Empty,
+			wantErr:  wantStatusCode(codes.NotFound),
+		},
+		{
+			name: "storage error: Get",
+			fields: fields{
+				storage: &testutil.StorageWithError{GetErr: ErrSomeError},
+			},
+			args: args{
+				req: &orchestrator.UpdateMetricImplementationRequest{
+					Implementation: &assessment.MetricImplementation{
+						MetricId: "TransportEncryptionEnabled",
+						Lang:     assessment.MetricImplementation_LANGUAGE_REGO,
+						Code:     "package example",
+					},
+				},
+			},
+			wantImpl: assert.Empty,
+			wantErr:  wantStatusCode(codes.Internal),
+		},
+		{
+			name: "storage error: Save",
 			fields: fields{
 				storage: &testutil.StorageWithError{SaveErr: ErrSomeError},
 			},
@@ -672,7 +857,7 @@ func TestService_UpdateMetricImplementation(t *testing.T) {
 				},
 			},
 			wantImpl: assert.Empty,
-			wantErr:  true,
+			wantErr:  wantStatusCode(codes.Internal),
 		},
 		{
 			name: "update",
@@ -700,6 +885,7 @@ func TestService_UpdateMetricImplementation(t *testing.T) {
 					assert.True(t, impl.UpdatedAt.AsTime().Before(time.Now())) &&
 					assert.NoError(t, impl.Validate())
 			},
+			wantErr: assert.NoError,
 		},
 	}
 
@@ -713,11 +899,8 @@ func TestService_UpdateMetricImplementation(t *testing.T) {
 				events:                tt.fields.events,
 			}
 			gotImpl, err := svc.UpdateMetricImplementation(tt.args.in0, tt.args.req)
-			if (err != nil) != tt.wantErr {
-				t.Errorf("Service.UpdateMetricImplementation() error = %v, wantErrMessage %v", err, tt.wantErr)
-				return
-			}
 
+			tt.wantErr(t, err)
 			tt.wantImpl(t, gotImpl)
 		})
 	}
@@ -744,6 +927,47 @@ func TestService_GetMetricConfiguration(t *testing.T) {
 		want         assert.ValueAssertionFunc
 		wantErr      assert.ErrorAssertionFunc
 	}{
+		{
+			name: "Invalid input",
+			args: args{
+				req: &orchestrator.GetMetricConfigurationRequest{
+					CloudServiceId: "InvalidCloudServiceID",
+				},
+			},
+			want:    assert.Empty,
+			wantErr: wantStatusCode(codes.InvalidArgument),
+		},
+		{
+			name: "Permission denied",
+			args: args{
+				req: &orchestrator.GetMetricConfigurationRequest{
+					MetricId:       testdata.MockMetricID1,
+					CloudServiceId: testdata.MockCloudServiceID1,
+				},
+			},
+			fields: fields{
+				authz: &servicetest.AuthorizationStrategyMock{},
+			},
+			want:    assert.Empty,
+			wantErr: wantStatusCode(codes.PermissionDenied),
+		},
+		{
+			name: "storage error",
+			fields: fields{
+				authz:   &service.AuthorizationStrategyAllowAll{},
+				storage: &testutil.StorageWithError{GetErr: ErrSomeError},
+			},
+			args: args{
+				req: &orchestrator.GetMetricConfigurationRequest{
+					MetricId:       testdata.MockMetricID1,
+					CloudServiceId: testdata.MockCloudServiceID1,
+				},
+			},
+			want: assert.Empty,
+			wantErr: func(tt assert.TestingT, err error, i ...interface{}) bool {
+				return assert.ErrorContains(t, err, ErrSomeError.Error())
+			},
+		},
 		{
 			name: "metric found",
 			fields: fields{
@@ -803,14 +1027,8 @@ func TestService_GetMetricConfiguration(t *testing.T) {
 					CloudServiceId: testdata.MockCloudServiceID1,
 				},
 			},
-			want: assert.Empty,
-			wantErr: func(tt assert.TestingT, err error, i ...interface{}) bool {
-				gotStatus, ok := status.FromError(err)
-				if !ok {
-					return false
-				}
-				return assert.Equal(t, gotStatus.Code(), codes.NotFound)
-			},
+			want:    assert.Empty,
+			wantErr: wantStatusCode(codes.NotFound),
 		},
 	}
 
@@ -852,7 +1070,7 @@ func TestService_ListMetricConfigurations(t *testing.T) {
 		fields       fields
 		args         args
 		wantResponse *orchestrator.ListMetricConfigurationResponse
-		wantErr      bool
+		wantErr      assert.ErrorAssertionFunc
 	}{
 		{
 			name: "error",
@@ -863,7 +1081,34 @@ func TestService_ListMetricConfigurations(t *testing.T) {
 			args: args{
 				req: &orchestrator.ListMetricConfigurationRequest{},
 			},
-			wantErr: true,
+			wantErr: wantStatusCode(codes.InvalidArgument),
+		},
+		{
+			name: "Permission denied",
+			fields: fields{
+				authz: &servicetest.AuthorizationStrategyMock{},
+			},
+			args: args{
+				req: &orchestrator.ListMetricConfigurationRequest{
+					CloudServiceId: testdata.MockCloudServiceID1,
+				},
+			},
+			wantResponse: nil,
+			wantErr:      wantStatusCode(codes.PermissionDenied),
+		},
+		{
+			name: "storage error",
+			fields: fields{
+				storage: &testutil.StorageWithError{ListErr: ErrSomeError},
+				authz:   &service.AuthorizationStrategyAllowAll{},
+			},
+			args: args{
+				ctx: context.Background(),
+				req: &orchestrator.ListMetricConfigurationRequest{
+					CloudServiceId: testdata.MockCloudServiceID1,
+				},
+			},
+			wantErr: wantStatusCode(codes.Internal),
 		},
 		{
 			name: "no error",
@@ -873,11 +1118,7 @@ func TestService_ListMetricConfigurations(t *testing.T) {
 					_ = s.Create(&orchestrator.CloudService{
 						Id: testdata.MockCloudServiceID1,
 					})
-					_ = s.Create(&assessment.MetricConfiguration{
-						MetricId:       testdata.MockMetricID1,
-						CloudServiceId: testdata.MockCloudServiceID1,
-						Operator:       "==",
-					})
+					_ = s.Create(MockMetricConfiguration1)
 				}),
 				authz: servicetest.NewAuthorizationStrategy(true),
 			},
@@ -888,13 +1129,10 @@ func TestService_ListMetricConfigurations(t *testing.T) {
 			},
 			wantResponse: &orchestrator.ListMetricConfigurationResponse{
 				Configurations: map[string]*assessment.MetricConfiguration{
-					testdata.MockMetricID1: {
-						MetricId:       testdata.MockMetricID1,
-						CloudServiceId: testdata.MockCloudServiceID1,
-						Operator:       "==",
-					},
+					testdata.MockMetricID1: MockMetricConfiguration1,
 				},
 			},
+			wantErr: assert.NoError,
 		},
 	}
 
@@ -910,13 +1148,11 @@ func TestService_ListMetricConfigurations(t *testing.T) {
 			}
 			gotResponse, err := svc.ListMetricConfigurations(tt.args.ctx, tt.args.req)
 
-			// TODO(anatheka): Test failes because of incorrect metrics.json file
-			// assert.NoError(t, gotResponse.Validate())
+			// Check if response validation succeds
+			assert.NoError(t, gotResponse.Validate())
+
+			tt.wantErr(t, err)
 
-			if (err != nil) != tt.wantErr {
-				t.Errorf("Service.ListMetricConfigurations() error = %v, wantErrMessage %v", err, tt.wantErr)
-				return
-			}
 			if !reflect.DeepEqual(gotResponse, tt.wantResponse) {
 				t.Errorf("Service.ListMetricConfigurations() = %v, want %v", gotResponse, tt.wantResponse)
 			}
@@ -946,6 +1182,70 @@ func TestService_UpdateMetricConfiguration(t *testing.T) {
 		want    assert.ValueAssertionFunc
 		wantErr assert.ErrorAssertionFunc
 	}{
+		{
+			name: "Metric configuration invalid",
+			args: args{
+				req: &orchestrator.UpdateMetricConfigurationRequest{
+					CloudServiceId: testdata.MockCloudServiceID1,
+					MetricId:       testdata.MockMetricID1,
+					Configuration: &assessment.MetricConfiguration{
+						Operator:       "invalidOperator",
+						TargetValue:    testdata.MockMetricConfigurationTargetValueString,
+						MetricId:       testdata.MockMetricID1,
+						CloudServiceId: testdata.MockCloudServiceID1,
+					},
+				},
+			},
+			want: assert.Empty,
+			wantErr: func(tt assert.TestingT, err error, i ...interface{}) bool {
+				return assert.ErrorContains(t, err, "value does not match regex pattern")
+			},
+		},
+		{
+			name: "Permission denied",
+			fields: fields{
+				authz: &servicetest.AuthorizationStrategyMock{},
+			},
+			args: args{
+				req: &orchestrator.UpdateMetricConfigurationRequest{
+					CloudServiceId: testdata.MockCloudServiceID1,
+					MetricId:       testdata.MockMetricID1,
+					Configuration: &assessment.MetricConfiguration{
+						CloudServiceId: testdata.MockCloudServiceID1,
+						MetricId:       testdata.MockMetricID1,
+						Operator:       "<",
+						TargetValue:    testdata.MockMetricConfigurationTargetValueString,
+					},
+				},
+			},
+			want: assert.Empty,
+			wantErr: func(tt assert.TestingT, err error, i ...interface{}) bool {
+				return assert.ErrorContains(t, err, service.ErrPermissionDenied.Error())
+			},
+		},
+		{
+			name: "storage error",
+			fields: fields{
+				authz:   &service.AuthorizationStrategyAllowAll{},
+				storage: &testutil.StorageWithError{SaveErr: ErrSomeError},
+			},
+			args: args{
+				req: &orchestrator.UpdateMetricConfigurationRequest{
+					CloudServiceId: testdata.MockCloudServiceID1,
+					MetricId:       testdata.MockMetricID1,
+					Configuration: &assessment.MetricConfiguration{
+						CloudServiceId: testdata.MockCloudServiceID1,
+						MetricId:       testdata.MockMetricID1,
+						Operator:       "<",
+						TargetValue:    testdata.MockMetricConfigurationTargetValueString,
+					},
+				},
+			},
+			want: assert.Empty,
+			wantErr: func(tt assert.TestingT, err error, i ...interface{}) bool {
+				return assert.ErrorContains(t, err, ErrSomeError.Error())
+			},
+		},
 		{
 			name: "metricId is missing in request",
 			fields: fields{
@@ -1243,3 +1543,13 @@ func TestService_UpdateMetricConfiguration(t *testing.T) {
 		})
 	}
 }
+
+func wantStatusCode(code codes.Code) assert.ErrorAssertionFunc {
+	return func(t assert.TestingT, err error, i ...interface{}) bool {
+		gotStatus, ok := status.FromError(err)
+		if !ok {
+			return false
+		}
+		return assert.Equal(t, gotStatus.Code(), code)
+	}
+}