[GitHub] [yunikorn-k8shim] pbacsko commented on pull request #613: [YUNIKORN-1575] [shim] Ability to specify guaranteedResources via namespace annotations

["pbacsko (via GitHub)" <gi...@apache.org>]

pbacsko commented on PR #613:
URL: https://github.com/apache/yunikorn-k8shim/pull/613#issuecomment-1593456106

   > @pbacsko One more question, how can we add the unit test to cover the log, for example, the log here, how we can improve the unit test to cover this, using log hook? Thanks!
   > 
   > ```
   > 	if err != nil {
   > 		log.Logger().Warn("Unable to process namespace.guaranteed annotation",
   > 			zap.String("namespace", namespaceObj.Name),
   > 			zap.String("namespace.guaranteed is", namespaceGuaranteed))
   > 		return nil
   > 	}
   > ```
   
   I think it's an overkill to capture log output. If you can check some side effect (return value, mock state) then do it, otherwise it's fine to skip. There are paths in the code where it's difficult to simulate an error condition, in those cases, we don't have coverage.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org