o-nikolas commented on code in PR #56578:
URL: https://github.com/apache/airflow/pull/56578#discussion_r2434042574
##########
providers/cncf/kubernetes/tests/unit/cncf/kubernetes/hooks/test_kubernetes.py:
##########
@@ -760,9 +760,11 @@ def test_check_kueue_deployment_running(self,
mock_get_deployment_status, mock_l
@mock.patch(HOOK_MODULE + ".KubernetesHook.log")
@mock.patch(HOOK_MODULE + ".KubernetesHook.get_deployment_status")
def test_check_kueue_deployment_raise_exception(self,
mock_get_deployment_status, mock_log):
- mock_get_deployment_status.side_effect = ValueError
+ mock_get_deployment_status.side_effect = ValueError(
+ "Exception occurred while checking for Deployment status."
+ )
- with pytest.raises(ValueError):
+ with pytest.raises(ValueError, match="Exception occurred while
checking for Deployment status."):
Review Comment:
I agree that making up an error message is not helpful. I would do either:
1. If the error message is truly empty in the production usecase, then
assert the message is indeed empty string. What is wrong with this?
2. If for some reason we want to have a message, then I would update the src
code to move what is now being included in a `self.log.exception` message into
the exception itself. Generally log and raise is a bad practice anyway and this
would move the message into the exception and you'd have something to test
against in the unit test. But I still prefer option 1, slightly.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
