Merge lp:~zyga/checkbox/result-history into lp:checkbox

[Zygmunt Krynicki]

56b7b43 checkbox-ng:service: fix copy-paste error
17c2aea plainbox:session:jobs: fix PEP257 issues
f23ccc2 plainbox:session:jobs: keep track of result history
284f0b8 plainbox:session:suspend: fix PEP257 issue
98647a1 plainbox:session:suspend: store full result history
d2282fb plainbox:session:result: replay the full result history
19e21e2 plainbox:exporter:text: fix PEP257 issue
8b389d0 plainbox:exporter:text: display result history
3f0f87d plainbox:exporter:html: display result history

[lp:~zyga/checkbox/result-history]

3823. By Zygmunt Krynicki on 2015-05-28

plainbox:exporter:text: display result history

Signed-off-by: Zygmunt Krynicki <email address hidden>

3824. By Zygmunt Krynicki on 2015-05-28

plainbox:exporter:html: display result history

Signed-off-by: Zygmunt Krynicki <email address hidden>

[Sylvain Pineau (sylvain-pineau)]

tested with both gui and cli using the smoke testplan. all the results are available in the report.

+1

review: Approve

=== modified file 'checkbox-ng/checkbox_ng/service.py'
--- checkbox-ng/checkbox_ng/service.py	2015-05-28 10:04:37 +0000
+++ checkbox-ng/checkbox_ng/service.py	2015-05-28 11:25:44 +0000
@@ -528,7 +528,7 @@
         return self.native.comments or ""
 
     @comments.setter
-    def comments(self, value):
+    def comments(self, new_value):
         """
         set comments to a new value
         """

=== modified file 'plainbox/plainbox/data/report/checkbox.html'
--- plainbox/plainbox/data/report/checkbox.html	2015-05-19 17:56:35 +0000
+++ plainbox/plainbox/data/report/checkbox.html	2015-05-28 11:25:44 +0000
@@ -127,6 +127,9 @@
         font-size: 10px;
         line-height: 14px;
       }
+      tr.historic-run td:first-child {
+        padding-left: 2em;
+      }
       .data {
         display:    none;
       }
@@ -217,6 +220,7 @@
                         <th>Test ID</th>
                         <th>Result</th>
                         <th>Certification status</th>
+                        <th>Run</th>
                         <th>Comment</th>
                     </tr>
                 </thead>
@@ -226,6 +230,7 @@
                         <td>{{ job_state.job.tr_summary() }}</td>
                         <td style='font-weight: bold; color: {{ job_state.result.outcome_meta().color_hex }}'>{{ job_state.result.outcome_meta().tr_label }}</td>
                         <td>{{ job_state.effective_certification_status }}</td>
+                        <td>{{ job_state.result_history|length }}</td>
                         {%- if job_state.result.comments != None %}
                         <td>{{ job_state.result.comments }}</td>
                         {%- else %}
@@ -239,6 +244,26 @@
                         {%- endif %}
                         {%- endif %}
                     </tr>
+                    {%- for result in job_state.result_history[:-1] %}
+                    <tr class='historic-run'>
+                        <td>{{ job_state.job.tr_summary() }}</td>
+                        <td style='font-weight: bold; color: {{ result.outcome_meta().color_hex }}'>{{ result.outcome_meta().tr_label }}</td>
+                        <td></td>
+                        <td>{{ loop.index }}</td>
+                        {%- if result.comments != None %}
+                        <td>{{ result.comments }}</td>
+                        {%- else %}
+                        {%- if result.io_log_as_flat_text != "" %}
+                        <td><div style="vertical-align: middle; width: 420px; overflow: auto;">
+                                <pre>{{ result.io_log_as_flat_text }}</pre>
+                            </div>
+                        </td>
+                        {%- else %}
+                        <td>&nbsp;</td>
+                        {%- endif %}
+                        {%- endif %}
+                    </tr>
+                    {%- endfor %}
                 {%- endfor %}
                 </tbody>
             </table>

