Differential D104

Make sure python directive returns a string
ClosedPublic
Actions

Authored by lbrabec on May 22 2014, 9:46 AM.

Details

Reviewers
kparal
tflink
mkrizek
jskladan
Maniphest Tasks
T95: Make task output in the runner more readable
Commits
rLTRNd4de0b2847ec: Make sure python directive returns a string
Summary

Quick fix for T95 was introduced in D47, this is proper one. Callable task method must now return string.

Test Plan

$ python runtask.py -i xchat-2.8.8-21.fc20 -t koji_build -a x86_64 ../task-rpmlint/rpmlint.yml
$ python runtask.py -i tigervnc-1.3.0-14.fc20 -t koji_build -a x86_64 ../task-examplebodhi/examplebodhi.yml

Diff Detail

Repository
rLTRN libtaskotron
Lint
Lint Skipped
Unit
Unit Tests Skipped
tflink added a comment.May 22 2014, 12:17 PM

I'm not sure I understand the reason for enforcing the return of a single string from the python directive other than making sure it's consistent.

Can you elaborate a bit on why enforcing a single string return is the best solution here when other directives return non-strings?

kparal added a comment.May 28 2014, 2:09 PM

Tim, thanks for the comment. I thought about it and I realized several important things.

My initial motivation was to make check output consistent and my reasoning was like this:

  1. Checks are expected to return TAP.
  2. TAP is a string. It can't be a list, it doesn't make much sense. If it were a list, should we simply join it by newlines? Or would it be a list of individual TAP results, so that we would have to join them in a proper TAP manner (delimiters and such) and then generate the TAP header? It's too complicated, too much to explain, too much to document, without any clear benefit. Therefore TAP is a string. Simple, expected.
  3. Checks are run through the python directive.
  4. Outcome: The python directive must return a string and we should enforce it.

However, I realized:
a) We currently have no documentation at all regarding directive output. Different directives can return a range variety of data, and there's no way to knowing what data unless you read the source code. We should put return data documentation into the directive docstring in the very least.
b) The check doesn't have to be executed by the python directive. That is not true at the moment, but can/should easily become true in the future (bash checks, for example).
c) The python directives doesn't have to execute checks. It can be some other code, for example some environment preparation scripts (and then, in another python directive can the actual check be run).
d) We currently print to stdout anything that we find in Runner.working_data and assume they are check results. That's a bit shortsighted, because working_data should be used for arbitrary data transfer, not just check results. We will need to change that as well and find a better way to distinguishing check results from other data.

This is just a food for thought at the moment, we won't solve all of that here, we will need to file individual tickets for these issue. Here, I think we could start with the basics:

  1. This patch should be modified to allow str and None values as python directive output. That covers the most basic needs (TAP output and not-a-check python script).
  2. The python directive's docstring should be adjusted to reflect that.

If you agree with the core problems mentioned, I would create individual tickets for them. This patch might get reverted once we solve them, but in the mean time, it would ensure consistent and valid check output. What do you think?

kparal retitled this revision from Proper fix for T95 to Make sure python directive returns a string.May 28 2014, 2:21 PM
tflink requested changes to this revision.May 30 2014, 3:53 PM

TAP is a string. It can't be a list, it doesn't make much sense. If it were a list, should we simply join it by newlines? Or would it be a list of individual TAP results, so that we would have to join them in a proper TAP manner (delimiters and such) and then generate the TAP header? It's too complicated, too much to explain, too much to document, without any clear benefit. Therefore TAP is a string. Simple, expected.
Checks are run through the python directive.
Outcome: The python directive must return a string and we should enforce it.

OK, that makes sense for the time being, at least. Are our reporting directives capable of understanding multiple TAP results concatenated together? Do we currently have any checks that output multiple TAP streams? I can't think of any off the top of my head.

a) We currently have no documentation at all regarding directive output. Different directives can return a range variety of data, and there's no way to knowing what data unless you read the source code. We should put return data documentation into the directive docstring in the very least.

Yeah, that's a problem. I think that it's partially covered by T183 and T142 but there's a lot of stuff not covered by those two tickets.

b) The check doesn't have to be executed by the python directive. That is not true at the moment, but can/should easily become true in the future (bash checks, for example).
c) The python directives doesn't have to execute checks. It can be some other code, for example some environment preparation scripts (and then, in another python directive can the actual check be run).
d) We currently print to stdout anything that we find in Runner.working_data and assume they are check results. That's a bit shortsighted, because working_data should be used for arbitrary data transfer, not just check results. We will need to change that as well and find a better way to distinguishing check results from other data.

These are good points. Thanks for bringing them up.

This is just a food for thought at the moment, we won't solve all of that here, we will need to file individual tickets for these issue. Here, I think we could start with the basics:

Yeah, I think that we need to spend some time making sure that our internals are at least reasonably consistent and documented. I suspect that the directive docs will go a ways toward making this better but that's getting a bit off topic for a code review :)

This patch should be modified to allow str and None values as python directive output. That covers the most basic needs (TAP output and not-a-check python script). The python directive's docstring should be adjusted to reflect that.

Agreed. @lbrabec, would ypu update the revision to do this?

If you agree with the core problems mentioned, I would create individual tickets for them. This patch might get reverted once we solve them, but in the mean time, it would ensure consistent and valid check output. What do you think?

Sounds like a plan to me.

This revision now requires changes to proceed.May 30 2014, 3:53 PM
lbrabec updated this revision to Diff 340.Jun 2 2014, 9:56 AM
lbrabec edited the test plan for this revision. (Show Details)
  • python directive can return string or none
lbrabec updated this revision to Diff 341.Jun 2 2014, 10:02 AM
  • fixing bad commit
kparal requested changes to this revision.Jun 2 2014, 10:22 AM

Are all of those test_python_directive.py changes still necessary when we allowed None to be returned?

Can you please add a new unit tests that verifies that TaskotronDirectiveError gets raised when you try to return something else than None and basestring? (Be sure to also test corner cases like an empty string, or an empty list). Thanks.

libtaskotron/directives/python_directive.py
69

You should use if output is not None. Otherwise an empty list (for example) would not get checked.

70–73

This could easily fit on two lines, but I don't want to talk too much into your code structuring habits :-) What interests me, though, are the redundant str() calls I see lately. If there was just "%s" % output and output could be anything, then yes, if output was a tuple, then it could break the formatting. But since you already call type() on it and you know what gets returned (a type), you no longer need to manually cast it to string, that's what %s does for you.

Just a nitpick.

libtaskotron/runner.py
200–203

It seems your formatting is somehow broken (no whitespace between variable name and value?).

I'd suggest a bit more readable approach (not tested):

log.info('Check execution finished. Showing stored variables:')
for name, value in task_runner.working_data.items():
    log.info("%${s}:\n%s" % (name, value))
This revision now requires changes to proceed.Jun 2 2014, 10:22 AM
lbrabec updated this revision to Diff 342.Jun 2 2014, 11:24 AM
  • new unit tests and code polishing
kparal accepted this revision.Jun 3 2014, 9:16 AM
kparal added inline comments.
libtaskotron/runner.py
203

Thanks for correcting my typo :-)

lbrabec closed this revision.Jun 3 2014, 9:27 AM
lbrabec updated this revision to Diff 345.

Closed by commit rLTRNd4de0b2847ec (authored by @lbrabec).

Revision Contents

Diff 345

View Options

libtaskotron/directives/python_directive.py

Show First 20 Lines • Show All 60 Lines • ▼ Show 20 Line(s)56 def execute(self, task_module, method_name, kwargs):
61 @param kwargs kwargs to pass as parameters into the callable 61​ @param kwargs kwargs to pass as parameters into the callable
62 @return output from the executed method 62​ @return output from the executed method
63 """ 63​ """
64 64​
65 task_method = self._do_getattr(task_module, method_name) 65​ task_method = self._do_getattr(task_module, method_name)
66 66​
67 output = task_method(**kwargs) 67​ output = task_method(**kwargs)
68 68​
69​ if output is not None and not isinstance(output, basestring):
kparalUnsubmitted
Not Done

You should use if output is not None. Otherwise an empty list (for example) would not get checked.

70​ raise TaskotronDirectiveError(
71​ "Callable task method must return string or None, actual "
72​ "returned type: %s" % type(output))
73​
kparalUnsubmitted
Not Done

This could easily fit on two lines, but I don't want to talk too much into your code structuring habits :-) What interests me, though, are the redundant str() calls I see lately. If there was just "%s" % output and output could be anything, then yes, if output was a tuple, then it could break the formatting. But since you already call type() on it and you know what gets returned (a type), you no longer need to manually cast it to string, that's what %s does for you.

Just a nitpick.

69 return output 74​ return output
70 75​
71 def load_pyfile(self, filename): 76​ def load_pyfile(self, filename):
72 """Import python code specified 77​ """Import python code specified
73 78​
74 @param filename absolute path to the python file 79​ @param filename absolute path to the python file
75 """ 80​ """
76 81​
▲ Show 20 Lines • Show All 48 Lines • Show Last 20 Lines
View Options

libtaskotron/runner.py

