Closed Bug 975136 Opened 6 years ago Closed 6 years ago

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

Categories

(Testing :: Mozbase, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla30

People

(Reporter: dadeldev, Assigned: dadeldev)

References

()

Details

(Whiteboard: [mentor=whimboo][lang=python])

Attachments

(2 files, 1 obsolete file)

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.
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: ProcessHandler's parameters onTimeout and onFinish expect a function but accept only arrays → Update documentation for ProcessHandler's onTimeout and onFinish parameters to mention tuples
Whiteboard: [mentor=whimboo][lang=python]
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.
I don't see any harm in accepting a function in addition to a list.
So I propose a patch fixing both docs and code to accept a single function as well as a list.
Attachment #8379950 - Flags: review?(wlachance)
Comment on attachment 8379950 [details] [diff] [review]
Accepts both function and list for onTimeout and onFinish (fix code and docs)

lgtm!
Attachment #8379950 - Flags: review?(wlachance) → review+
https://hg.mozilla.org/mozilla-central/rev/daec597aca9c
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
A test would have been nice to have here, So that we can ensure it works. Dadel, would you mind to add that?
Assignee: nobody → dadeldev
Flags: in-testsuite?
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.
Attachment #8380372 - Flags: review+
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.
Attachment #8380372 - Flags: review+ → review?(wlachance)
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.
Attachment #8380372 - Flags: review?(wlachance) → review+
parans were kept for Python3 compatibility but spaces were removed
Attachment #8380372 - Attachment is obsolete: true
Attachment #8387090 - Flags: review+
Summary: Update documentation for ProcessHandler's onTimeout and onFinish parameters to mention tuples → Fix code and documentation for ProcessHandler's onTimeout and onFinish parameters to accept either single function or array
You need to log in before you can comment on or make changes to this bug.