=== modified file 'plainbox/plainbox/impl/exporter/test_text.py'
--- plainbox/plainbox/impl/exporter/test_text.py	2014-11-11 06:06:55 +0000
+++ plainbox/plainbox/impl/exporter/test_text.py	2015-05-28 11:25:44 +0000
@@ -44,7 +44,7 @@
         data = mock.Mock(
             run_list=[job],
             job_state_map={
-                job.id: mock.Mock(result=result, job=job)
+                job.id: mock.Mock(result=result, job=job, result_history=())
             }
         )
         stream = BytesIO()

=== modified file 'plainbox/plainbox/impl/exporter/text.py'
--- plainbox/plainbox/impl/exporter/text.py	2015-03-31 08:38:35 +0000
+++ plainbox/plainbox/impl/exporter/text.py	2015-05-28 11:25:44 +0000
@@ -25,15 +25,15 @@
 
     THIS MODULE DOES NOT HAVE STABLE PUBLIC API
 """
+from plainbox.i18n import gettext as _
 from plainbox.impl.commands.inv_run import Colorizer
 from plainbox.impl.exporter import SessionStateExporterBase
 from plainbox.impl.result import outcome_meta
 
 
 class TextSessionStateExporter(SessionStateExporterBase):
-    """
-    Human-readable session state exporter.
-    """
+
+    """Human-readable session state exporter."""
 
     def __init__(self, option_list=None, color=None):
         super().__init__(option_list)
@@ -55,9 +55,23 @@
                             outcome_meta(state.result.outcome).color_ansi
                         ), state.job.tr_summary(),
                     ).encode("UTF-8"))
+                if len(state.result_history) > 1:
+                    stream.write(_("  history: {0}\n").format(
+                        ', '.join(
+                            self.C.custom(
+                                result.outcome_meta().tr_outcome,
+                                result.outcome_meta().color_ansi)
+                            for result in state.result_history)
+                    ).encode("UTF-8"))
             else:
                 stream.write(
                     "{:^15}: {}\n".format(
                         state.result.tr_outcome(),
                         state.job.tr_summary(),
                     ).encode("UTF-8"))
+                if state.result_history:
+                    print(_("History:"), ', '.join(
+                        self.C.custom(
+                            result.outcome_meta().unicode_sigil,
+                            result.outcome_meta().color_ansi)
+                        for result in state.result_history))

=== modified file 'plainbox/plainbox/impl/session/jobs.py'
--- plainbox/plainbox/impl/session/jobs.py	2015-04-09 13:20:17 +0000
+++ plainbox/plainbox/impl/session/jobs.py	2015-05-28 11:25:44 +0000
@@ -16,6 +16,8 @@
 # You should have received a copy of the GNU General Public License
 # along with Checkbox.  If not, see <http://www.gnu.org/licenses/>.
 """
+Job State.
+
 :mod:`plainbox.impl.session.jobs` -- jobs state handling
 ========================================================
 
@@ -39,8 +41,9 @@
 
 
 class InhibitionCause(IntEnum):
+
     """
-    There are four possible not-ready causes:
+    There are four possible not-ready causes.
 
         UNDESIRED:
             This job was not selected to run in this session
@@ -58,6 +61,7 @@
         FAILED_RESOURCE:
             This job has a resource requirement that evaluated to a false value
     """
+
     UNDESIRED = 0
     PENDING_DEP = 1
     FAILED_DEP = 2
@@ -68,6 +72,8 @@
 def cause_convert_assign_filter(
         instance: pod.POD, field: pod.Field, old: "Any", new: "Any") -> "Any":
     """
+    Assign filter for for JobReadinessInhibitor.cause.
+
     Custom assign filter for the JobReadinessInhibitor.cause field that
     produces a very specific error message.
     """