Show First 20 Lines • Show All 191 Lines • ▼ Show 20 Line(s)183def main():
192 192​
193 task_data = taskyaml.parse_yaml_from_file(arg_data['taskfile']) 193​ task_data = taskyaml.parse_yaml_from_file(arg_data['taskfile'])
194 194​
195 if not task_data: 195​ if not task_data:
196 raise TaskotronYamlError('Input file should not be empty') 196​ raise TaskotronYamlError('Input file should not be empty')
197 197​
198 task_runner = Runner(task_data, arg_data) 198​ task_runner = Runner(task_data, arg_data)
199 task_runner.run() 199​ task_runner.run()
200 200​
201 for output in task_runner.working_data: 201​ log.info('Check execution finished. Showing stored variables:')
202 if python_utils.sequence(task_runner.working_data[output], basestring): 202​ for name, value in task_runner.working_data.items():
203 log.info("\n" + "\n".join(task_runner.working_data[output])) 203​ log.info("${%s}:\n%s" % (name, value))
kparalUnsubmitted
Not Done

It seems your formatting is somehow broken (no whitespace between variable name and value?).

I'd suggest a bit more readable approach (not tested):

log.info('Check execution finished. Showing stored variables:')
for name, value in task_runner.working_data.items():
    log.info("%${s}:\n%s" % (name, value))
kparalUnsubmitted
Not Done

Thanks for correcting my typo :-)

204 else: ​
205 log.info("\n" + task_runner.working_data[output]) ​
View Options

testing/test_python_directive.py

