Merge lp:~zyga/checkbox/fix-1378300 into lp:checkbox

[Zygmunt Krynicki]

f29a9ee plainbox:result: add JobResultBase.is_hollow
6df3da5 plainbox:abc: add IJobResult.is_hollow
27a0bfe plainbox:session:jobs: ensure that initial memory results are hollow
b3d3d48 plainbox:session:resume: always retain desired jobs
2d44c53 plainbox:session:resume: add 4th resume helper
f205eb1 plainbox:session:resume: enable 4th resume helper
bef4ea9 plainbox:session:resume: add 4th peek helper
f6aed57 plainbox:session:resume: enable 4th peek helper
7ab91e8 plainbox:session:suspend: add 4th suspend format
5e753c6 plainbox:session:suspend: use the 4th format by default

[Daniel Manrique (roadmr)]

OK, I tested it (see repro case in the last comment on the bug, and the original report is good for this too), and the code makes sense.

review: Approve

=== modified file 'plainbox/plainbox/abc.py'
--- plainbox/plainbox/abc.py	2014-09-25 15:31:26 +0000
+++ plainbox/plainbox/abc.py	2014-10-08 18:01:17 +0000
@@ -264,6 +264,20 @@
             bytes object that was obtained from that stream.
         """
 
+    @abstractproperty
+    def is_hollow(self):
+        """
+        flag that indicates if the result is hollow
+
+        Hollow results may have been created but hold no data at all.
+        Hollow results are also tentatively deprecated, once we have some
+        time to re-factor SessionState and specifically the job_state_map
+        code we will remove the need to have hollow results.
+
+        Hollow results are not saved, beginning with
+        :class:`plainbox.impl.session.suspend.SessionSuspendHelper4`.
+        """
+
 
 class IJobQualifier(metaclass=ABCMeta):
     """

=== modified file 'plainbox/plainbox/impl/result.py'
--- plainbox/plainbox/impl/result.py	2014-10-06 09:50:18 +0000
+++ plainbox/plainbox/impl/result.py	2014-10-08 18:01:17 +0000
@@ -185,6 +185,20 @@
     def io_log(self):
         return tuple(self.get_io_log())
 
+    @property
+    def is_hollow(self):
+        """
+        flag that indicates if the result is hollow
+
+        Hollow results may have been created but hold no data at all.
+        Hollow results are also tentatively deprecated, once we have some
+        time to re-factor SessionState and specifically the job_state_map
+        code we will remove the need to have hollow results.
+
+        Hollow results are not saved, beginning with
+        :class:`plainbox.impl.session.suspend.SessionSuspendHelper4`.
+        """
+        return not bool(self._data)
 
 class MemoryJobResult(_JobResultBase):
     """

=== modified file 'plainbox/plainbox/impl/session/jobs.py'
--- plainbox/plainbox/impl/session/jobs.py	2014-09-13 11:37:17 +0000
+++ plainbox/plainbox/impl/session/jobs.py	2014-10-08 18:01:17 +0000
@@ -201,9 +201,8 @@
         """
         self._job = job
         self._readiness_inhibitor_list = [UndesiredJobReadinessInhibitor]
-        self._result = MemoryJobResult({
-            'outcome': IJobResult.OUTCOME_NONE
-        })
+        self._result = MemoryJobResult({})
+        assert self._result.is_hollow
 
     def __repr__(self):
         fmt = ("<{} job:{!r} readiness_inhibitor_list:{!r} result:{!r}>")

=== modified file 'plainbox/plainbox/impl/session/resume.py'
--- plainbox/plainbox/impl/session/resume.py	2014-09-12 13:57:59 +0000
+++ plainbox/plainbox/impl/session/resume.py	2014-10-08 18:01:17 +0000
@@ -160,6 +160,8 @@
             return SessionPeekHelper2().peek_json(json_repr)
         elif version == 3:
             return SessionPeekHelper3().peek_json(json_repr)
+        elif version == 4:
+            return SessionPeekHelper4().peek_json(json_repr)
         else:
             raise IncompatibleSessionError(
                 _("Unsupported version {}").format(version))
@@ -236,6 +238,9 @@
         elif version == 3:
             return SessionResumeHelper3(
                 self.job_list).resume_json(json_repr, early_cb)
+        elif version == 4:
+            return SessionResumeHelper4(
+                self.job_list).resume_json(json_repr, early_cb)
         else:
             raise IncompatibleSessionError(
                 _("Unsupported version {}").format(version))
