From b4f492d45547258abc7d1084b64711b4af8f3f84 Mon Sep 17 00:00:00 2001
From: Amador Pahim <apahim@redhat.com>
Date: Fri, 1 Apr 2016 13:53:11 -0300
Subject: [PATCH] avocado.core.loader fix crash on loader exception

When an invalid python code is present in a test class, avocado crashes
badly. This patch adds a TestLoaderError class to be used in those cases,
so we use it to fake the invalid test class, fail the test with ERROR estatus
and report the exception in job log.

Reference: https://trello.com/c/3zDIjTuY
Signed-off-by: Amador Pahim <apahim@redhat.com>
---
 avocado/core/loader.py             | 11 +++++++----
 avocado/core/runner.py             | 27 +++++++--------------------
 avocado/core/test.py               | 14 ++++++++++++++
 selftests/functional/test_basic.py | 24 ++++++++++++++++++++++++
 4 files changed, 52 insertions(+), 24 deletions(-)

diff --git a/avocado/core/loader.py b/avocado/core/loader.py
index 962b7007..e0a290a6 100644
--- a/avocado/core/loader.py
+++ b/avocado/core/loader.py
@@ -244,10 +244,13 @@ class TestLoaderProxy(object):
                 sys.path.insert(0, test_module_dir)
                 f, p, d = imp.find_module(module_name, [test_module_dir])
                 test_module = imp.load_module(module_name, f, p, d)
-            except ImportError as details:
-                raise ImportError("Unable to import test's module with "
-                                  "sys.path=%s\n\n%s" % (", ".join(sys.path),
-                                                         details))
+            except:
+                # On load_module exception we fake the test class and pass
+                # the exc_info as parameter to be logged.
+                test_parameters['methodName'] = 'test'
+                exception = stacktrace.prepare_exc_info(sys.exc_info())
+                test_parameters['exception'] = exception
+                return test.TestError(**test_parameters)
             finally:
                 if test_module_dir in sys.path:
                     sys.path.remove(test_module_dir)
diff --git a/avocado/core/runner.py b/avocado/core/runner.py
index ccb37493..298b28b1 100644
--- a/avocado/core/runner.py
+++ b/avocado/core/runner.py
@@ -91,11 +91,6 @@ class TestStatus(object):
                     for _ in queue:     # Return all unprocessed messages back
                         self.queue.put(_)
                     return msg
-                elif "load_exception" in msg:
-                    raise exceptions.TestError("Avocado crashed during test "
-                                               "load. Some reports might have "
-                                               "not been generated. "
-                                               "Aborting...")
                 else:   # Not an early_status message
                     queue.append(msg)
 
@@ -234,21 +229,13 @@ class TestRunner(object):
         sys.stdout = output.LoggingFile(logger=logger_list_stdout)
         sys.stderr = output.LoggingFile(logger=logger_list_stderr)
 
-        try:
-            instance = loader.load_test(test_factory)
-            if instance.runner_queue is None:
-                instance.runner_queue = queue
-            runtime.CURRENT_TEST = instance
-            early_state = instance.get_state()
-            early_state['early_status'] = True
-            queue.put(early_state)
-        except Exception:
-            exc_info = sys.exc_info()
-            app_logger = logging.getLogger('avocado.app')
-            app_logger.exception('Exception loading test')
-            tb_info = stacktrace.tb_info(exc_info)
-            queue.put({'load_exception': tb_info})
-            return
+        instance = loader.load_test(test_factory)
+        if instance.runner_queue is None:
+            instance.runner_queue = queue
+        runtime.CURRENT_TEST = instance
+        early_state = instance.get_state()
+        early_state['early_status'] = True
+        queue.put(early_state)
 
         def timeout_handler(signum, frame):
             e_msg = "Timeout reached waiting for %s to end" % instance
diff --git a/avocado/core/test.py b/avocado/core/test.py
index d2850779..b3cfe316 100644
--- a/avocado/core/test.py
+++ b/avocado/core/test.py
@@ -786,3 +786,17 @@ class ReplaySkipTest(SkipTest):
     """
 
     _skip_reason = "Test skipped due to a job replay filter!"
+
+
+class TestError(Test):
+    """
+    Generic test error.
+    """
+
+    def __init__(self, *args, **kwargs):
+        exception = kwargs.pop('exception')
+        Test.__init__(self, *args, **kwargs)
+        self.exception = exception
+
+    def test(self):
+        self.error(self.exception)
diff --git a/selftests/functional/test_basic.py b/selftests/functional/test_basic.py
index 6d139a06..36e8c4f1 100644
--- a/selftests/functional/test_basic.py
+++ b/selftests/functional/test_basic.py
@@ -62,6 +62,17 @@ class FakeStatusTest(Test):
         pass
 '''
 
+INVALID_PYTHON_TEST = '''
+from avocado import Test
+
+class MyTest(Test):
+
+    non_existing_variable_causing_crash
+
+    def test_my_name(self):
+        pass
+'''
+
 
 class RunnerOperationTest(unittest.TestCase):
 
@@ -363,6 +374,19 @@ class RunnerOperationTest(unittest.TestCase):
                      "/foo:bar ==> b", "/foo:baz ==> c", "/bar:bar ==> bar"):
             self.assertEqual(log.count(line), 3)
 
+    def test_invalid_python(self):
+        os.chdir(basedir)
+        test = script.make_script(os.path.join(self.tmpdir, 'test.py'),
+                                  INVALID_PYTHON_TEST)
+        cmd_line = './scripts/avocado --show test run --sysinfo=off '\
+                   '--job-results-dir %s %s' % (self.tmpdir, test)
+        result = process.run(cmd_line, ignore_status=True)
+        expected_rc = exit_codes.AVOCADO_TESTS_FAIL
+        self.assertEqual(result.exit_status, expected_rc,
+                         "Avocado did not return rc %d:\n%s" %
+                         (expected_rc, result))
+        self.assertIn('MyTest.test_my_name -> TestError', result.stdout)
+
     def tearDown(self):
         shutil.rmtree(self.tmpdir)
 
-- 
GitLab