Merge pull request #217 from PyFixate/fix-logging-encoding-errors

Fix encoding errors in csv-writer and logger

Author: Jasper-Harvey0

docs/release-notes.rst

@@ -14,6 +14,8 @@ Improvements
 ############
 
 - fxconfig now prevents duplicate entries from being added to the config file.
+- csv-writer thread crash will now abort the test. 
+- UTF-8 encoding is now explicitly used for the csv test log and the debug log file. Improves reliability.
 
 *************
 Version 0.6.3

src/fixate/main.py

@@ -12,7 +12,6 @@
 import fixate.config
 from fixate.core.exceptions import SequenceAbort
 from fixate.core.ui import user_info_important, user_serial, user_ok
-from fixate.reporting import register_csv, unregister_csv
 from fixate.ui_cmdline import register_cmd_line, unregister_cmd_line
 import fixate.sequencer
 
@@ -314,8 +313,6 @@ def ui_run(self):
                     ]
                 except (AttributeError, KeyError):
                     pass
-            register_csv()
-            self.sequencer.status = "Running"
 
             self.sequencer.run_sequence()
             if not self.sequencer.non_interactive:
@@ -328,7 +325,6 @@ def ui_run(self):
             input(traceback.print_exc())
             raise
         finally:
-            unregister_csv()
             if serial_response == "ABORT_FORCE" or test_selector == "ABORT_FORCE":
                 return ReturnCodes.ABORTED
             if serial_number is None:
@@ -410,7 +406,7 @@ def run_main_program(test_script_path=None, main_args=None):
         args.diagnostic_log_dir.mkdir(parents=True, exist_ok=True)
 
         handler = RotateEachInstanceHandler(
-            args.diagnostic_log_dir / "fixate.log", backupCount=10
+            args.diagnostic_log_dir / "fixate.log", backupCount=10, encoding="utf-8"
         )
         handler.setFormatter(
             logging.Formatter("%(asctime)s - %(name)s - %(levelname)s - %(message)s")

src/fixate/reporting/__init__.py

@@ -1 +1 @@
-from fixate.reporting.csv import register_csv, unregister_csv
+from fixate.reporting.csv import CSVWriter

src/fixate/reporting/csv.py

@@ -115,45 +115,55 @@ class CSVWriter:
     def __init__(self):
         self.csv_queue = Queue()
         self.csv_writer = None
-        self.reporting = CsvReporting()
+
+        self.exception_in_test = False
+        self.failed = False
+        self.chk_cnt = 0
+        self.csv_path = ""
+        self.test_module = None
+        self.start_time = None
+        self.current_test = None
+        self.data = fixate.config.get_config_dict()
+        self.data.update(fixate.config.get_plugin_data("plg_csv"))
+        self.exception = None
+
+        self._topics = [
+            (self.test_start, "Test_Start"),
+            (self.test_comparison, "Check"),
+            (self.test_exception, "Test_Exception"),
+            (self.test_complete, "Test_Complete"),
+            (self.sequence_update, "Sequence_Update"),
+            (self.sequence_complete, "Sequence_Complete"),
+            (self.user_wait_start, "UI_block_start"),
+            (self.user_wait_end, "UI_block_end"),
+            (self.driver_open, "driver_open"),
+        ]
 
     def install(self):
-        self.csv_writer = ExcThread(
-            target=self._csv_write, args=(self.csv_queue,), name="csv-writer"
-        )
+        self.csv_writer = ExcThread(target=self._csv_write, name="csv-writer")
         self.csv_writer.start()
 
+        for callback, topic in self._topics:
+            pub.subscribe(callback, topic)
+
     def uninstall(self):
+        for callback, topic in self._topics:
+            pub.unsubscribe(callback, topic)
+
         if self.csv_writer:
             self.csv_queue.put(None)
             self.csv_writer.join()
         self.csv_writer = None
 
-    def _csv_write(self, cmd_q):
-        while True:
-            line = cmd_q.get()
-            if line is None:
-                break  # Command send to close csv_writer
-            try:
-                os.makedirs(os.path.dirname(self.reporting.csv_path))
-            except OSError as e:
-                pass
-            with open(self.reporting.csv_path, "a+", newline="") as f:
-                writer = csv.writer(f, quoting=csv.QUOTE_MINIMAL)
-                writer.writerow(line)
-
+    def ensure_alive(self):
+        if self.exception:
+            raise RuntimeError(
+                f"Exception in {self.csv_writer.name} thread"
+            ) from self.exception
 
-class CsvReporting:
-    def __init__(self):
-        self.exception_in_test = False
-        self.failed = False
-        self.chk_cnt = 0
-        self.csv_path = ""
-        self.test_module = None
-        self.start_time = None
-        self.current_test = None
-        self.data = fixate.config.get_config_dict()
-        self.data.update(fixate.config.get_plugin_data("plg_csv"))
+        if not self.csv_writer.is_alive():
+            # If thread has exited without throwing an exception
+            raise RuntimeError("csv-writer thread not active")
 
     def sequence_update(self, status):
         # Do Start Sequence Reporting
@@ -336,59 +346,26 @@ def extract_test_parameters(test_cls):
         keys = sorted(set(test_cls.__dict__) - set(comp.__dict__))
         return [(key, test_cls.__dict__[key]) for key in keys]
 
+    def _csv_write(self):
+        while True:
+            line = self.csv_queue.get()
+            if line is None:
+                break  # Command send to close csv_writer
+            try:
+                os.makedirs(os.path.dirname(self.csv_path))
+            except OSError as e:
+                pass
+            with open(self.csv_path, "a+", newline="", encoding="utf-8") as f:
+                writer = csv.writer(f, quoting=csv.QUOTE_MINIMAL)
+                try:
+                    writer.writerow(line)
+                except Exception as e:
+                    self.exception = e
+
     def _write_line_to_csv(self, line):
         """
         :param line:
          single line of data with each column as an element in the list
         :return:
         """
-        global writer
-        writer.csv_queue.put(line)
-        # try:
-        #     os.makedirs(self.csv_dir)
-        # except OSError:
-        #     pass
-        # with open(self.csv_path, 'a+', newline='') as f:
-        #     writer = csv.writer(f, quoting=csv.QUOTE_MINIMAL)
-        #     writer.writerow(line)
-
-
-writer = None
-
-
-def register_csv():
-    """
-    :param csv_dir: Base directory for for csv file
-    :param args: Args as parsed into the command line interface
-    :return:
-    """
-    global writer
-    writer = CSVWriter()
-    writer.install()
-    pub.subscribe(writer.reporting.test_start, "Test_Start")
-    pub.subscribe(writer.reporting.test_comparison, "Check")
-    pub.subscribe(writer.reporting.test_exception, "Test_Exception")
-    pub.subscribe(writer.reporting.test_complete, "Test_Complete")
-    pub.subscribe(writer.reporting.sequence_update, "Sequence_Update")
-    pub.subscribe(writer.reporting.sequence_complete, "Sequence_Complete")
-    pub.subscribe(writer.reporting.user_wait_start, "UI_block_start")
-    pub.subscribe(writer.reporting.user_wait_end, "UI_block_end")
-    pub.subscribe(writer.reporting.driver_open, "driver_open")
-
-
-def unregister_csv():
-    """
-    Note, will disable the final result eg. Unit Passed
-    :return:
-    """
-    global writer
-    if writer is not None:
-        pub.unsubscribe(writer.reporting.test_start, "Test_Start")
-        pub.unsubscribe(writer.reporting.test_comparison, "Check")
-        pub.unsubscribe(writer.reporting.test_exception, "Test_Exception")
-        pub.unsubscribe(writer.reporting.test_complete, "Test_Complete")
-        pub.unsubscribe(writer.reporting.sequence_update, "Sequence_Update")
-        pub.unsubscribe(writer.reporting.sequence_complete, "Sequence_Complete")
-        pub.unsubscribe(writer.reporting.user_wait_start, "UI_block_start")
-        pub.unsubscribe(writer.reporting.user_wait_end, "UI_block_end")
-        writer.uninstall()
+        self.csv_queue.put(line)

src/fixate/sequencer.py

@@ -6,6 +6,7 @@
 from fixate.core.exceptions import SequenceAbort, CheckFail
 from fixate.core.ui import user_retry_abort_fail
 from fixate.core.checks import CheckResult
+from fixate.reporting import CSVWriter
 
 STATUS_STATES = ["Idle", "Running", "Paused", "Finished", "Restart", "Aborted"]
 
@@ -109,6 +110,7 @@ def __init__(self):
         self.context = ContextStack()
         self.context_data = {}
         self.end_status = "N/A"
+        self.reporting_service = CSVWriter()
 
         # Sequencer behaviour. Don't ask the user when things to wrong, just marks tests as failed.
         # This does not change the behaviour of tests that call out to the user. They will still block as required.
@@ -215,6 +217,7 @@ def run_sequence(self):
         Runs the sequence from the beginning to end once
         :return:
         """
+        self.reporting_service.install()
         self.status = "Running"
         try:
             self.run_once()
@@ -225,13 +228,28 @@ def run_sequence(self):
                     top.current().exit()
                 self.context.pop()
 
+        self.reporting_service.uninstall()
+
     def run_once(self):
         """
         Runs through the tests once as are pushed onto the context stack.
         Ie. One run through of the tests
         Once finished sets the status to Finished
         """
         while self.context:
+            try:
+                self.reporting_service.ensure_alive()
+            except Exception as e:
+                # We cannot log to file. Abort testing and exit
+                pub.sendMessage(
+                    "Test_Exception",
+                    exception=e,
+                    test_index=self.levels(),
+                )
+                pub.sendMessage("Sequence_Abort", exception=e)
+                self._handle_sequence_abort()
+                return
+
             if self.status == "Running":
                 try:
                     top = self.context.top()
@@ -373,7 +391,8 @@ def retry_prompt(self):
         """Prompt the user when something goes wrong.
 
         For retry return True, to fail return False and to abort raise and abort exception. Respect the
-        non_interactive flag, which can be set by the command line option --non-interactive"""
+        non_interactive flag, which can be set by the command line option --non-interactive
+        """
 
         if self.non_interactive:
             return False