@@ -247,11 +252,13 @@
     after doing a session resume.
     """
 
-    def __init__(self, jobs_repr):
+    def __init__(self, jobs_repr, desired_job_list_repr):
         super().__init__(Origin.get_caller_origin())
         # Set of ids of jobs to retain (computed as keys of the
         # dictionary taken from the session resume representation)
-        self._retain_id_set = frozenset(jobs_repr)
+        self._retain_id_set = (
+            frozenset(jobs_repr)
+            | frozenset(desired_job_list_repr))
 
     def get_simple_match(self, job):
         return job.id not in self._retain_id_set
@@ -393,6 +400,19 @@
     """
 
 
+class SessionPeekHelper4(SessionPeekHelper3):
+    """
+    Helper class for implementing session peek feature
+
+    This class works with data constructed by
+    :class:`~plainbox.impl.session.suspend.SessionSuspendHelper1` which has
+    been pre-processed by :class:`SessionPeekHelper` (to strip the initial
+    envelope).
+
+    The only goal of this class is to reconstruct session state meta-data.
+    """
+
+
 class SessionResumeHelper1(MetaDataHelper1MixIn):
     """
     Helper class for implementing session resume feature
@@ -607,10 +627,13 @@
         go wrong must have gone wrong before.
         """
 
-        # Representation of all of the job definitions
+        # Representation of all of the important job definitions
         jobs_repr = _validate(session_repr, key='jobs', value_type=dict)
+        # Representation of all the desired job definitions
+        desired_job_list_repr = _validate(
+            session_repr, key='desired_job_list', value_type=list)
         # Qualifier ready to select jobs to remove
-        qualifier = ResumeDiscardQualifier(jobs_repr)
+        qualifier = ResumeDiscardQualifier(jobs_repr, desired_job_list_repr)
         # NOTE: this should never raise ValueError (which signals that we
         # tried to remove a job which is in the run list) because it should
         # only remove jobs that were not in the representation and any job in
@@ -726,6 +749,25 @@
     """
 
 
+class SessionResumeHelper4(SessionResumeHelper3):
+    """
+    Helper class for implementing session resume feature
+
+    This class works with data constructed by
+    :class:`~plainbox.impl.session.suspend.SessionSuspendHelper4` which has
+    been pre-processed by :class:`SessionResumeHelper` (to strip the initial
+    envelope).
+
+    Due to the constraints of what can be represented in a suspended session,
+    this class cannot work in isolation. It must operate with a list of know
+    jobs.
+
+    Since (most of the) jobs are being provided externally (as they represent
+    the non-serialized parts of checkbox or other job providers) several
+    failure modes are possible. Those are documented in :meth:`resume()`
+    """
+
+
 def _validate(obj, **flags):
     """
     Multi-purpose extraction and validation function.

=== modified file 'plainbox/plainbox/impl/session/suspend.py'
--- plainbox/plainbox/impl/session/suspend.py	2014-03-17 11:31:12 +0000
+++ plainbox/plainbox/impl/session/suspend.py	2014-10-08 18:01:17 +0000
@@ -77,6 +77,8 @@
    :attr:`plainbox.impl.session.state.SessionMetaData.app_blob`
 3) Same as '2' but suspends
    :attr:`plainbox.impl.session.state.SessionMetaData.app_id`
