Merge lp:~agateau/checkbox/keep-tests-ordered2 into lp:checkbox

[Aurélien Gâteau]

This is a second try at getting the keep-tests-ordered feature in. The first attempt got merged-in and rejected later, but I am not sure exactly why. If the changes there introduce some bugs I would be happy to fix them.

[Daniel Manrique (roadmr)]

Hi Aurélien:

Adding the "sortkey" element to a job caused unforeseen and serious consequences, such as being unable to submit results to certification.canonical.com; this is because the job schema has changed and the "receiving end" would need to be updated as well to cope with this.

However, since the "sortkey" is used only for visual sorting, we felt it incorrect to add support on the website, only to strip the key away upon receipt.

My apologies for not giving you a heads-up that the feature had been reverted (checkbox rev 1319); this had to be done in a bit of a hurry because it was blocking all submissions to certification, both by the certification team and OEM/QA. My aim was to reimplement that feature using a different technique but, while I have the basics of a new implementation that doesn't touch the jobs themselves, I haven't had the time to finish it.

I'll mark this as "Needs Fixing" as I notice you're still adding a sortkey to jobs. Let's coordinate so I can show you the progress I have and we can get this feature back in checkbox ASAP (which was my plan from the beginning, apologies again for not having had the time to finish this).

review: Needs Fixing

[Greg Vallande (gvallande)]

@roadmr: Has there been any movement on this one? Don't forget about this one, please.

review: Needs Information

[Daniel Manrique (roadmr)]

Hi,

I'm still working on an alternate implementation without using sortkey in the jobs. I'm trying to accomodate the two use cases we have, which are Aurélien's (without using a whitelist) and Certification's (with a whitelist).

[Daniel Manrique (roadmr)]

Hello,

This has been on hold for a while. Ordering in checkbox is a more complex affair than originally thought; everybody wants a different way of sorting, both visually and at run time, and we don't have a clear way of handling all use cases yet.

In any case, the way it's done in this code is, again, by adding the sortkey attribute to jobs which, as explained above, causes other problems. Based on this and the fact that ordering is still somewhat undefined, I'll have to reject this merge request, as it would be impossible to merge it as is. Thanks for your code and we'll keep trying to get ordering working right.

review: Disapprove

Unmerged revisions

1328. By Aurélien Gâteau on 2012-03-21

Make the gtk treeview lists tests in the order they are defined in job files

1327. By Aurélien Gâteau on 2012-03-21

Add a "sortkey" attribute to jobs

Will be used by the UI to present jobs in the order they appear in the job
definition file.

=== modified file 'checkbox/lib/template.py'
--- checkbox/lib/template.py	2012-03-15 14:46:06 +0000
+++ checkbox/lib/template.py	2012-04-02 10:14:18 +0000
@@ -70,11 +70,11 @@
 
     def load_file(self, file, filename="<stream>"):
         elements = []
-        for string in self._reader(file):
+        for index, string in enumerate(self._reader(file)):
             if not string:
                 break
 
-            element = {}
+            element = {"sortkey": index}
 
             def _save(field, value, extended):
                 extended = extended.rstrip("\n")
 
=== modified file 'checkbox/lib/template_i18n.py'
--- checkbox/lib/template_i18n.py	2012-03-14 21:11:06 +0000
+++ checkbox/lib/template_i18n.py	2012-04-02 10:14:18 +0000
@@ -18,6 +18,7 @@
 #
 import re
 import locale
+import types
 
 from os import environ
 from gettext import gettext as _
@@ -116,7 +117,11 @@
                     continue
 
             if key in element:
-                filter[key] = self._filter_field(element[key])
+                value = element[key]
+                if type(value) in types.StringTypes:
+                    filter[key] = self._filter_field(value)
+                else:
+                    filter[key] = value
                 continue
             else:
                 field = "_%s" % key
 
