1284405 - --socket-timeout option fails with "TypeError: a float is required"

Status: VERIFIED FIXED

(Remote Protocol :: Marionette, defect)

Description

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

Running marionette with the --socket-timeout option fails all the time with the following message:

$ marionette --binary path_to_fx --socket-timeout 10
Traceback (most recent call last):

  File "C:\mozilla\venv\lib\site-packages\marionette\runtests.py", line 69, in run
    runner.run_tests(tests)

  File "C:\mozilla\venv\lib\site-packages\marionette\runner\base.py", line 806, in run_tests
    if self.capabilities["browserName"] == "Fennec":

  File "C:\mozilla\venv\lib\site-packages\marionette\runner\base.py", line 593, in capabilities
    self.marionette.start_session()

  File "C:\mozilla\venv\lib\site-packages\marionette_driver\marionette.py", line 1075, in start_session
    self.socket_timeout)

  File "C:\mozilla\venv\lib\site-packages\marionette_driver\transport.py", line 136, in __init__
    self.sock.settimeout(self.socket_timeout)

  File "C:\Python27\Lib\socket.py", line 224, in meth
    return getattr(self._sock,name)(*args)

TypeError: a float is required

It looks like that we do not correctly pass through the option but internally handle it as string?

Comment 1

[Nelson João Morais (:njasm)]

hi. I Would like to work on this bug.
 
Would it be a viable approach (?) to add a simple check like:

        if args.socket_timeout is not None:
            try:
                args.socket_timeout = float(args.socket_timeout)
            except:
                self.error('--socket-timeout value ''%s'' must be convertible to float.' % args.socket_timeout)

after the verification of the arg jsdebugger in line: https://dxr.mozilla.org/mozilla-central/source/testing/marionette/harness/marionette/runner/base.py#444

Comment 2

[Andreas Tolfsen ❲:ato❳]

I really miss atof from stdlib.h here, since float() in Python appears to throw on “unsafe” input data.  Not convinced that is a feature.

Anyway, I think your approach looks sane.  My only comment would be to change the error message to adhere to how error messages are usually given on Unix:

    self.error("socket timeout must be convertible to float: %s" % args.socket_timeout)

Comment 3

[Nelson João Morais (:njasm)]

Attachment: Bug 1284405 - Better handling of --socket-timeout cli argument value.

Review commit: https://reviewboard.mozilla.org/r/64912/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/64912/

Comment 4

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

Why don't we instruct the argument parser to interpret the option value as float?

https://dxr.mozilla.org/mozilla-central/source/testing/marionette/harness/session/runner/base.py#313

Comment 5

[Andreas Tolfsen ❲:ato❳]

What whimboo suggests is probably a better idea.  I didn’t think of it.

njasm, can you make the necessary changes?  It should be possible to set

    type=float

as a keyword argument on where the --socket-timeout flag is defined.

Comment 6

[Nelson João Morais (:njasm)]

Yes, that's the way to do it. I've also missed that.

How can I assign myself to this bug?

later today I'll make the changes, test, and re-submit the patch.

Comment 7

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

We usually wait with the assignment until the patch has been uploaded. Once that has been done we can update the bug accordingly. 

Regarding testing I would propose that we also get a test added for the new marionette harness unittest suite:

https://dxr.mozilla.org/mozilla-central/source/testing/marionette/harness/marionette/tests/harness_unit/test_marionette_runner.py 

Maja should be able to help in case there are questions.

Comment 8

[Nelson João Morais (:njasm)]

Attachment: bug_1284405-fix_better_handling_of_--socket-timeout_arg.patch1

I'm uploading here the patch instead of pushing it to mozreview for feedback.

While working on the test, I've thought in creating a separated  test_marionette_arguments.py class next to test_marionette_runner.py, but i'm not sure if we need that already.

Comment 9

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

Comment on attachment 8772264 [details] [diff] [review]
bug_1284405-fix_better_handling_of_--socket-timeout_arg.patch1

I'm not familiar with pytest yet so I would forward the feedback request to Maja. But some comments you can find below:

>-import pytest
>+import pytest, sys

Please don't add multiple module imports in the same line. Also ensure to differentiate between global and local (venv) imports.

>+    # backup real argv
>+    argv_backup = sys.argv
>+    sys.argv = []
>+    sys.argv.append('marionette')
>+    sys.argv.append('--socket-timeout')
>+    sys.argv.append(sock_timeout_value)
>+    parser = MarionetteArguments()

I wish we wouldn't have to do it and that MarionetteArguments() would accept an argv argument optionally. It would make things way easier. I would let Maja speak for that given that she knows the harness well enough and how much work it would be to get implemented.

Comment 10

[Nelson João Morais (:njasm)]

I've looked into the MarionetteArguments class inheritance and we're able to pass in a list of args to parse_args() for processing. Sorry that i've missed that.

this will allow us to:

> Please don't add multiple module imports in the same line. Also ensure to
> differentiate between global and local (venv) imports.