+4) Same as '3' but hollow results are not saved and jobs that only
+   have hollow results are not mentioned in the job -> checksum map.
 """
 
 import gzip
@@ -435,5 +437,71 @@
         return data
 
 
+class SessionSuspendHelper4(SessionSuspendHelper3):
+    """
+    Helper class for computing binary representation of a session.
+
+    The helper only creates a bytes object to save. Actual saving should
+    be performed using some other means, preferably using
+    :class:`~plainbox.impl.session.storage.SessionStorage`.
+
+    This class creates version '4' snapshots.
+    """
+
+    VERSION = 4
+
+    def _repr_SessionState(self, obj):
+        """
+        Compute the representation of :class:`SessionState`
+
+        :returns:
+            JSON-friendly representation
+        :rtype:
+            dict
+
+        The result is a dictionary with the following items:
+
+            ``jobs``:
+                Dictionary mapping job id to job checksum.
+                The checksum is computed with
+                :attr:`~plainbox.impl.job.JobDefinition.checksum`.
+                Only jobs that actually have a result are mentioned here.
+                The automatically generated "None" result that is always
+                present for every job is skipped.
+
+            ``results``
+                Dictionary mapping job id to a list of results.
+                Each result is represented by data computed by
+                :meth:`_repr_JobResult()`. Only jobs that actually have
+                a result are mentioned here. The automatically generated
+                "None" result that is always present for every job is skipped.
+
+            ``desired_job_list``:
+                List of (ids) of jobs that are desired (to be executed)
+
+            ``metadata``:
+                The representation of meta-data associated with the session
+                state object.
+        """
+        return {
+            "jobs": {
+                state.job.id: state.job.checksum
+                for state in obj.job_state_map.values()
+                if not state.result.is_hollow
+            },
+            "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)]
+                for state in obj.job_state_map.values()
+                if not state.result.is_hollow
+            },
+            "desired_job_list": [
+                job.id for job in obj.desired_job_list
+            ],
+            "metadata": self._repr_SessionMetaData(obj.metadata),
+        }
+
+
 # Alias for the most recent version
-SessionSuspendHelper = SessionSuspendHelper3
+SessionSuspendHelper = SessionSuspendHelper4

=== modified file 'plainbox/plainbox/impl/session/test_resume.py'
--- plainbox/plainbox/impl/session/test_resume.py	2014-07-30 09:20:03 +0000
+++ plainbox/plainbox/impl/session/test_resume.py	2014-10-08 18:01:17 +0000
@@ -61,10 +61,11 @@
     def setUp(self):
         # The initializer accepts the jobs representation dictionary but uses
         # keys only. Here the values are dummy None objects
-        self.obj = ResumeDiscardQualifier({'foo': None, 'bar': None})
+        self.obj = ResumeDiscardQualifier({'foo': None, 'bar': None}, ['froz'])
 
     def test_init(self):
-        self.assertEqual(self.obj._retain_id_set, frozenset(['foo', 'bar']))
+        self.assertEqual(
+            self.obj._retain_id_set, frozenset(['foo', 'bar', 'froz']))
 
     def test_get_simple_match(self):
         # Direct hits return the IGNORE vote as those jobs are not to be
@@ -76,6 +77,9 @@
         self.assertEqual(
             self.obj.get_vote(JobDefinition({'id': 'bar'})),
             IJobQualifier.VOTE_IGNORE)
+        self.assertEqual(
+            self.obj.get_vote(JobDefinition({'id': 'froz'})),
+            IJobQualifier.VOTE_IGNORE)
         # Jobs that are in the retain set are NOT designated
         self.assertEqual(
             self.obj.designates(JobDefinition({'id': 'bar'})), False)
@@ -140,12 +144,22 @@
         SessionResumeHelper([]).resume(data)
         mocked_helper3.resume_json.assertCalledOnce()
 
-    def test_resume_dispatch_v4(self):
-        data = gzip.compress(
-            b'{"version":4}')
+    @mock.patch('plainbox.impl.session.resume.SessionResumeHelper4')
+    def test_resume_dispatch_v4(self, mocked_helper4):
+        data = gzip.compress(
+            b'{"session":{"desired_job_list":[],"jobs":{},"metadata":'
+            b'{"app_blob":null,"app_id":null,"flags":[],'
+            b'"running_job_name":null,"title":null'
+            b'},"results":{}},"version":4}')
+        SessionResumeHelper([]).resume(data)
+        mocked_helper4.resume_json.assertCalledOnce()
+
+    def test_resume_dispatch_v5(self):
+        data = gzip.compress(
+            b'{"version":5}')
         with self.assertRaises(IncompatibleSessionError) as boom:
             SessionResumeHelper([]).resume(data)
-        self.assertEqual(str(boom.exception), "Unsupported version 4")
+        self.assertEqual(str(boom.exception), "Unsupported version 5")
 
 
 class SessionResumeTests(TestCase):
@@ -1710,6 +1724,9 @@
             'jobs': {
                 job_a.id: job_a.checksum
             },
+            'desired_job_list': [
+                job_a.id
+            ],
             'results': {
                 job_a.id: [],
             }

=== modified file 'plainbox/plainbox/impl/session/test_suspend.py'
--- plainbox/plainbox/impl/session/test_suspend.py	2014-07-30 09:06:50 +0000
+++ plainbox/plainbox/impl/session/test_suspend.py	2014-10-08 18:01:17 +0000
@@ -28,6 +28,7 @@
 from unittest import TestCase
 import gzip
 
+from plainbox.abc import IJobResult
 from plainbox.impl.job import JobDefinition
 from plainbox.impl.result import DiskJobResult
 from plainbox.impl.result import IOLogRecord
@@ -37,6 +38,7 @@
 from plainbox.impl.session.suspend import SessionSuspendHelper1
 from plainbox.impl.session.suspend import SessionSuspendHelper2
 from plainbox.impl.session.suspend import SessionSuspendHelper3
+from plainbox.impl.session.suspend import SessionSuspendHelper4
 from plainbox.vendor import mock
 
 
@@ -686,3 +688,93 @@
             b'{"app_blob":null,"app_id":null,"flags":[],'
             b'"running_job_name":null,"title":null},"results":{}},'
             b'"version":3}'))
+
+
+class SessionSuspendHelper4Tests(SessionSuspendHelper3Tests):
+    """
+    Tests for various methods of SessionSuspendHelper4
+    """
+
+    def setUp(self):
+        self.helper = SessionSuspendHelper4()
+
+    def test_json_repr_current_version(self):
+        """
+        verify what the version field is
+        """
+        data = self.helper._json_repr(SessionState([]))
+        self.assertEqual(data['version'], 4)
+
+    def test_repr_SessionState_typical_session(self):
+        """
+        verify the representation of a SessionState with some unused jobs
+
+        Unused jobs should just have no representation. Their checksum
+        should not be mentioned. Their results (empty results) should be
+        ignored.
+        """
+        used_job = JobDefinition({
+            "plugin": "shell",
+            "id": "used",
+            "command": "echo 'hello world'",
+        })
+        unused_job = JobDefinition({
+            "plugin": "shell",
+            "id": "unused",
+            "command": "echo 'hello world'",
+        })
+        used_result = MemoryJobResult({
+            "io_log": [
+                (0.0, "stdout", b'hello world\n'),
+            ],
+            'outcome': IJobResult.OUTCOME_PASS
+        })
+        session_state = SessionState([used_job, unused_job])
+        session_state.update_desired_job_list([used_job])
+        session_state.update_job_result(used_job, used_result)
+        data = self.helper._repr_SessionState(session_state)
+        self.assertEqual(data, {
+            'jobs': {
+                'used': ('8c393c19fdfde1b6afc5b79d0a1617ecf7531cd832a16450dc'
+                         '2f3f50d329d373')
+            },
+            'results': {
+                'used': [{
+                    'comments': None,
+                    'execution_duration': None,
+                    'io_log': [[0.0, 'stdout', 'aGVsbG8gd29ybGQK']],
+                    'outcome': 'pass',
+                    'return_code': None
+                }]
+            },
+            'desired_job_list': ['used'],
+            'metadata': {
+                'title': None,
+                'flags': [],
+                'running_job_name': None,
+                'app_blob': None,
+                'app_id': None,
+            },
+        })
+
+    def test_suspend(self):
+        """
+        verify that the suspend() method returns gzipped JSON representation
+        """
+        data = self.helper.suspend(SessionState([]))
+        # XXX: we cannot really test what the compressed data looks like
+        # because apparently python3.2 gzip output is non-deterministic.
+        # It seems to be an instance of the gzip bug that was fixed a few
+        # years ago.
+        #
+        # I've filed a bug on python3.2 in Ubuntu and Python upstream project
+        # https://bugs.launchpad.net/ubuntu/+source/python3.2/+bug/871083
+        #
+        # In the meantime we can only test that we got bytes out
+        self.assertIsInstance(data, bytes)
+        # And that we can gzip uncompress them and get what we expected
+        self.assertEqual(gzip.decompress(data), (
+            b'{"session":{"desired_job_list":[],"jobs":{},"metadata":'
+            b'{"app_blob":null,"app_id":null,"flags":[],'
+            b'"running_job_name":null,"title":null},"results":{}},'
+            b'"version":4}'))

=== modified file 'plainbox/plainbox/impl/test_result.py'
--- plainbox/plainbox/impl/test_result.py	2014-02-17 16:25:52 +0000
+++ plainbox/plainbox/impl/test_result.py	2014-10-08 18:01:17 +0000
@@ -52,6 +52,7 @@
         self.assertIsNone(result.comments)
         self.assertEqual(result.io_log, ())
         self.assertIsNone(result.return_code)
+        self.assertTrue(result.is_hollow)
 
     def test_everything(self):
         result = DiskJobResult({
@@ -73,6 +74,7 @@
         self.assertEqual(result.comments, "it said blah")
         self.assertEqual(result.io_log, ((0, 'stdout', b'blah\n'),))
         self.assertEqual(result.return_code, 0)
+        self.assertFalse(result.is_hollow)
 
 
 class MemoryJobResultTests(TestCase):
@@ -85,6 +87,7 @@
         self.assertIsNone(result.comments)
         self.assertEqual(result.io_log, ())
         self.assertIsNone(result.return_code)
+        self.assertTrue(result.is_hollow)
 
     def test_everything(self):
         result = MemoryJobResult({
@@ -102,6 +105,7 @@
         self.assertEqual(result.comments, "it said blah")
         self.assertEqual(result.io_log, ((0, 'stdout', b'blah\n'),))
         self.assertEqual(result.return_code, 0)
+        self.assertFalse(result.is_hollow)
 
 
 class IOLogRecordWriterTests(TestCase):