[PATCH 0/4] toaster: Improve display of errors and Toaster exceptions

[PATCH 0/4] toaster: Improve display of errors and Toaster exceptions

[Ed Bartosh]

Hi,

Fixes recording of error messages so that CRITICAL errors are
properly logged and presented to the user.

Modifies how Toaster exceptions are displayed, so they are treated the
same way as other classes of ERROR.

The following changes since commit a8e82d93bfa8feb722912d03ee26bbad7e6fe9f7:

  toaster: Modify "New build" button behaviour for cli builds project (2015-10-15 14:54:27 +0300)

are available in the git repository at:

  git://git.yoctoproject.org/poky-contrib ed/toaster/submit/elliot/toaster/error_display-8320
  http://git.yoctoproject.org/cgit.cgi/poky-contrib/log/?h=ed/toaster/submit/elliot/toaster/error_display-8320

Elliot Smith (4):
  toaster: Record critical errors
  toaster: Remove Toaster exceptions section of build dashboard
  toaster: Guard against builds with no targets
  toaster: Add tests for error message display on the build dashboard

 lib/bb/ui/buildinfohelper.py                       |  5 +-
 lib/bb/ui/toasterui.py                             |  2 +
 lib/toaster/orm/models.py                          | 24 +++---
 .../toastergui/templates/builddashboard.html       | 47 ++---------
 lib/toaster/toastergui/tests.py                    | 91 +++++++++++++++++++++-
 5 files changed, 116 insertions(+), 53 deletions(-)

--
Regards,
Ed

[PATCH 1/4] toaster: Record critical errors

[Ed Bartosh]

From: Elliot Smith <elliot.smith@intel.com>

