[GitHub] [airflow] kosteev commented on a diff in pull request #33500: Fix KubernetesPodOperator duplicating logs when interrupted

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

kosteev commented on code in PR #33500:
URL: https://github.com/apache/airflow/pull/33500#discussion_r1301156996


##########
tests/providers/cncf/kubernetes/utils/test_pod_manager.py:
##########
@@ -252,6 +253,26 @@ def test_parse_log_line(self):
         assert timestamp == pendulum.parse(real_timestamp)
         assert line == log_message
 
+    @mock.patch("airflow.providers.cncf.kubernetes.utils.pod_manager.PodManager.read_pod_logs")
+    def test_fetch_container_logs_returning_last_timestamp(self, mock_read_pod_logs):
+        timestamp_string = "2020-10-08T14:16:17.793417674Z"
+
+        mock_read_pod_logs.return_value = [bytes(f"{timestamp_string} message", "utf-8"), b"notimestamp"]

Review Comment:
   Nit: seems that new line above can be omit. Given that there are too many new lines in the method, I would strive to reduce them if possible to make code more logically grouped for better readability.



##########
tests/providers/cncf/kubernetes/utils/test_pod_manager.py:
##########
@@ -252,6 +253,26 @@ def test_parse_log_line(self):
         assert timestamp == pendulum.parse(real_timestamp)
         assert line == log_message
 
+    @mock.patch("airflow.providers.cncf.kubernetes.utils.pod_manager.PodManager.read_pod_logs")
+    def test_fetch_container_logs_returning_last_timestamp(self, mock_read_pod_logs):
+        timestamp_string = "2020-10-08T14:16:17.793417674Z"
+
+        mock_read_pod_logs.return_value = [bytes(f"{timestamp_string} message", "utf-8"), b"notimestamp"]
+
+        self.first_time = True
+
+        def mock_container_is_running(pod, container_name):
+            if self.first_time:
+                self.first_time = False
+                return True
+            return False
+
+        self.pod_manager.container_is_running = mock_container_is_running

Review Comment:
   This has to be done with mock.patch, otherwise it will be preserved for next tests.



##########
tests/providers/cncf/kubernetes/utils/test_pod_manager.py:
##########
@@ -252,6 +253,26 @@ def test_parse_log_line(self):
         assert timestamp == pendulum.parse(real_timestamp)
         assert line == log_message
 
+    @mock.patch("airflow.providers.cncf.kubernetes.utils.pod_manager.PodManager.read_pod_logs")
+    def test_fetch_container_logs_returning_last_timestamp(self, mock_read_pod_logs):
+        timestamp_string = "2020-10-08T14:16:17.793417674Z"
+
+        mock_read_pod_logs.return_value = [bytes(f"{timestamp_string} message", "utf-8"), b"notimestamp"]
+
+        self.first_time = True

Review Comment:
   This looks a bit clumsy to define here self variable that is used only in this test.
   I can suggest this approach:
   ```
   def mock_container_is_running(...):
       if mock_container_is_running.first_time:
           ...
       ...
   
   mock_container_is_running.first_time = True
   ```



-- 
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: commits-unsubscribe@airflow.apache.org

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