Repository: DakaraProject/dakara-base

Issue:
Unknown exceptions are not displayed when progress bar is used
It looks like when an unknown exception is caught, the stderr wrapper is still active and the logs are hidden.

Discussion:

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 64cf9a6..4c45890 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -35,6 +35,14 @@
 - You can now specify custom log format and log level in `dakara_base.config.create_logger` with arguments `custom_log_format` and `custom_log_level`.
 - Access to and create user-level stored Dakara config files with `dakara_base.config.get_config_file` and `dakara_base.config.create_config_file`.
 
+### Changed
+
+- `progress_bar`: the progress bar now displays percentage.
+
+### Fixed
+
+- `progress_bar`: When an exception was raised within a progress bar, it would prevent to stop the capture of stderr, leading to hide any further log entry.
+
 ## 1.1.0 - 2019-09-16
 
 ### Added
diff --git a/src/dakara_base/progress_bar.py b/src/dakara_base/progress_bar.py
index 178f716..dddb71f 100644
--- a/src/dakara_base/progress_bar.py
+++ b/src/dakara_base/progress_bar.py
@@ -66,16 +66,18 @@ class ShrinkableTextWidget(WidgetBase):
         return text.ljust(width)
 
 
-def progress_bar(*args, text=None, **kwargs):
-    """Gives the default un-muted progress bar for the project
+def progress_bar(iterator, *args, text=None, **kwargs):
+    """Generator that gives the default un-muted progress bar for the project
 
-    It prints an optionnal shrinkable text, a timer, a progress bar and an ETA.
+    It prints an optionnal shrinkable text (if a text is provided), a
+    percentage progress, a progress bar and an adaptative ETA.
 
     Args:
+        iterator (iterator): iterator of items to use the bar with.
         text (str): text to display describing the current operation.
 
     Returns:
-        generator: progress bar.
+        generator object: item handled by the progress bar.
     """
     widgets = []
 
@@ -84,27 +86,41 @@ def progress_bar(*args, text=None, **kwargs):
         widgets.extend([ShrinkableTextWidget(text), " "])
 
     # add other widgets
-    widgets.extend([progressbar.Timer(), progressbar.Bar(), progressbar.ETA()])
+    widgets.extend(
+        [
+            progressbar.Percentage(),
+            " ",
+            progressbar.Bar(),
+            " ",
+            progressbar.Timer(),
+            " ",
+            progressbar.AdaptiveETA(),
+        ]
+    )
 
     # create progress bar
-    return progressbar.progressbar(*args, widgets=widgets, **kwargs)
+    with progressbar.ProgressBar(*args, widgets=widgets, **kwargs) as progress:
+        for item in progress(iterator):
+            yield item
 
 
-def null_bar(*args, text=None, **kwargs):
-    """Gives the defaylt muted progress bar for the project
+def null_bar(iterator, *args, text=None, **kwargs):
+    """Generator that gives the defaylt muted progress bar for the project
 
     It only logs the optionnal text.
 
     Args:
+        iterator (iterator): iterator of items to use the bar with.
         text (str): text to log describing the current operation.
 
     Returns:
-        progressbar.bar.NullBar: null progress bar that does not do anything.
+        generator object: item handled by the progress bar.
     """
     # log text immediately
     if text:
         logger.info(text)
 
     # create null progress bar
-    bar = progressbar.NullBar()
-    return bar(*args, **kwargs)
+    with progressbar.NullBar(*args, **kwargs) as progress:
+        for item in progress(iterator):
+            yield item


Test patch:
diff --git a/tests/test_progress_bar.py b/tests/test_progress_bar.py
index 3d1acee..3e8d032 100644
--- a/tests/test_progress_bar.py
+++ b/tests/test_progress_bar.py
@@ -1,7 +1,11 @@
 import logging
+import sys
+from contextlib import contextmanager
 from io import StringIO
 from unittest import TestCase
-from unittest.mock import MagicMock
+from unittest.mock import MagicMock, patch
+
+import progressbar
 
 from dakara_base import progress_bar
 
@@ -85,12 +89,13 @@ class ProgressBarTestCase(TestCase):
             lines = self.get_lines(file)
 
         # assert the lines