@@ -78,6 +84,7 @@
 
 
 class JobReadinessInhibitor(pod.POD):
+
     """
     Class representing the cause of a job not being ready to execute.
 
@@ -122,6 +129,7 @@
             Provides additional context for the problem caused by a failing
             resource expression.
     """
+
     # XXX: PENDING_RESOURCE is not strict, there are multiple states that are
     # clumped here which is something I don't like. A resource may be still
     # "pending" as in PENDING_DEP (it has not ran yet) or it could have ran but
@@ -176,11 +184,13 @@
             ).format(self.cause.name))
 
     def __repr__(self):
+        """Get a custom debugging representation of an inhibitor."""
         return "<{} cause:{} related_job:{!r} related_expression:{!r}>".format(
             self.__class__.__name__, self.cause.name, self.related_job,
             self.related_expression)
 
     def __str__(self):
+        """Get a human-readable text representation of an inhibitor."""
         if self.cause == InhibitionCause.UNDESIRED:
             # TRANSLATORS: as in undesired job
             return _("undesired")
@@ -211,7 +221,10 @@
 
 
 class OverridableJobField(pod.Field):
+
     """
+    A custom Field for modeling job values that can be overridden.
+
     A readable-writable field that has a special initial value ``JOB_VALUE``
     which is interpreted as "load this value from the corresponding job
     definition".
@@ -222,11 +235,13 @@
 
     def __init__(self, job_field, doc=None, type=None, notify=False,
                  assign_filter_list=None):
+        """Initialize a new overridable job field."""
         super().__init__(
             doc, type, JOB_VALUE, None, notify, assign_filter_list)
         self.job_field = job_field
 
     def __get__(self, instance, owner):
+        """Get an overriden (if any) value of an overridable job field."""
         value = super().__get__(instance, owner)
         if value is JOB_VALUE:
             return getattr(instance.job, self.job_field)
@@ -235,22 +250,28 @@
 
 
 def job_assign_filter(instance, field, old_value, new_value):
-    # FIXME: This setter should not exist. job attribute should be
-    # read-only. This is a temporary kludge to get session restoring
-    # over DBus working. Once a solution that doesn't involve setting
-    # a JobState's job attribute is implemented, please remove this
-    # awful method.
+    """
+    A custom setter for the JobState.job.
+
+    .. warning::
+        This setter should not exist. job attribute should be read-only. This
+        is a temporary kludge to get session restoring over DBus working. Once
+        a solution that doesn't involve setting a JobState's job attribute is
+        implemented, please remove this awful method.
+    """
     return new_value
 
 
 def job_via_assign_filter(instance, field, old_value, new_value):
+    """A custom setter for JobState.via_job."""
     if (old_value is not pod.UNSET and not isinstance(new_value, JobDefinition)
-        and not new_value is None):
+            and new_value is not None):
         raise TypeError("via_job must be the actual job, not the checksum")
     return new_value
 
 
 class JobState(pod.POD):
+
     """
     Class representing the state of a job in a session.
 
@@ -284,6 +305,11 @@
         initial_fn=lambda: MemoryJobResult({}),
         notify=True)
 
+    result_history = pod.Field(
+        doc="a tuple of result_history of the associated job",
+        type=tuple, initial=(), notify=True,
+        assign_filter_list=[pod.typed, pod.typed.sequence(IJobResult)])
+
     via_job = pod.Field(
         doc="the parent job definition",
         type=JobDefinition,
@@ -299,16 +325,36 @@
         doc="the effective certification status of this job",
         type=str)
 
