975136 - Fix code and documentation for ProcessHandler's onTimeout and onFinish parameters to accept either single function or array

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[dadel]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:27.0) Gecko/20100101 Firefox/27.0 (Beta/Release)
Build ID: 20140130123901

Steps to reproduce:

The mozprocess module provides ProcessHandler to launch commands.
As the documentation says (https://mozbase.readthedocs.org/en/latest/mozprocess.html), the parameters onTimeout and onFinish expect a function that will be called respectively when reaching a given timeout, or at the end of execution.

So the following code should work:

from mozprocess import processhandler
def timeout():
    print ("timed out!")

p = processhandler.ProcessHandler(['sleep','2'],onTimeout=timeout)
p.run(timeout=1)
p.wait()


And this should also work:

from mozprocess import processhandler
def finish():
    print ("finished!")

p = processhandler.ProcessHandler(['sleep', '1'], onFinish=finish)
p.run()
p.wait()



Actual results:

A TypeError is raised for each example code:

File "/.../mozprocess/processhandler.py", line 616, in __init__
    self.onTimeoutHandlers = list(onTimeout)
TypeError: 'function' object is not iterable


File "/.../mozprocess/processhandler.py", line 617, in __init__
    self.onFinishHandlers = list(onFinish)
TypeError: 'function' object is not iterable



Expected results:

According to the documentation, a single function should be accepted instead of a list and the previous code examples shouldn't fail.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Dadel, thanks a lot for the report. I have checked this and as it looks like the documentation is wrong here. Both parameters are tuples, so a change like this will work:

p = processhandler.ProcessHandler(['sleep','2'], onTimeout=(timeout,))

To fix this we would have to change the two parameter descriptions here:
http://mxr.mozilla.org/mozilla-central/source/testing/mozbase/mozprocess/mozprocess/processhandler.py#43

Would you have interest to get this fixed? I'm happy to mentor you.

Comment 2

[dadel]

Hi Henrik,

Yes I want to fix it. But I think it's better to accept both a function or a list of functions as it is the case for processOutputLine.

Comment 3

[Andrew Halberstadt [:ahal]]

I don't see any harm in accepting a function in addition to a list.

Comment 4

[dadel]

Attachment: Accepts both function and list for onTimeout and onFinish (fix code and docs)

Comment 5

[dadel]

So I propose a patch fixing both docs and code to accept a single function as well as a list.

Comment 6

[William Lachance (:wlach)]

Comment on attachment 8379950 [details] [diff] [review]
Accepts both function and list for onTimeout and onFinish (fix code and docs)

lgtm!

Comment 7

[William Lachance (:wlach)]

Inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/daec597aca9c

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/daec597aca9c

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2]]

A test would have been nice to have here, So that we can ensure it works. Dadel, would you mind to add that?

Comment 10

[dadel]

Henrik, I wrote unit tests for these two and params and also for processOutputLine (fixed in Bug 964506).
I don't know if I should file a new bug for that so I attach it here even if it concerns an other bug also.

Comment 11

[dadel]

Attachment: Test params processOutputLine, onTimeout and onFinish

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8380372 [details] [diff] [review]
Test params processOutputLine, onTimeout and onFinish

Review of attachment 8380372 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Dadel for the tests! It would be really good to have those. Lets get the tests reviewed by William who already reviewed your implementation patch.

Comment 13

[William Lachance (:wlach)]

Comment on attachment 8380372 [details] [diff] [review]
Test params processOutputLine, onTimeout and onFinish

Review of attachment 8380372 [details] [diff] [review]:
-----------------------------------------------------------------

To be honest I feel like these tests are kind of overkill, but I guess it was suggested that you write them and they don't do much harm. :P r+, will commit later today.

::: testing/mozbase/mozprocess/tests/test_mozprocess_params.py
@@ +12,5 @@
> +
> +    def test_process_outputline_handler(self):
> +        """Parameter processOutputLine is accepted with a single function"""
> +        def output(line):
> +            print ("output " + str(line))

We should probably omit the parans around the print statement, for consistency with the rest of mozbase. Other than that, looks fine.

Comment 14

[dadel]

Attachment: TestParamsOutputlineOntimeoutOnfinish.patch

parans were kept for Python3 compatibility but spaces were removed

Comment 15

[William Lachance (:wlach)]

Inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/d250a7f22fef

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d250a7f22fef