Closed
Bug 962495
Opened 11 years ago
Closed 11 years ago
[mozrunner] Calling runner.start() with a timeout or outputTimeout, it doesn't handle stopped process
Categories
(Testing :: Mozbase, defect)
Testing
Mozbase
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: whimboo, Assigned: jotes)
References
Details
(Whiteboard: [mentor=whimboo][lang=py])
Attachments
(2 files)
With my work on bug 960084 I have seen that mozrunner doesn't correctly handle the timeout, which can be given in runner.start(). Even the process has been killed, mozrunner assumes it is still running.
Attached you will find an example, which can be integrated later as test for mozrunner once the patch on bug 960084 has been landed.
Steps:
1. Export BROWSER_PATH and set it to your firefox binary
2. Run 'python runner.py'
The execution should not assert.
Reporter | ||
Comment 1•11 years ago
|
||
The same also applies to outputTimeout where it also fails to detect the stopped process.
Summary: [mozrunner] Calling runner.start() with a timeout doesn't handle stopped process → [mozrunner] Calling runner.start() with a timeout or outputTimeout, it doesn't handle stopped process
Reporter | ||
Updated•11 years ago
|
Whiteboard: [mentor=whimboo][lang=py]
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → jot
Assignee | ||
Comment 2•11 years ago
|
||
Currently we support only 2 types of process_handler - Popen, ProcessHandler.
Another, more clean method would be to implement poll() method in ProcessHandler to remove typecheck and just use natural duck-typing. I wasn't sure if we should go this way.
TIA for review :)
Attachment #8365619 -
Flags: review?(hskupin)
Reporter | ||
Comment 3•11 years ago
|
||
(In reply to Jarek Śmiejczak from comment #2)
> Another, more clean method would be to implement poll() method in
> ProcessHandler to remove typecheck and just use natural duck-typing. I
> wasn't sure if we should go this way.
Right. It's not only the situation for poll() but also for wait() and kill(), which I have fixed a couple of moments ago on bug 965183. So we should really move this into mozprocess. William, what is your opinion on that?
Flags: needinfo?(wlachance)
Comment 4•11 years ago
|
||
(In reply to Henrik Skupin (:whimboo) from comment #3)
> (In reply to Jarek Śmiejczak from comment #2)
> > Another, more clean method would be to implement poll() method in
> > ProcessHandler to remove typecheck and just use natural duck-typing. I
> > wasn't sure if we should go this way.
>
> Right. It's not only the situation for poll() but also for wait() and
> kill(), which I have fixed a couple of moments ago on bug 965183. So we
> should really move this into mozprocess. William, what is your opinion on
> that?
Looks like ProcessHandler / ProcessHandlerMixin already has wait and kill. Adding poll would be a natural extension.
Flags: needinfo?(wlachance)
Reporter | ||
Comment 5•11 years ago
|
||
Comment on attachment 8365619 [details] [diff] [review]
0001-Bug-962495-fixed-is_running-behaviour-in-mozrunner.-.patch
Review of attachment 8365619 [details] [diff] [review]:
-----------------------------------------------------------------
::: mozrunner/mozrunner/base.py
@@ +123,5 @@
> + if isinstance(self.process_handler, subprocess.Popen):
> + return self.process_handler.poll() is None
> + elif isinstance(self.process_handler, ProcessHandler):
> + return self.process_handler.proc.poll() is None
> + return False
I think it would be good if we would check for a valid process_handler first, so that we do not have to mix up the conditions. Also it would make it easier later if we are going to have such a wrapper in ProcessHandler itself.
For now I would be fine with it. But I would like to have another review by Andrew.
Attachment #8365619 -
Flags: review?(hskupin)
Attachment #8365619 -
Flags: review?(ahalberstadt)
Attachment #8365619 -
Flags: review+
Comment 6•11 years ago
|
||
Comment on attachment 8365619 [details] [diff] [review]
0001-Bug-962495-fixed-is_running-behaviour-in-mozrunner.-.patch
Review of attachment 8365619 [details] [diff] [review]:
-----------------------------------------------------------------
Looks good, though I guess if we are adding a poll() method to mozprocess in the other bug, we might as well block on that.
Attachment #8365619 -
Flags: review?(ahalberstadt) → review+
Assignee | ||
Comment 7•11 years ago
|
||
We can create new bug and set it as a blocker for this one or I can implement it now. I'm not sure who should take decide on that matter.
Reporter | ||
Comment 8•11 years ago
|
||
I filed bug 965326 for the follow-up work.
Reporter | ||
Comment 9•11 years ago
|
||
Landed on master:
https://github.com/mozilla/mozbase/commit/b7f3c9cc143770446f28c66ceb3da6412108e15b
Thanks Jarek!
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•