Closed Bug 1284405 Opened 6 years ago Closed 6 years ago

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

Categories

(Testing :: Marionette, defect)

49 Branch
defect
Not set
normal

Tracking

(firefox50 fixed)

VERIFIED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: whimboo, Assigned: njasm)

Details

(Whiteboard: [good first bug][lang=py])

Attachments

(2 files, 2 obsolete files)

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?
Whiteboard: [good first bug][lang=py]
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
Flags: needinfo?(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)
Flags: needinfo?(ato)
Attachment #8771942 - Flags: review?(ato)
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
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.
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.
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.
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.
Attachment #8771942 - Attachment is obsolete: true
Attachment #8771942 - Flags: review?(ato)
Attachment #8772264 - Flags: feedback?(hskupin)
Attachment #8772264 - Flags: feedback?(ato)
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.
Attachment #8772264 - Flags: feedback?(hskupin) → feedback?(mjzffr)
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.
Attachment #8772264 - Attachment is obsolete: true
Attachment #8772264 - Flags: feedback?(mjzffr)
Attachment #8772264 - Flags: feedback?(ato)
Attachment #8772383 - Flags: feedback?(mjzffr)
Attachment #8772383 - Flags: feedback?(ato)
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)`
Attachment #8772383 - Flags: feedback?(mjzffr) → feedback-
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
Attachment #8772383 - Attachment is obsolete: true
Attachment #8772383 - Flags: feedback?(ato)
Attachment #8772547 - Flags: feedback?(mjzffr)
Attachment #8772547 - Flags: feedback?(mjzffr) → feedback+
Maja, thanks.

I'm ready to push the patch to mozreview, who can I request a review from?
(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.
Assignee: nobody → njmorais
Status: NEW → ASSIGNED
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/
Attachment #8771942 - Attachment description: Bug 1284405 - Fix handling of --socket-timeout cli argument value. → Bug 1284405 - Better handling of --socket-timout cli argument value.
Attachment #8771942 - Attachment is obsolete: false
Attachment #8771942 - Flags: review?(mjzffr)
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/
Attachment #8771942 - Attachment description: Bug 1284405 - Better handling of --socket-timout cli argument value. → Bug 1284405 - Better handling of --socket-timeout cli argument value.
Pushed by mjzffr@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/bc563c2d6901
Better handling of --socket-timeout cli argument value. r=maja_zf
https://hg.mozilla.org/mozilla-central/rev/bc563c2d6901
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
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.
Status: RESOLVED → VERIFIED
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.
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.
You need to log in before you can comment on or make changes to this bug.