965183 - [mozrunner] --interactive mode is broken

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[Andrei Eftimie]

Running mozmill with --manual currently fails:

> (mozmill)sl77-212:964318 andrei.eftimie$ mozmill -b /Applications/FirefoxNightly.app/ --manual
> Traceback (most recent call last):
>   File "/Users/andrei.eftimie/work/mozilla/mozmill/bin/mozmill", line 8, in <module>
>     load_entry_point('mozmill==2.1-dev', 'console_scripts', 'mozmill')()
>   File "/Users/andrei.eftimie/work/mozilla/mozmill/src/mozmill/mozmill/__init__.py", line 812, in cli
>     CLI(args).run()
>   File "/Users/andrei.eftimie/work/mozilla/mozmill/src/mozmill/mozmill/__init__.py", line 785, in run
>     mozmill.runner.wait()
>   File "/Users/andrei.eftimie/work/mozilla/mozbase/src/mozrunner/mozrunner/base.py", line 106, in wait
>     self.returncode = self.process_handler.wait(timeout)
> TypeError: wait() takes exactly 1 argument (2 given)

This works correctly with mozmill 2.0.3

Comment 1

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

Andrei, are you sure that you have mozrunner 5.31 installed? Looks like something is wrong with your setup here. Also you are running with mozmill version 2.1-dev and not on hotfix-2.0.

Comment 2

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

Ok, so I can only see this on OS X but not on Linux nor Windows. The failure appears a couple of seconds after Firefox has been opened. This is actually a mozrunner bug and caused by my refactoring on bug 960084.

Comment 3

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

So we actually have at least two issues here. The one as mentioned above and if you are trying to stop the application:

Exception TypeError: "kill() got an unexpected keyword argument 'sig'" in <bound method FirefoxRunner.__del__ of <mozrunner.local.FirefoxRunner object at 0x107584dd0>> ignored

That failure was already existent before my refactoring but we never noticed. I have seen this now while working on a test. So I will fix both and others if more appear.

Comment 4

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

Attachment: Patch v1

Fix for mozrunner's interactive mode and two more tests for the broken methods. This patch has been tested on OS X so far. While it's waiting for review I will the tests on Windows and Linux.

Comment 5

[Andrew Halberstadt [:ahal]]

Comment on attachment 8367324 [details] [diff] [review]
Patch v1

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

Thanks, lgtm.

::: mozrunner/mozrunner/base.py
@@ +98,5 @@
>  
>          """
>          if self.is_running():
> +            # The interactive mode uses directly a Popen process instance. It's
> +            # wait() method doesn't have any parameters. So handle it separately.

I don't think this comment is necessary, the isinstance call tells us the same thing.

@@ +135,5 @@
>          if not self.is_running():
>              return
>  
> +        # The interactive mode uses directly a Popen process instance. It's
> +        # kill() method doesn't have any parameters. So handle it separately.

Same.

Comment 6

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

The tests are passing on Linux and Windows and interactive mode works fine there.

Comment 7

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

(In reply to Andrew Halberstadt [:ahal] from comment #5)
> >          if self.is_running():
> > +            # The interactive mode uses directly a Popen process instance. It's
> > +            # wait() method doesn't have any parameters. So handle it separately.
> 
> I don't think this comment is necessary, the isinstance call tells us the
> same thing.

Well, I removed that if condition because it was not that clear to me. By just looking at the code you could also assume that kill() for Popen() has a timeout value but which defaults to None, so it waits forever. I think it is better to be explicit here. Shall I still remove those lines?

Comment 8

[Andrew Halberstadt [:ahal]]

I guess it doesn't matter, you can leave it if you want.

Comment 9

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

Attachment: Patch v1.1

While applying the patch git announced a white-space issue which I have fixed in this version.

Comment 10

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

https://github.com/mozilla/mozbase/commit/ec1fbe23a3041654739bd02b731aa17d954ba91e (master)