=== modified file 'checkbox_gtk/gtk_interface.py'
--- checkbox_gtk/gtk_interface.py	2012-03-14 21:11:06 +0000
+++ checkbox_gtk/gtk_interface.py	2012-04-02 10:14:18 +0000
@@ -409,9 +409,9 @@
 
         def set_options(options, default, parent=None):
             if isinstance(options, dict):
-                keys = sorted(options.keys())
-                values = [options[k] for k in keys]
-                for text, child in zip(keys, values):
+                items = options.items()
+                items.sort(key=lambda x: x[0].sortkey)
+                for text, child in items:
                     active = text in default
                     iter = treestore.append(parent, [text, active])
                     set_options(child, default.get(text, {}), iter)
 
=== modified file 'plugins/jobs_info.py'
--- plugins/jobs_info.py	2012-03-21 12:33:48 +0000
+++ plugins/jobs_info.py	2012-04-02 10:14:18 +0000
@@ -46,7 +46,9 @@
     "resources": List(String(), required=False),
     "timeout": Int(required=False),
     "user": String(required=False),
-    "data": String(required=False)})
+    "data": String(required=False),
+    "sortkey": Int(required=False, value=0),
+    })
 
 
 class JobsInfo(Plugin):
 
=== modified file 'plugins/suites_prompt.py'
--- plugins/suites_prompt.py	2012-03-20 22:31:41 +0000
+++ plugins/suites_prompt.py	2012-04-02 10:14:18 +0000
@@ -29,6 +29,20 @@
 
 from gettext import gettext as _
 
+class OptionString(str):
+    """
+    This class is used as a key in the dict passed to the interface to define
+    options. It works like a string for all interfaces, but provide a "sortkey"
+    attribute so that interface can opt-in to list tests according to the order
+    defined by this key.
+    "sortkey" is initialized from the "sortkey" job attribute.
+    """
+    def __init__(self, *args, **kwargs):
+        super(OptionString, self).__init__(*args, **kwargs)
+        self.sortkey = 0
+
+    def __repr__(self):
+        return "<text: %s, sortkey: %d>" % (self, self.sortkey)
 
 class SuitesPrompt(Plugin):
 
@@ -42,8 +56,9 @@
     def register(self, manager):
         super(SuitesPrompt, self).register(manager)
 
+        self._jobs = {}
         self._depends = {}
-        self._jobs = {}
+        self._visible_names = {}
         self._statuses = {}
         self._persist = None
         self._recover = False 
@@ -77,13 +92,14 @@
         self.store = store
 
     def report_job(self, job):
+        self._jobs[job["name"]] = job
         if job.get("type") == "suite":
             attribute = "description"
         else:
             attribute = "name"
 
         if attribute in job:
-            self._jobs[job["name"]] = job[attribute]
+            self._visible_names[job["name"]] = job[attribute]
             if "suite" in job:
                 self._depends[job["name"]] = [job["suite"]]
             if job.get("type") == "test":
@@ -92,7 +108,7 @@
     def prompt_gather(self, interface):
         # Resolve dependencies
         resolver = Resolver()
-        for key in self._jobs.iterkeys():
+        for key in self._visible_names.iterkeys():
             depends = self._depends.get(key, [])
             resolver.add(key, *depends)
 
@@ -109,11 +125,18 @@
             suboptions = options
             dependencies = resolver.get_dependencies(job)
             for dependency in dependencies:
+<<<<<<< TREE
                 if dependency in tests:
                     value = tests[dependency]["status"]
                 else:
                     value = self._statuses.get(dependency, {})
                 suboptions = suboptions.setdefault(self._jobs[dependency], value)
+=======
+                value = self._statuses.get(dependency, {})
+                option = OptionString(self._visible_names[dependency])
+                option.sortkey = self._jobs[dependency]["sortkey"]
+                suboptions = suboptions.setdefault(option, value)
+>>>>>>> MERGE-SOURCE
 
         # Build defaults
         defaults = self.persist.get("default")