+    # NOTE: the `result` property just exposes the last result from the
+    # `result_history` tuple above. The API is used everywhere so it should not
+    # be broken in any way but the way forward is the sequence stored in
+    # `result_history`.
+    #
+    # The one particularly annoying part of this implementation is that each
+    # job state always has at least one result. Even if there was no testing
+    # done yet. This OUTCOME_NONE result needs to be filtered out at various
+    # times. I think it would be better if we could not have it in the
+    # sequence-based API anymore. Otherwise each test will have two
+    # result_history (more if you count things like resuming a session).
+
+    @result.change_notifier
+    def _result_changed(self, old, new):
+        # Don't track the initial assignment over UNSET
+        if old is pod.UNSET:
+            return
+        assert new != old
+        assert isinstance(new, IJobResult)
+        if new.is_hollow:
+            return
+        logger.debug("Appending result %r to history: %r", new, self.result_history)
+        self.result_history += (new,)
+
     def can_start(self):
-        """
-        Quickly check if the associated job can run right now.
-        """
+        """Quickly check if the associated job can run right now."""
         return len(self.readiness_inhibitor_list) == 0
 
     def get_readiness_description(self):
-        """
-        Get a human readable description of the current readiness state
-        """
+        """Get a human readable description of the current readiness state."""
         if self.readiness_inhibitor_list:
             return _("job cannot be started: {}").format(
                 ", ".join((str(inhibitor)

=== modified file 'plainbox/plainbox/impl/session/resume.py'
--- plainbox/plainbox/impl/session/resume.py	2015-04-13 18:20:44 +0000
+++ plainbox/plainbox/impl/session/resume.py	2015-05-28 11:25:44 +0000
@@ -707,13 +707,10 @@
             result = self._build_JobResult(
                 result_repr, self.flags, self.location)
             result_list.append(result)
-        # Show the _LAST_ result to the session. Currently we only store one
-        # result but showing the most recent (last) result should be good
-        # in general.
-        if len(result_list) > 0:
-            logger.debug(
-                _("calling update_job_result(%r, %r)"), job, result_list[-1])
-            session.update_job_result(job, result_list[-1])
+        # Replay each result, one by one
+        for result in result_list:
+            logger.debug(_("calling update_job_result(%r, %r)"), job, result)
+            session.update_job_result(job, result)
 
     @classmethod
     def _restore_SessionState_desired_job_list(cls, session, session_repr):

=== modified file 'plainbox/plainbox/impl/session/suspend.py'
--- plainbox/plainbox/impl/session/suspend.py	2015-04-13 13:58:00 +0000
+++ plainbox/plainbox/impl/session/suspend.py	2015-05-28 11:25:44 +0000
@@ -238,7 +238,7 @@
         }
 
     def _repr_JobResult(self, obj, session_dir):
-        """ Compute the representation of one of IJobResult subclasses.  """
+        """Compute the representation of one of IJobResult subclasses."""
         if isinstance(obj, DiskJobResult):
             return self._repr_DiskJobResult(obj, session_dir)
         elif isinstance(obj, MemoryJobResult):
@@ -510,11 +510,10 @@
                 if not state.result.is_hollow or state.job.id in id_run_list
             },
             "results": {
-                # Currently we store only one result but we may store
-                # more than that in a later version.
-                state.job.id: [self._repr_JobResult(state.result, session_dir)]
+                state.job.id: [self._repr_JobResult(result, session_dir)
+                               for result in state.result_history]
                 for state in obj.job_state_map.values()
-                if not state.result.is_hollow
+                if len(state.result_history) > 0
             },
             "desired_job_list": [
                 job.id for job in obj.desired_job_list

=== modified file 'plainbox/plainbox/impl/session/test_jobs.py'
--- plainbox/plainbox/impl/session/test_jobs.py	2015-04-09 13:20:17 +0000
+++ plainbox/plainbox/impl/session/test_jobs.py	2015-05-28 11:25:44 +0000
@@ -140,6 +140,7 @@
     def test_smoke(self):
         self.assertIsNotNone(self.job_state.result)
         self.assertIs(self.job_state.result.outcome, IJobResult.OUTCOME_NONE)
+        self.assertEqual(self.job_state.result_history, ())
         self.assertEqual(self.job_state.readiness_inhibitor_list, [
             UndesiredJobReadinessInhibitor])
         self.assertEqual(self.job_state.effective_category_id,
@@ -164,6 +165,16 @@
         self.job_state.result = result
         self.assertIs(self.job_state.result, result)
 
+    def test_result_history_keeps_track_of_result_changes(self):
+        # XXX: this example will fail if subsequent results are identical
+        self.assertEqual(self.job_state.result_history, ())
+        result1 = make_job_result(outcome='fail')
+        self.job_state.result = result1
+        self.assertEqual(self.job_state.result_history, (result1,))
+        result2 = make_job_result(outcome='pass')
+        self.job_state.result = result2
+        self.assertEqual(self.job_state.result_history, (result1, result2))
+
     def test_setting_result_fires_signal(self):
         """
         verify that assigning state.result fires the on_result_changed signal

=== modified file 'plainbox/plainbox/test-data/html-exporter/with_both_certification_status.html'
--- plainbox/plainbox/test-data/html-exporter/with_both_certification_status.html	2015-05-19 17:56:35 +0000
+++ plainbox/plainbox/test-data/html-exporter/with_both_certification_status.html	2015-05-28 11:25:44 +0000
@@ -123,6 +123,9 @@
         font-size: 10px;
         line-height: 14px;
       }
+      tr.historic-run td:first-child {
+        padding-left: 2em;
+      }
       .data {
         display:    none;
       }
@@ -203,6 +206,7 @@
                         <th>Test ID</th>
                         <th>Result</th>
                         <th>Certification status</th>
+                        <th>Run</th>
                         <th>Comment</th>
                     </tr>
                 </thead>
@@ -211,6 +215,7 @@
                         <td>job 1</td>
                         <td style='font-weight: bold; color: #DC3912'>failed</td>
                         <td>blocker</td>
+                        <td>1</td>
                         <td><div style="vertical-align: middle; width: 420px; overflow: auto;">
                                 <pre>FATAL ERROR
 </pre>
@@ -221,16 +226,25 @@
                         <td>job 2</td>
                         <td style='font-weight: bold; color: #DC3912'>failed</td>
                         <td>non-blocker</td>
+                        <td>2</td>
                         <td><div style="vertical-align: middle; width: 420px; overflow: auto;">
                                 <pre>FATAL ERROR
 </pre>
                             </div>
                         </td>
                     </tr>
+                    <tr class='historic-run'>
+                        <td>job 2</td>
+                        <td style='font-weight: bold; color: #6AA84F'>passed</td>
+                        <td></td>
+                        <td>1</td>
+                        <td>blah blah</td>
+                    </tr>
                     <tr>
                         <td>job 3</td>
                         <td style='font-weight: bold; color: #FF9900'>skipped</td>
                         <td>unspecified</td>
+                        <td>1</td>
                         <td>No such device</td>
                     </tr>
                 </tbody>

=== modified file 'plainbox/plainbox/test-data/html-exporter/with_certification_blocker.html'
--- plainbox/plainbox/test-data/html-exporter/with_certification_blocker.html	2015-05-19 17:56:35 +0000
+++ plainbox/plainbox/test-data/html-exporter/with_certification_blocker.html	2015-05-28 11:25:44 +0000
@@ -123,6 +123,9 @@
         font-size: 10px;
         line-height: 14px;
       }
+      tr.historic-run td:first-child {
+        padding-left: 2em;
+      }
       .data {
         display:    none;
       }
@@ -186,6 +189,7 @@
                         <th>Test ID</th>
                         <th>Result</th>
                         <th>Certification status</th>
+                        <th>Run</th>
                         <th>Comment</th>
                     </tr>
                 </thead>
@@ -194,6 +198,7 @@
                         <td>job 1</td>
                         <td style='font-weight: bold; color: #DC3912'>failed</td>
                         <td>blocker</td>
+                        <td>1</td>
                         <td><div style="vertical-align: middle; width: 420px; overflow: auto;">
                                 <pre>FATAL ERROR
 </pre>
@@ -204,12 +209,14 @@
                         <td>job 2</td>
                         <td style='font-weight: bold; color: #6AA84F'>passed</td>
                         <td>unspecified</td>
+                        <td>1</td>
                         <td>blah blah</td>
                     </tr>
                     <tr>
                         <td>job 3</td>
                         <td style='font-weight: bold; color: #FF9900'>skipped</td>
                         <td>unspecified</td>
+                        <td>1</td>
                         <td>No such device</td>
                     </tr>
                 </tbody>

=== modified file 'plainbox/plainbox/test-data/html-exporter/with_certification_non_blocker.html'
--- plainbox/plainbox/test-data/html-exporter/with_certification_non_blocker.html	2015-05-19 17:56:35 +0000
+++ plainbox/plainbox/test-data/html-exporter/with_certification_non_blocker.html	2015-05-28 11:25:44 +0000
@@ -123,6 +123,9 @@
         font-size: 10px;
         line-height: 14px;
       }
+      tr.historic-run td:first-child {
+        padding-left: 2em;
+      }
       .data {
         display:    none;
       }
@@ -184,6 +187,7 @@
                         <th>Test ID</th>
                         <th>Result</th>
                         <th>Certification status</th>
+                        <th>Run</th>
                         <th>Comment</th>
                     </tr>
                 </thead>
@@ -192,6 +196,7 @@
                         <td>job 1</td>
                         <td style='font-weight: bold; color: #DC3912'>failed</td>
                         <td>non-blocker</td>
+                        <td>1</td>
                         <td><div style="vertical-align: middle; width: 420px; overflow: auto;">
                                 <pre>FATAL ERROR
 </pre>
@@ -202,12 +207,14 @@
                         <td>job 2</td>
                         <td style='font-weight: bold; color: #6AA84F'>passed</td>
                         <td>unspecified</td>
+                        <td>1</td>
                         <td>blah blah</td>
                     </tr>
                     <tr>
                         <td>job 3</td>
                         <td style='font-weight: bold; color: #FF9900'>skipped</td>
                         <td>unspecified</td>
+                        <td>1</td>
                         <td>No such device</td>
                     </tr>
                 </tbody>

=== modified file 'plainbox/plainbox/test-data/html-exporter/without_certification_status.html'
--- plainbox/plainbox/test-data/html-exporter/without_certification_status.html	2015-05-19 17:56:35 +0000
+++ plainbox/plainbox/test-data/html-exporter/without_certification_status.html	2015-05-28 11:25:44 +0000
@@ -123,6 +123,9 @@
         font-size: 10px;
         line-height: 14px;
       }
+      tr.historic-run td:first-child {
+        padding-left: 2em;
+      }
       .data {
         display:    none;
       }
@@ -167,6 +170,7 @@
                         <th>Test ID</th>
                         <th>Result</th>
                         <th>Certification status</th>
+                        <th>Run</th>
                         <th>Comment</th>
                     </tr>
                 </thead>
@@ -175,6 +179,7 @@
                         <td>job 1</td>
                         <td style='font-weight: bold; color: #DC3912'>failed</td>
                         <td>unspecified</td>
+                        <td>1</td>
                         <td><div style="vertical-align: middle; width: 420px; overflow: auto;">
                                 <pre>FATAL ERROR
 </pre>
@@ -185,12 +190,14 @@
                         <td>job 2</td>
                         <td style='font-weight: bold; color: #6AA84F'>passed</td>
                         <td>unspecified</td>
+                        <td>1</td>
                         <td>blah blah</td>
                     </tr>
                     <tr>
                         <td>job 3</td>
                         <td style='font-weight: bold; color: #FF9900'>skipped</td>
                         <td>unspecified</td>
+                        <td>1</td>
                         <td>No such device</td>
                     </tr>
                 </tbody>