1from libtaskotron.directives import python_directive 1​from libtaskotron.directives import python_directive
2​from libtaskotron.exceptions import TaskotronDirectiveError
2from dingus import Dingus 3​from dingus import Dingus
3import os 4​import os
4import imp 5​import imp
5import pytest 6​import pytest
6import copy 7​import copy
7 8​
8 9​
9class TestCheckfile(object): 10​class TestCheckfile(object):
Show All 35 Lines
45 46​
46class TestExecutePyfile(object): 47​class TestExecutePyfile(object):
47 48​
48 def test_task_method_is_found(self, monkeypatch): 49​ def test_task_method_is_found(self, monkeypatch):
49 ref_modulename = 'foo' 50​ ref_modulename = 'foo'
50 ref_methodname = 'bar' 51​ ref_methodname = 'bar'
51 ref_kwargs = {'baz': 'why', 'blah': 2} 52​ ref_kwargs = {'baz': 'why', 'blah': 2}
52 53​
53 stub_getattr = Dingus() 54​ stub_getattr = Dingus(return_value=lambda baz, blah: "str")
54 55​
55 test_directive = python_directive.PythonDirective() 56​ test_directive = python_directive.PythonDirective()
56 monkeypatch.setattr(test_directive, '_do_getattr', stub_getattr) 57​ monkeypatch.setattr(test_directive, '_do_getattr', stub_getattr)
57 58​
58 test_directive.execute(ref_modulename, ref_methodname, ref_kwargs) 59​ test_directive.execute(ref_modulename, ref_methodname, ref_kwargs)
59 60​
60 assert (stub_getattr.calls()[0].name, stub_getattr.calls()[0].args) == ( 61​ assert (stub_getattr.calls()[0].name, stub_getattr.calls()[0].args) == (
61 '()', (ref_modulename, ref_methodname)) 62​ '()', (ref_modulename, ref_methodname))
62 63​
63 def test_task_method_is_executed(self, monkeypatch): 64​ def test_task_method_is_executed(self, monkeypatch):
64 ref_modulename = 'foo' 65​ ref_modulename = 'foo'
65 ref_methodname = 'bar' 66​ ref_methodname = 'bar'
66 ref_kwargs = {'baz': 'why', 'blah': 2} 67​ ref_kwargs = {'baz': 'why', 'blah': 2}
67 68​
68 stub_getattr = Dingus() 69​ stub_task = Dingus(return_value="str")
70​ stub_getattr = Dingus(return_value=stub_task)
69 71​
70 test_directive = python_directive.PythonDirective() 72​ test_directive = python_directive.PythonDirective()
71 monkeypatch.setattr(test_directive, '_do_getattr', stub_getattr) 73​ monkeypatch.setattr(test_directive, '_do_getattr', stub_getattr)
72 74​
73 test_directive.execute(ref_modulename, ref_methodname, ref_kwargs) 75​ test_directive.execute(ref_modulename, ref_methodname, ref_kwargs)
74 76​
75 assert (stub_getattr.calls()[1].name, stub_getattr.calls()[1].args, 77​ assert (stub_task.calls()[0].name, stub_task.calls()[0].args,
76 stub_getattr.calls()[1].kwargs) == ('()', (), ref_kwargs) 78​ stub_task.calls()[0].kwargs) == ('()', (), ref_kwargs)
79​
80​
81​class TestPyfileOutputException(object):
82​
83​ def test_task_doesnt_rise_on_unicode_str(self, monkeypatch):
84​ ref_modulename = 'foo'
85​ ref_methodname = 'bar'
86​ ref_kwargs = {'baz': 'why', 'blah': 2}
87​
88​ stub_task = Dingus(return_value=u"unicode")
89​ stub_getattr = Dingus(return_value=stub_task)
90​
91​ test_directive = python_directive.PythonDirective()
92​ monkeypatch.setattr(test_directive, '_do_getattr', stub_getattr)
93​
94​ test_directive.execute(ref_modulename, ref_methodname, ref_kwargs)
95​
96​ assert (stub_task.calls()[0].name, stub_task.calls()[0].args,
97​ stub_task.calls()[0].kwargs) == ('()', (), ref_kwargs)
98​
99​ def test_task_doesnt_rise_on_empty_str(self, monkeypatch):
100​ ref_modulename = 'foo'
101​ ref_methodname = 'bar'
102​ ref_kwargs = {'baz': 'why', 'blah': 2}
103​
104​ stub_task = Dingus(return_value="")
105​ stub_getattr = Dingus(return_value=stub_task)
106​
107​ test_directive = python_directive.PythonDirective()
108​ monkeypatch.setattr(test_directive, '_do_getattr', stub_getattr)
109​
110​ test_directive.execute(ref_modulename, ref_methodname, ref_kwargs)
111​
112​ assert (stub_task.calls()[0].name, stub_task.calls()[0].args,
113​ stub_task.calls()[0].kwargs) == ('()', (), ref_kwargs)
114​
115​ def test_task_doesnt_rise_on_none(self, monkeypatch):
116​ ref_modulename = 'foo'
117​ ref_methodname = 'bar'
118​ ref_kwargs = {'baz': 'why', 'blah': 2}
119​
120​ stub_task = Dingus(return_value=None)
121​ stub_getattr = Dingus(return_value=stub_task)
122​
123​ test_directive = python_directive.PythonDirective()
124​ monkeypatch.setattr(test_directive, '_do_getattr', stub_getattr)
125​
126​ test_directive.execute(ref_modulename, ref_methodname, ref_kwargs)
127​
128​ assert (stub_task.calls()[0].name, stub_task.calls()[0].args,
129​ stub_task.calls()[0].kwargs) == ('()', (), ref_kwargs)
130​
131​ def test_task_rises_on_empty_list(self, monkeypatch):
132​ ref_modulename = 'foo'
133​ ref_methodname = 'bar'
134​ ref_kwargs = {'baz': 'why', 'blah': 2}
135​
136​ stub_task = Dingus(return_value=[])
137​ stub_getattr = Dingus(return_value=stub_task)
138​
139​ test_directive = python_directive.PythonDirective()
140​ monkeypatch.setattr(test_directive, '_do_getattr', stub_getattr)
141​
142​ with pytest.raises(TaskotronDirectiveError):
143​ test_directive.execute(ref_modulename, ref_methodname, ref_kwargs)
144​
145​ def test_task_rises_on_wrong_type(self, monkeypatch):
146​ ref_modulename = 'foo'
147​ ref_methodname = 'bar'
148​ ref_kwargs = {'baz': 'why', 'blah': 2}
149​
150​ stub_task = Dingus(return_value={'han': 'shot', 'first': True})
151​ stub_getattr = Dingus(return_value=stub_task)
152​
153​ test_directive = python_directive.PythonDirective()
154​ monkeypatch.setattr(test_directive, '_do_getattr', stub_getattr)
155​
156​ with pytest.raises(TaskotronDirectiveError):
157​ test_directive.execute(ref_modulename, ref_methodname, ref_kwargs)
77 158​
78 159​
79class TestProcess(object): 160​class TestProcess(object):
80 161​
81 def setup_method(self, method): 162​ def setup_method(self, method):
82 163​
83 self.ref_taskyaml = '/path/to/foo/footask.yml' 164​ self.ref_taskyaml = '/path/to/foo/footask.yml'
84 self.ref_input = {'file': 'foo.py', 'callable': 'bar', 'kwarg1': 'baz'} 165​ self.ref_input = {'file': 'foo.py', 'callable': 'bar', 'kwarg1': 'baz'}
▲ Show 20 Lines • Show All 52 Lines • ▼ Show 20 Line(s)209 def test_process_executes(self, monkeypatch):
137 218​
138 test_directive.process(copy.copy(self.ref_input), 219​ test_directive.process(copy.copy(self.ref_input),
139 self.ref_env) 220​ self.ref_env)
140 221​
141 execute_call = self.stub_execute.calls[0] 222​ execute_call = self.stub_execute.calls[0]
142 223​
143 assert (execute_call.name, execute_call.args, execute_call.kwargs) == ( 224​ assert (execute_call.name, execute_call.args, execute_call.kwargs) == (
144 '()', (self.stub_imported, self.ref_input['callable'], ref_kwargs), {}) 225​ '()', (self.stub_imported, self.ref_input['callable'], ref_kwargs), {})
145No newline at end of file 226​No newline at end of file