-        self.assertEqual(len(lines), 2)
+        self.assertEqual(len(lines), 3)
         self.assertEqual(
             lines,
             [
-                "some text here   Elapsed Time: 0:00:00|           |ETA:  --:--:--",
-                "some text here   Elapsed Time: 0:00:00|###########|Time:  0:00:00",
+                "some text here   N/A% |    | Elapsed Time: 0:00:00 ETA:  --:--:--",
+                "some text here   100% |####| Elapsed Time: 0:00:00 Time:  0:00:00",
+                "some text here   100% |####| Elapsed Time: 0:00:00 Time:  0:00:00",
             ],
         )
 
@@ -105,15 +110,109 @@ class ProgressBarTestCase(TestCase):
             lines = self.get_lines(file)
 
         # assert the lines
-        self.assertEqual(len(lines), 2)
+        self.assertEqual(len(lines), 3)
         self.assertEqual(
             lines,
             [
-                "Elapsed Time: 0:00:00|                            |ETA:  --:--:--",
-                "Elapsed Time: 0:00:00|############################|Time:  0:00:00",
+                "N/A% |                     | Elapsed Time: 0:00:00 ETA:  --:--:--",
+                "100% |#####################| Elapsed Time: 0:00:00 Time:  0:00:00",
+                "100% |#####################| Elapsed Time: 0:00:00 Time:  0:00:00",
             ],
         )
 
+    def test_stderr_on_no_exception(self):
+        """Test to check stderr is not captured if no exceptions occur
+        """
+        stderr = StringIO()
+        initial_stderr = sys.stderr
+
+        # we patch `sys.stderr` as it is risky to work directly on it, and we
+        # patch `progressbar.streams.original_stderr` as it is defined at
+        # module loading
+        with patch("sys.stderr", stderr), patch(
+            "progressbar.streams.original_stderr", stderr
+        ), wrap_stderr_progressbar():
+            wrapped_stderr = sys.stderr
+
+            # execute the progressbar without exception
+            with StringIO() as file:
+                for _ in progress_bar.progress_bar(range(1), fd=file, term_width=65):
+                    pass
+
+            sys.stderr.write("error")
+            after_stderr = sys.stderr
+
+        final_stderr = sys.stderr
+
+        # assert stderrs
+        self.assertIs(initial_stderr, final_stderr)
+        self.assertIs(wrapped_stderr, after_stderr)
+        self.assertIsNot(initial_stderr, wrapped_stderr)
+        self.assertIsNot(stderr, wrapped_stderr)
+        self.assertIsNot(stderr, initial_stderr)
+
+        # assert stderr value
+        value = stderr.getvalue()
+        self.assertEqual(value, "error")
+
+    def test_no_stderr_on_exception(self):
+        """Test to check stderr does not remain captured after an exception
+
+        When leaving a progress bar by an exception, it does not call
+        `progressbar.streams.stop_capturing` and the stderr is allways
+        captured.
+        """
+
+        class MyException(Exception):
+            pass
+
+        stderr = StringIO()
+        initial_stderr = sys.stderr
+
+        with patch("sys.stderr", stderr), patch(
+            "progressbar.streams.original_stderr", stderr
+        ), wrap_stderr_progressbar():
+            wrapped_stderr = sys.stderr
+
+            # execute the progressbar with an exception
+            with StringIO() as file:
+                try:
+                    for _ in progress_bar.progress_bar(
+                        range(1), fd=file, term_width=65
+                    ):
+                        raise MyException("error")
+
+                except MyException:
+                    pass
+
+            sys.stderr.write("error")
+            after_stderr = sys.stderr
+
+        final_stderr = sys.stderr
+
+        # assert stderrs
+        self.assertIs(initial_stderr, final_stderr)
+        self.assertIs(wrapped_stderr, after_stderr)
+        self.assertIsNot(initial_stderr, wrapped_stderr)
+        self.assertIsNot(stderr, wrapped_stderr)
+        self.assertIsNot(stderr, initial_stderr)
+
+        # assert stderr value
+        value = stderr.getvalue()
+        self.assertEqual(value, "error")
+
+
+@contextmanager
+def wrap_stderr_progressbar():
+    """Temporary wrap stderr with progressbar tools
+    """
+    try:
+        progressbar.streams.wrap_stderr()
+        yield None
+
+    finally:
+        progressbar.streams.unwrap_stderr()
+
 
 class NullBarTestCase(TestCase):
     """Test the null progress bar