Critical errors (where a build failed for reasons of
misconfiguration, such as a machine being specified which is not
in a project's layers) were being ignored (only log records
up to ERROR level were being logged to Toaster's db). This meant that
the build would fail but would not correctly report why.

Add support for CRITICAL error levels to the LogMessage model,
include errors at this level in the errors property for a build,
and show errors at this level in the build dashboard.

[YOCTO #8320]

Signed-off-by: Elliot Smith <elliot.smith@intel.com>
Signed-off-by: Ed Bartosh <ed.bartosh@linux.intel.com>
---
 lib/bb/ui/buildinfohelper.py                         |  5 ++++-
 lib/bb/ui/toasterui.py                               |  2 ++
 lib/toaster/orm/models.py                            | 20 +++++++++++++-------
 lib/toaster/toastergui/templates/builddashboard.html | 11 ++++++-----
 4 files changed, 25 insertions(+), 13 deletions(-)

diff --git a/lib/bb/ui/buildinfohelper.py b/lib/bb/ui/buildinfohelper.py
index a8c884d..286fb6a 100644
--- a/lib/bb/ui/buildinfohelper.py
+++ b/lib/bb/ui/buildinfohelper.py
@@ -1394,7 +1394,9 @@ class BuildInfoHelper(object):
 
         log_information = {}
         log_information['build'] = self.internal_state['build']
-        if event.levelno == formatter.ERROR:
+        if event.levelno == formatter.CRITICAL:
+            log_information['level'] = LogMessage.CRITICAL
+        elif event.levelno == formatter.ERROR:
             log_information['level'] = LogMessage.ERROR
         elif event.levelno == formatter.WARNING:
             log_information['level'] = LogMessage.WARNING
@@ -1407,6 +1409,7 @@ class BuildInfoHelper(object):
         log_information['pathname'] = event.pathname
         log_information['lineno'] = event.lineno
         logger.info("Logging error 2: %s", log_information)
+
         self.orm_wrapper.create_logmessage(log_information)
 
     def close(self, errorcode):
diff --git a/lib/bb/ui/toasterui.py b/lib/bb/ui/toasterui.py
index dbe0d09..0ed774e 100644
--- a/lib/bb/ui/toasterui.py
+++ b/lib/bb/ui/toasterui.py
@@ -145,10 +145,12 @@ def main(server, eventHandler, params ):
                     event.levelno = formatter.ERROR
 
                 buildinfohelper.store_log_event(event)
+
                 if event.levelno >= formatter.ERROR:
                     errors = errors + 1
                 elif event.levelno == formatter.WARNING:
                     warnings = warnings + 1
+
                 # For "normal" logging conditions, don't show note logs from tasks
                 # but do show them if the user has changed the default log level to
                 # include verbose/debug messages
diff --git a/lib/toaster/orm/models.py b/lib/toaster/orm/models.py
index 44a453a..6ca45e0 100644
--- a/lib/toaster/orm/models.py
+++ b/lib/toaster/orm/models.py
@@ -353,7 +353,9 @@ class Build(models.Model):
 
     @property
     def errors(self):
-        return (self.logmessage_set.filter(level=LogMessage.ERROR)|self.logmessage_set.filter(level=LogMessage.EXCEPTION))
+        return (self.logmessage_set.filter(level=LogMessage.ERROR) |
+                self.logmessage_set.filter(level=LogMessage.EXCEPTION) |
+                self.logmessage_set.filter(level=LogMessage.CRITICAL))
 
     @property
     def warnings(self):
@@ -1285,16 +1287,20 @@ class LogMessage(models.Model):
     INFO = 0
     WARNING = 1
     ERROR = 2
-
-    LOG_LEVEL = ( (INFO, "info"),
-            (WARNING, "warn"),
-            (ERROR, "error"),
-            (EXCEPTION, "toaster exception"))
+    CRITICAL = 3
+
+    LOG_LEVEL = (
+        (INFO, "info"),
+        (WARNING, "warn"),
+        (ERROR, "error"),
+        (CRITICAL, "critical"),
+        (EXCEPTION, "toaster exception")
+    )
 
     build = models.ForeignKey(Build)
     task  = models.ForeignKey(Task, blank = True, null=True)
     level = models.IntegerField(choices=LOG_LEVEL, default=INFO)
-    message=models.CharField(max_length=240)
+    message = models.CharField(max_length=240)
     pathname = models.FilePathField(max_length=255, blank=True)
     lineno = models.IntegerField(null=True)
 
diff --git a/lib/toaster/toastergui/templates/builddashboard.html b/lib/toaster/toastergui/templates/builddashboard.html
index aa99134..fc6cae6 100644
--- a/lib/toaster/toastergui/templates/builddashboard.html
+++ b/lib/toaster/toastergui/templates/builddashboard.html
@@ -69,11 +69,12 @@
     <div class="accordion-body collapse in" id="collapse-errors">
       <div class="accordion-inner">
         <div class="span10">
-          {% for error in logmessages %}{% if error.level == 2 %}
-            <div class="alert alert-error">
-              <pre>{{error.message}}</pre>
-            </div>
-          {% endif %}
+          {% for error in logmessages %}
+            {% if error.level == 2 or error.level == 3 %}
+              <div class="alert alert-error">
+                <pre>{{error.message}}</pre>
+              </div>
+            {% endif %}
           {% endfor %}
         </div>
       </div>
-- 
2.1.4

[PATCH 2/4] toaster: Remove Toaster exceptions section of build dashboard

[Ed Bartosh]

From: Elliot Smith <elliot.smith@intel.com>

Fatal build errors were displayed as exceptions, and highlighted
with less severity than they deserved.

Roll back to treating Toaster exceptions as errors by removing
the toaster_exceptions member on Build objects and displaying
EXCEPTION events in the errors section on the dashboard.

[YOCTO #8320]

Signed-off-by: Elliot Smith <elliot.smith@intel.com>
Signed-off-by: Ed Bartosh <ed.bartosh@linux.intel.com>
---
 lib/toaster/orm/models.py                          |  4 --
 .../toastergui/templates/builddashboard.html       | 45 ++--------------------
 2 files changed, 4 insertions(+), 45 deletions(-)

diff --git a/lib/toaster/orm/models.py b/lib/toaster/orm/models.py
index 6ca45e0..a784b1b 100644
--- a/lib/toaster/orm/models.py
+++ b/lib/toaster/orm/models.py
@@ -348,10 +348,6 @@ class Build(models.Model):
         return Build.BUILD_OUTCOME[int(self.outcome)][1]
 
     @property
-    def toaster_exceptions(self):
-        return self.logmessage_set.filter(level=LogMessage.EXCEPTION)
-
-    @property
     def errors(self):
         return (self.logmessage_set.filter(level=LogMessage.ERROR) |
                 self.logmessage_set.filter(level=LogMessage.EXCEPTION) |
diff --git a/lib/toaster/toastergui/templates/builddashboard.html b/lib/toaster/toastergui/templates/builddashboard.html
index fc6cae6..75ca8bc 100644
--- a/lib/toaster/toastergui/templates/builddashboard.html
+++ b/lib/toaster/toastergui/templates/builddashboard.html
@@ -44,14 +44,6 @@
 
 {%endif%}
     </div>
-    {% if build.toaster_exceptions.count > 0 %}
-    <div class="row">
-        <small class="pull-right">
-        <i class="icon-question-sign get-help get-help-blue" title="" data-original-title="Toaster exceptions do not affect your build: only the operation of Toaster"></i>
-        <a class="show-exceptions" href="#exceptions">Toaster threw {{build.toaster_exceptions.count}} exception{{build.toaster_exceptions.count|pluralize}}</a>
-    </small>
-    </div>
-    {% endif %}
   </div>
 </div>
 
@@ -69,12 +61,10 @@
     <div class="accordion-body collapse in" id="collapse-errors">
       <div class="accordion-inner">
         <div class="span10">
-          {% for error in logmessages %}
-            {% if error.level == 2 or error.level == 3 %}
-              <div class="alert alert-error">
-                <pre>{{error.message}}</pre>
-              </div>
-            {% endif %}
+          {% for error in build.errors %}
+            <div class="alert alert-error">
+              <pre>{{error.message}}</pre>
+            </div>
           {% endfor %}
         </div>
       </div>
@@ -271,33 +261,6 @@
 </div>
 {% endif %}
 
-
-{% if build.toaster_exceptions.count > 0 %}
-<div class="accordion span10 pull-right" id="exceptions">
-  <div class="accordion-group">
-    <div class="accordion-heading">
-      <a class="accordion-toggle exception toggle-exceptions">
-        <h2 id="exception-toggle">
-          <i class="icon-warning-sign"></i>
-          {{build.toaster_exceptions.count}} Toaster exception{{build.toaster_exceptions.count|pluralize}}
-        </h2>
-      </a>
-    </div>
-    <div class="accordion-body collapse" id="collapse-exceptions">
-      <div class="accordion-inner">
-        <div class="span10">
-          {% for exception in build.toaster_exceptions %}
-            <div class="alert alert-exception">
-              <pre>{{exception.message}}</pre>
-            </div>
-          {% endfor %}
-        </div>
-      </div>
-    </div>
-  </div>
-</div>
-{% endif %}
-
 <script type="text/javascript">
     $(document).ready(function() {
         //show warnings section when requested from the previous page
-- 
2.1.4

[PATCH 3/4] toaster: Guard against builds with no targets

[Ed Bartosh]

From: Elliot Smith <elliot.smith@intel.com>

Although this probably can't happen in practice (builds without
targets), this constraint is not currently enforced in the data model.

In the unlikely event that a build has no target (e.g. in test cases),
this causes a template rendering error.

Rather than rework the data model, add a guard to the template
to prevent it from rendering the target name if there are no
targets associated with the build.

Signed-off-by: Elliot Smith <elliot.smith@intel.com>
Signed-off-by: Ed Bartosh <ed.bartosh@linux.intel.com>
---
 lib/toaster/toastergui/templates/builddashboard.html | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/lib/toaster/toastergui/templates/builddashboard.html b/lib/toaster/toastergui/templates/builddashboard.html
index 75ca8bc..c39fe34 100644
--- a/lib/toaster/toastergui/templates/builddashboard.html
+++ b/lib/toaster/toastergui/templates/builddashboard.html
@@ -3,7 +3,12 @@
 {% load projecttags %}
 
 {% block parentbreadcrumb %}
-{{build.get_sorted_target_list.0.target}} {%if build.target_set.all.count > 1%}(+ {{build.target_set.all.count|add:"-1"}}){%endif%} {{build.machine}} ({{build.completed_on|date:"d/m/y H:i"}})
+{% if build.get_sorted_target_list.count > 0 %}
+  {{build.get_sorted_target_list.0.target}}
+  &nbsp;
+{% endif %}
+
+{%if build.target_set.all.count > 1%}(+ {{build.target_set.all.count|add:"-1"}}){%endif%} {{build.machine}} ({{build.completed_on|date:"d/m/y H:i"}})
 {% endblock %}
 
 {% block buildinfomain %}
@@ -62,7 +67,7 @@
       <div class="accordion-inner">
         <div class="span10">
           {% for error in build.errors %}
-            <div class="alert alert-error">
+            <div class="alert alert-error" data-error="{{ error.id }}">
               <pre>{{error.message}}</pre>
             </div>
           {% endfor %}
-- 
2.1.4

[PATCH 4/4] toaster: Add tests for error message display on the build dashboard

[Ed Bartosh]

From: Elliot Smith <elliot.smith@intel.com>

Add tests to ensure that log messages with CRITICAL level and
with EXCEPTION level are shown in the main errors display
on the build dashboard.

[YOCTO #8320]

Signed-off-by: Elliot Smith <elliot.smith@intel.com>
Signed-off-by: Ed Bartosh <ed.bartosh@linux.intel.com>
---
 lib/toaster/toastergui/tests.py | 91 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 89 insertions(+), 2 deletions(-)

diff --git a/lib/toaster/toastergui/tests.py b/lib/toaster/toastergui/tests.py
index 7c4bd39..29dd7fd 100644
--- a/lib/toaster/toastergui/tests.py
+++ b/lib/toaster/toastergui/tests.py
@@ -26,15 +26,15 @@ from django.test.client import RequestFactory
 from django.core.urlresolvers import reverse
 from django.utils import timezone
 
-from orm.models import Project, Release, BitbakeVersion, Package
+from orm.models import Project, Release, BitbakeVersion, Package, LogMessage
 from orm.models import ReleaseLayerSourcePriority, LayerSource, Layer, Build
 from orm.models import Layer_Version, Recipe, Machine, ProjectLayer, Target
 from orm.models import CustomImageRecipe, ProjectVariable
 from orm.models import Branch
 
 from toastergui.tables import SoftwareRecipesTable
-from bs4 import BeautifulSoup
 import json
+from bs4 import BeautifulSoup
 import re
 
 PROJECT_NAME = "test project"
@@ -832,3 +832,90 @@ class ProjectPageTests(TestCase):
         response = self.client.get(url, follow=True)
 
         self.assertEqual(response.status_code, 200)
+
+class BuildDashboardTests(TestCase):
+    """ Tests for the build dashboard /build/X """
+
+    def setUp(self):
+        bbv = BitbakeVersion.objects.create(name="bbv1", giturl="/tmp/",
+                                            branch="master", dirpath="")
+        release = Release.objects.create(name="release1",
+                                         bitbake_version=bbv)
+        project = Project.objects.create_project(name=PROJECT_NAME,
+                                                 release=release)
+
+        now = timezone.now()
+
+        self.build1 = Build.objects.create(project=project,
+                                           started_on=now,
+                                           completed_on=now)
+
+        # exception
+        msg1 = 'an exception was thrown'
+        self.exception_message = LogMessage.objects.create(
+            build=self.build1,
+            level=LogMessage.EXCEPTION,
+            message=msg1
+        )
+
+        # critical
+        msg2 = 'a critical error occurred'
+        self.critical_message = LogMessage.objects.create(
+            build=self.build1,
+            level=LogMessage.CRITICAL,
+            message=msg2
+        )
+
+    def _get_build_dashboard_errors(self):
+        """
+        Get a list of HTML fragments representing the errors on the
+        build dashboard
+        """
+        url = reverse('builddashboard', args=(self.build1.id,))
+        response = self.client.get(url)
+        soup = BeautifulSoup(response.content)
+        return soup.select('#errors div.alert-error')
+
+    def _check_for_log_message(self, log_message):
+        """
+        Check whether the LogMessage instance <log_message> is
+        represented as an HTML error in the build dashboard page
+        """
+        errors = self._get_build_dashboard_errors()
+        self.assertEqual(len(errors), 2)
+
+        expected_text = log_message.message
+        expected_id = str(log_message.id)
+
+        found = False
+        for error in errors:
+            error_text = error.find('pre').text
+            text_matches = (error_text == expected_text)
+
+            error_id = error['data-error']
+            id_matches = (error_id == expected_id)
+
+            if text_matches and id_matches:
+                found = True
+                break
+
+        template_vars = (expected_text, error_text,
+                         expected_id, error_id)
+        assertion_error_msg = 'exception not found as error: ' \
+            'expected text "%s" and got "%s"; ' \
+            'expected ID %s and got %s' % template_vars
+        self.assertTrue(found, assertion_error_msg)
+
+    def test_exceptions_show_as_errors(self):
+        """
+        LogMessages with level EXCEPTION should display in the errors
+        section of the page
+        """
+        self._check_for_log_message(self.exception_message)
+
+    def test_criticals_show_as_errors(self):
+        """
+        LogMessages with level CRITICAL should display in the errors
+        section of the page
+        """
+        self._check_for_log_message(self.critical_message)
-- 
2.1.4

Re: [PATCH 4/4] toaster: Add tests for error message display on the build dashboard

[Richard Purdie]

On Thu, 2015-10-15 at 15:45 +0300, Ed Bartosh wrote:
> From: Elliot Smith <elliot.smith@intel.com>
> 
> Add tests to ensure that log messages with CRITICAL level and
> with EXCEPTION level are shown in the main errors display
> on the build dashboard.
> 
> [YOCTO #8320]
> 
> Signed-off-by: Elliot Smith <elliot.smith@intel.com>
> Signed-off-by: Ed Bartosh <ed.bartosh@linux.intel.com>
> ---
>  lib/toaster/toastergui/tests.py | 91 ++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 89 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/toaster/toastergui/tests.py b/lib/toaster/toastergui/tests.py
> index 7c4bd39..29dd7fd 100644
> --- a/lib/toaster/toastergui/tests.py
> +++ b/lib/toaster/toastergui/tests.py
> @@ -26,15 +26,15 @@ from django.test.client import RequestFactory
>  from django.core.urlresolvers import reverse
>  from django.utils import timezone
>  
> -from orm.models import Project, Release, BitbakeVersion, Package
> +from orm.models import Project, Release, BitbakeVersion, Package, LogMessage
>  from orm.models import ReleaseLayerSourcePriority, LayerSource, Layer, Build
>  from orm.models import Layer_Version, Recipe, Machine, ProjectLayer, Target
>  from orm.models import CustomImageRecipe, ProjectVariable
>  from orm.models import Branch
>  
>  from toastergui.tables import SoftwareRecipesTable
> -from bs4 import BeautifulSoup
>  import json
> +from bs4 import BeautifulSoup
>  import re
 
Sadly this failed to apply to master, please send a version which
does...

Cheers,

Richard