Repository: clouditor/clouditor
Pull request #908 (2022-12-20 09:49:27)

Refactor StoreAssessmentResult response
Based on the nested methods StoreAssessmentResult and StoreAssessmentResults the response message is not correct for the StoreAssessmentResult method. We have to create the response message only in the StoreAssessmentResults method because we use a gRPC response stream there. If no gRPC response stream is used, only an error message is returned.

codecov[bot]: # [Codecov](https://codecov.io/gh/clouditor/clouditor/pull/908?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=clouditor) Report
> Merging [#908](https://codecov.io/gh/clouditor/clouditor/pull/908?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=clouditor) (ced3480) into [main](https://codecov.io/gh/clouditor/clouditor/commit/9a8097a5d4054e1484d079a0d49a6b09aafe7283?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=clouditor) (9a8097a) will **not change** coverage.
> The diff coverage is `100.00%`.

```diff
@@           Coverage Diff           @@
##             main     #908   +/-   ##
=======================================
  Coverage   81.98%   81.98%           
=======================================
  Files          85       85           
  Lines        7021     7021           
=======================================
  Hits         5756     5756           
  Misses        973      973           
  Partials      292      292           
```

| Flag | Coverage Δ | |
|---|---|---|
| unittests | `81.98% <100.00%> (ø)` | |

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://codecov.io/gh/clouditor/clouditor/pull/908?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=clouditor) | Coverage Δ | |
|---|---|---|
| [service/orchestrator/orchestrator.go](https://codecov.io/gh/clouditor/clouditor/pull/908/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=clouditor#diff-c2VydmljZS9vcmNoZXN0cmF0b3Ivb3JjaGVzdHJhdG9yLmdv) | `83.72% <100.00%> (ø)` | |

Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=clouditor). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=clouditor)


Patch:
diff --git a/service/orchestrator/orchestrator.go b/service/orchestrator/orchestrator.go
index ee11ce2f..96590328 100644
--- a/service/orchestrator/orchestrator.go
+++ b/service/orchestrator/orchestrator.go
@@ -210,23 +210,15 @@ func (s *Service) StoreAssessmentResult(_ context.Context, req *orchestrator.Sto
 	// Validate request
 	err = service.ValidateRequest(req)
 	if err != nil {
-		go s.informHook(nil, err)
-
-		resp = &orchestrator.StoreAssessmentResultResponse{
-			Status:        false,
-			StatusMessage: err.Error(),
-		}
-
-		return resp, err
+		log.Error(err)
+		return nil, status.Errorf(codes.InvalidArgument, "%v", err)
 	}
 
 	s.results[req.Result.Id] = req.Result
 
 	go s.informHook(req.Result, nil)
 
-	resp = &orchestrator.StoreAssessmentResultResponse{
-		Status: true,
-	}
+	resp = &orchestrator.StoreAssessmentResultResponse{}
 
 	return resp, nil
 }
@@ -254,9 +246,17 @@ func (s *Service) StoreAssessmentResults(stream orchestrator.Orchestrator_StoreA
 		storeAssessmentResultReq := &orchestrator.StoreAssessmentResultRequest{
 			Result: result.Result,
 		}
-		res, err = s.StoreAssessmentResult(context.Background(), storeAssessmentResultReq)
+		_, err = s.StoreAssessmentResult(context.Background(), storeAssessmentResultReq)
 		if err != nil {
-			log.Errorf("Error storing assessment result: %v", err)
+			// Create response message. The StoreAssessmentResult method does not need that message, so we have to create it here for the stream response.
+			res = &orchestrator.StoreAssessmentResultResponse{
+				Status:        false,
+				StatusMessage: err.Error(),
+			}
+		} else {
+			res = &orchestrator.StoreAssessmentResultResponse{
+				Status: true,
+			}
 		}
 
 		log.Debugf("Assessment result received (%v)", result.Result.Id)


Test patch:
diff --git a/service/orchestrator/orchestrator_test.go b/service/orchestrator/orchestrator_test.go
index aa110338..505feb6a 100644
--- a/service/orchestrator/orchestrator_test.go
+++ b/service/orchestrator/orchestrator_test.go
@@ -136,10 +136,8 @@ func TestAssessmentResultHook(t *testing.T) {
 					},
 				},
 			},
-			wantErr: false,
-			wantResp: &orchestrator.StoreAssessmentResultResponse{
-				Status: true,
-			},
+			wantErr:  false,
+			wantResp: &orchestrator.StoreAssessmentResultResponse{},
 		},
 	}
 
@@ -174,7 +172,7 @@ func TestStoreAssessmentResult(t *testing.T) {
 		name     string
 		args     args
 		wantResp *orchestrator.StoreAssessmentResultResponse
-		wantErr  bool
+		wantErr  assert.ErrorAssertionFunc
 	}{
 		{
 			name: "Store assessment to the map",
@@ -201,10 +199,8 @@ func TestStoreAssessmentResult(t *testing.T) {
 					},
 				},
 			},
-			wantErr: false,
-			wantResp: &orchestrator.StoreAssessmentResultResponse{
-				Status: true,
-			},
+			wantErr:  assert.NoError,
+			wantResp: &orchestrator.StoreAssessmentResultResponse{},
 		},
 		{
 			name: "Store assessment without metricId to the map",
@@ -230,11 +226,10 @@ func TestStoreAssessmentResult(t *testing.T) {
 					},
 				},
 			},
-			wantErr: true,
-			wantResp: &orchestrator.StoreAssessmentResultResponse{
-				Status:        false,
-				StatusMessage: "rpc error: code = InvalidArgument desc = invalid request: invalid StoreAssessmentResultRequest.Result: embedded message failed validation | caused by: invalid AssessmentResult.MetricId: value length must be at least 1 runes",
+			wantErr: func(t assert.TestingT, err error, i ...interface{}) bool {
+				return assert.ErrorContains(t, err, "caused by: invalid AssessmentResult.MetricId: value length must be at least 1 runes")
 			},
+			wantResp: nil,
 		},
 	}
 
@@ -242,13 +237,8 @@ func TestStoreAssessmentResult(t *testing.T) {
 		t.Run(tt.name, func(t *testing.T) {
 			s := NewService()
 			gotResp, err := s.StoreAssessmentResult(tt.args.in0, tt.args.assessment)
-			if (err != nil) != tt.wantErr {
-				t.Errorf("StoreAssessmentResult() error = %v, wantErrMessage %v", err, tt.wantErr)
-				return
-			}
-			if !reflect.DeepEqual(gotResp, tt.wantResp) {
-				t.Errorf("StoreAssessmentResult() gotResp = %v, want %v", gotResp, tt.wantResp)
-			}
+			tt.wantErr(t, err)
+			assert.Equal(t, tt.wantResp, gotResp)
 
 			if err == nil {
 				assert.NotNil(t, s.results[assessmentResultID1])
@@ -368,7 +358,7 @@ func TestStoreAssessmentResults(t *testing.T) {
 				i := 0
 				for elem := range tt.args.streamToServer.SentFromServer {
 					assert.Contains(t, elem.StatusMessage, tt.wantRespMessage[i].StatusMessage)
-					assert.Equal(t, elem.Status, tt.wantRespMessage[i].Status)
+					assert.Equal(t, tt.wantRespMessage[i].Status, elem.Status)
 					i++
 				}
 			}