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

Proposed by Aurélien Gâteau on 2012-04-02
Status: Rejected
Rejected by: Daniel Manrique on 2012-07-24
Proposed branch: lp:~agateau/checkbox/keep-tests-ordered2
Merge into: lp:checkbox
Diff against target: 154 lines (+40/-10) (has conflicts)
5 files modified
checkbox/lib/template.py (+2/-2)
checkbox/lib/template_i18n.py (+6/-1)
checkbox_gtk/gtk_interface.py (+3/-3)
plugins/jobs_info.py (+3/-1)
plugins/suites_prompt.py (+26/-3)
Text conflict in plugins/suites_prompt.py
To merge this branch: bzr merge lp:~agateau/checkbox/keep-tests-ordered2
Reviewer Review Type Date Requested Status
Daniel Manrique 2012-04-02 Disapprove on 2012-07-24
Greg Vallande (community) Needs Information on 2012-04-11
Review via email: mp+100378@code.launchpad.net

Description of the change

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.

To post a comment you must log in.
Daniel Manrique (roadmr) wrote :

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) wrote :

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

review: Needs Information
Daniel Manrique (roadmr) wrote :

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) wrote :

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.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'checkbox/lib/template.py'
2--- checkbox/lib/template.py 2012-03-15 14:46:06 +0000
3+++ checkbox/lib/template.py 2012-04-02 10:14:18 +0000
4@@ -70,11 +70,11 @@
5
6 def load_file(self, file, filename="<stream>"):
7 elements = []
8- for string in self._reader(file):
9+ for index, string in enumerate(self._reader(file)):
10 if not string:
11 break
12
13- element = {}
14+ element = {"sortkey": index}
15
16 def _save(field, value, extended):
17 extended = extended.rstrip("\n")
18
19=== modified file 'checkbox/lib/template_i18n.py'
20--- checkbox/lib/template_i18n.py 2012-03-14 21:11:06 +0000
21+++ checkbox/lib/template_i18n.py 2012-04-02 10:14:18 +0000
22@@ -18,6 +18,7 @@
23 #
24 import re
25 import locale
26+import types
27
28 from os import environ
29 from gettext import gettext as _
30@@ -116,7 +117,11 @@
31 continue
32
33 if key in element:
34- filter[key] = self._filter_field(element[key])
35+ value = element[key]
36+ if type(value) in types.StringTypes:
37+ filter[key] = self._filter_field(value)
38+ else:
39+ filter[key] = value
40 continue
41 else:
42 field = "_%s" % key
43
44=== modified file 'checkbox_gtk/gtk_interface.py'
45--- checkbox_gtk/gtk_interface.py 2012-03-14 21:11:06 +0000
46+++ checkbox_gtk/gtk_interface.py 2012-04-02 10:14:18 +0000
47@@ -409,9 +409,9 @@
48
49 def set_options(options, default, parent=None):
50 if isinstance(options, dict):
51- keys = sorted(options.keys())
52- values = [options[k] for k in keys]
53- for text, child in zip(keys, values):
54+ items = options.items()
55+ items.sort(key=lambda x: x[0].sortkey)
56+ for text, child in items:
57 active = text in default
58 iter = treestore.append(parent, [text, active])
59 set_options(child, default.get(text, {}), iter)
60
61=== modified file 'plugins/jobs_info.py'
62--- plugins/jobs_info.py 2012-03-21 12:33:48 +0000
63+++ plugins/jobs_info.py 2012-04-02 10:14:18 +0000
64@@ -46,7 +46,9 @@
65 "resources": List(String(), required=False),
66 "timeout": Int(required=False),
67 "user": String(required=False),
68- "data": String(required=False)})
69+ "data": String(required=False),
70+ "sortkey": Int(required=False, value=0),
71+ })
72
73
74 class JobsInfo(Plugin):
75
76=== modified file 'plugins/suites_prompt.py'
77--- plugins/suites_prompt.py 2012-03-20 22:31:41 +0000
78+++ plugins/suites_prompt.py 2012-04-02 10:14:18 +0000
79@@ -29,6 +29,20 @@
80
81 from gettext import gettext as _
82
83+class OptionString(str):
84+ """
85+ This class is used as a key in the dict passed to the interface to define
86+ options. It works like a string for all interfaces, but provide a "sortkey"
87+ attribute so that interface can opt-in to list tests according to the order
88+ defined by this key.
89+ "sortkey" is initialized from the "sortkey" job attribute.
90+ """
91+ def __init__(self, *args, **kwargs):
92+ super(OptionString, self).__init__(*args, **kwargs)
93+ self.sortkey = 0
94+
95+ def __repr__(self):
96+ return "<text: %s, sortkey: %d>" % (self, self.sortkey)
97
98 class SuitesPrompt(Plugin):
99
100@@ -42,8 +56,9 @@
101 def register(self, manager):
102 super(SuitesPrompt, self).register(manager)
103
104+ self._jobs = {}
105 self._depends = {}
106- self._jobs = {}
107+ self._visible_names = {}
108 self._statuses = {}
109 self._persist = None
110 self._recover = False
111@@ -77,13 +92,14 @@
112 self.store = store
113
114 def report_job(self, job):
115+ self._jobs[job["name"]] = job
116 if job.get("type") == "suite":
117 attribute = "description"
118 else:
119 attribute = "name"
120
121 if attribute in job:
122- self._jobs[job["name"]] = job[attribute]
123+ self._visible_names[job["name"]] = job[attribute]
124 if "suite" in job:
125 self._depends[job["name"]] = [job["suite"]]
126 if job.get("type") == "test":
127@@ -92,7 +108,7 @@
128 def prompt_gather(self, interface):
129 # Resolve dependencies
130 resolver = Resolver()
131- for key in self._jobs.iterkeys():
132+ for key in self._visible_names.iterkeys():
133 depends = self._depends.get(key, [])
134 resolver.add(key, *depends)
135
136@@ -109,11 +125,18 @@
137 suboptions = options
138 dependencies = resolver.get_dependencies(job)
139 for dependency in dependencies:
140+<<<<<<< TREE
141 if dependency in tests:
142 value = tests[dependency]["status"]
143 else:
144 value = self._statuses.get(dependency, {})
145 suboptions = suboptions.setdefault(self._jobs[dependency], value)
146+=======
147+ value = self._statuses.get(dependency, {})
148+ option = OptionString(self._visible_names[dependency])
149+ option.sortkey = self._jobs[dependency]["sortkey"]
150+ suboptions = suboptions.setdefault(option, value)
151+>>>>>>> MERGE-SOURCE
152
153 # Build defaults
154 defaults = self.persist.get("default")

Subscribers

People subscribed via source and target branches