No need to hack sys.argv anymore, so the import is not needed.


> I wish we wouldn't have to do it and that MarionetteArguments() would accept
> an argv argument optionally. It would make things way easier. I would let
> Maja speak for that given that she knows the harness well enough and how
> much work it would be to get implemented.

I'm changing the code to call BaseMarionetteArguments.parse_args() with testing args.
I'll upload a new patch here, nonetheless I'll wait for Maja's feedback on the new patch.

Comment 11

[Nelson João Morais (:njasm)]

Attachment: bug_1284405-fix_better_handling_of_--socket-timeout_arg.patch2

Comment 12

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8772383 [details] [diff] [review]
bug_1284405-fix_better_handling_of_--socket-timeout_arg.patch2

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

Thanks for adding a test. It's good overall and I've pointed out a few ways to improve it.

Regarding your question about separate test modules: :vakila is going to be reorganizing the tests soon anyway, so just keep your test in the same module.

::: testing/marionette/harness/marionette/tests/harness_unit/test_marionette_runner.py
@@ +217,5 @@
> +            return True
> +        except:
> +            return False
> +
> +    if _is_float_convertible(sock_timeout_value) is not True:

Change to `if not _is_float_convertible(sock_timeout_value)`

@@ +222,5 @@
> +        # should raising exception, since sock_timeout must be convertible to float.
> +        with pytest.raises(SystemExit) as ex:
> +            parser.parse_args(args=argv)
> +        assert ex.value is not None
> +        assert ex.value.message == 2

You can replace the two asserts with just `assert ex.code == 2`

@@ +225,5 @@
> +        assert ex.value is not None
> +        assert ex.value.message == 2
> +    else:
> +        # should pass without raising exception.
> +        parser = MarionetteArguments()

No need to re-instantiate MarionetteArguments()

@@ +230,5 @@
> +        args = parser.parse_args(args=argv)
> +        assert args is not None
> +        args_dic = vars(args)
> +        assert 'socket_timeout' in args_dic
> +        assert type(args_dic['socket_timeout']) is float

You can do something like `hasattr(args, 'socket_timeout')` and `args.socket_timeout == float(sock_timeout_value)`

Comment 13

[Nelson João Morais (:njasm)]

Maja, thanks for the feedback. 
I'm changing the test according to your feedback, but I'm getting:

 0:02.24 >           assert ex.code == 2
 0:02.24 E           AttributeError: 'ExceptionInfo' object has no attribute 'code'

according to pytest documentation in [1] there's no code attribute in ExceptionInfo. I've changed the code to ex.value.code == 2, that's probably what you wanted (?)

> @@ +222,5 @@
> > +        # should raising exception, since sock_timeout must be convertible to float.
> > +        with pytest.raises(SystemExit) as ex:
> > +            parser.parse_args(args=argv)
> > +        assert ex.value is not None
> > +        assert ex.value.message == 2
> 
> You can replace the two asserts with just `assert ex.code == 2`

I'll upload the patch with the requested changes, for your feedback once again if you don't mind.

thanks.

[1] http://doc.pytest.org/en/latest/_modules/_pytest/_code/code.html#ExceptionInfo

Comment 14

[Nelson João Morais (:njasm)]

Attachment: bug_1284405-fix_better_handling_of_--socket-timeout_arg.patch3

Comment 15

[Nelson João Morais (:njasm)]

Maja, thanks.

I'm ready to push the patch to mozreview, who can I request a review from?

Comment 16

[Andreas Tolfsen ❲:ato❳]

(In reply to Nelson João Morais (:njasm) from comment #15)
> I'm ready to push the patch to mozreview, who can I request a review from?

Most of the code appears harness related.  You can request a review from maja_zf.

Comment 17

[Nelson João Morais (:njasm)]

Comment on attachment 8771942 [details]
Bug 1284405 - Better handling of --socket-timeout cli argument value.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64912/diff/1-2/

Comment 18

[Nelson João Morais (:njasm)]

Comment on attachment 8771942 [details]
Bug 1284405 - Better handling of --socket-timeout cli argument value.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64912/diff/2-3/

Comment 19

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8771942 [details]
Bug 1284405 - Better handling of --socket-timeout cli argument value.

https://reviewboard.mozilla.org/r/64912/#review62692

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bc563c2d6901
Better handling of --socket-timeout cli argument value. r=maja_zf

Comment 21

[Carsten Book [:Tomcat]]

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

Comment 22

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

Nelson, thank you a lot for fixing that! It works perfectly and helps me a lot for my processes debugging work in Marionette without having to change appropriate code in transport.py.

Comment 23

[Nelson João Morais (:njasm)]

Henrik, I'm glad to know that. No need to thanks. I'm looking for other bugs to work on, if you know some open bugs that you might think that I'll probably be a good fit, just ping me in irc or so. I'm happy to help out.

Comment 24

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

Nelson, we definitely have bug 1259055 which can cause issues with our unit tests for Marionette. If you have interests to work on that I would kinda appreciate.