1315704 - [marionette harness tests] Test that BaseMarionetteArguments constructs arguments as expected

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect, P5)

Description

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

We need some tests that parse a list of arguments (like [1]) and then verify that the resulting parsed arguments are successfully processed by _build_kwargs [2].

This is to prevent regressions like Bug 1315522.

For example, when arguments ['marionette', '--addon', 'something'] are passed to the parser and then onto _build_kwargs, then the dictionary produced by _build_kwargs should include {'addons': 'something'}.

For an example of a similar test, see [3], but for this bug the arguments passed to 'build_kwargs_using' should be constructed via a call to BaseMarionetteArguments.

[1] https://dxr.mozilla.org/mozilla-central/rev/8e8b146fcb8b268e3c09b646087c6b2ef9f0af6f/testing/marionette/harness/marionette/tests/harness_unit/test_marionette_arguments.py#23

[2] https://dxr.mozilla.org/mozilla-central/rev/8e8b146fcb8b268e3c09b646087c6b2ef9f0af6f/testing/marionette/harness/marionette/runner/base.py#699

[3] https://dxr.mozilla.org/mozilla-central/rev/8e8b146fcb8b268e3c09b646087c6b2ef9f0af6f/testing/marionette/harness/marionette/tests/harness_unit/test_marionette_runner.py#132-138


Info for new contributors: 
* How to set up dev environment: https://wiki.mozilla.org/User:Mjzffr/New_Contributors
* How to run marionette harness tests: https://developer.mozilla.org/en-US/docs/Mozilla/QA/Marionette/Developer_setup#Marionette_Harness_Unit_Tests

Comment 1

[Tomislav Jurin]

If someone can mentor this I would like to try. Thanks

Comment 2

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

Hi svezauzeto12, I'm available to mentor. Please go ahead and get started based on the info I provide in Comment 0. 

If you have any questions as you proceed, either flag me here in the bug by checking the "Need more information from ..." field or ping me on IRC in #ateam or #automation.

Thanks in advance for working on this. :)

Comment 3

[Tomislav Jurin]

Okay I will give it a try :)

Thanks :)

Comment 4

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

Hi Tomislav, are you still working on this? Do you have any questions?

Comment 5

[Tomislav Jurin]

Hi Maja, tbh I forgot about this bug since I didn't have it listed in "My bugs". Of course that this is not an excuse , just mistake from my side.

If it is still not to late I will try to solve it now, if this is okay with you ? 

Thanks

Comment 6

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

Yes, go ahead. :) I will assign this bug to you once you submit a patch for review. If you decide not to work on it, please let us know by commenting on the bug. That way other volunteers can pick it up.

Regarding needinfo: if you leave a question for me here, please check the box "Need more information from", and set my id, not yours, so that I get a notification.

Comment 7

[Tomislav Jurin]

Hi Maja,

I was thinking about this one and I am kinda stuck on this:

""" but for this bug the arguments passed to 'build_kwargs_using' should be constructed via a call to BaseMarionetteArguments."""

is there any documentation on for this, because I have no idea how to call BaseMarionneteArguments.

Thank you

Comment 8

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

Hi Tomislav, 

There's no documentation about this part of Marionette harness, so your best option in general is to search through the code using a tool like https://dxr.mozilla.org

If you search for 'class BaseMarionetteArguments' you'll find that it's a child of the standard `argparse.ArgumentParser` class and it it uses that `parse_args` method [0].

In the case of `build_kwargs_using` [1], you likely need to make a new fixture that works the same way but uses BaseMarionetteArguments directly instead of using `mach_parsed_kwargs` (which is just a dict of previously parsed data). 

So your new fixture will instantiate BaseMarionetteArguments (`parser = BaseMarionetteArguments()`) then use it parse an arbitrary list of arguments (`parsed_args = parser.parse_args(some_list_of_args_that_you_are_testing)`) to call MarionetteTestRunner, etc. Note that it's important to call verify_usage after parse_args [2].

Ultimately, your goal is to be able to write a test somewhat like this:

```
def test_a_thing(fixture1, fixture2, build_kwargs_with_parser_using):
    # build_kwargs_with_parser_using fixture doesn't exist yet, you have to write it
    # cause BaseMarionetteArguments.parse_args and MarionetteTestRunner._build_kwargs to be called with some test arguments
    built_kwargs = build_kwargs_with_parser_using(['marionette', '--addon', 'something'])
    # check that args got processed as expected, i.e. built_kwargs contains {'addon': 'something'}

I hope that helps. Try to get this idea working and show me your work in progress by submitting patch -- I can give you some preliminary feedback. If you can think of a better way to test this, feel free to suggest that, too. :)


[0] https://docs.python.org/2/library/argparse.html#parsing-arguments

[1] https://dxr.mozilla.org/mozilla-central/rev/6dccae211ae5fec6a1c1244b878ce0b93860154f/testing/marionette/harness/marionette_harness/tests/harness_unit/test_marionette_runner.py#38-49

[2] Example of using BaseMarionetteArguments in actual harness: https://dxr.mozilla.org/mozilla-central/rev/6dccae211ae5fec6a1c1244b878ce0b93860154f/testing/marionette/harness/marionette_harness/runtests.py#54-55

Comment 9

[Tomislav Jurin]

Yes this is great explanation. Just one more question before I commit patch, where should I put my test ? 
In base.py or ?

Thanks

Comment 10

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

The all the test code and fixtures should go in testing/marionette/harness/marionette_harness/tests/harness_unit/test_marionette_runner.py

Comment 11

[Darshan Chaudhary]

Can I work on this?

Comment 12

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

Yes, go ahead.

Comment 13

[Jeremy Lam]

Hi Maja,

I am going to give this one a shot. I will update you with questions as I get stuck in.

Comment 14

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

Sounds good. Need-info me when you have questions.

Comment 15

[Leni Kadali [:leni1] [GMT+3]]

Attachment: test_bug_1315704.py

The attachment is a draft showing my approach. Once I've fine-tuned, then I will modify test_marionette_runner.py directly. 

I'm a little confused about how to incorporate your reference to the code here [1] into the test. Pointers would be appreciated. 
Also what arguments would I be passing? The list ['marionette', '--addon', 'something'] seems to be a template 

https://dxr.mozilla.org/mozilla-central/rev/6dccae211ae5fec6a1c1244b878ce0b93860154f/testing/marionette/harness/marionette_harness/tests/harness_unit/test_marionette_runner.py#38-49

Comment 16

[Leni Kadali [:leni1] [GMT+3]]

> The attachment is a draft showing my approach. Once I've fine-tuned, then I
> will modify test_marionette_runner.py directly. 
Unfortunately, attaching this as an attachment makes you download the file, which I didn't anticipate. 
So here's the draft code:
@pytest.fixture
def test_build_kwargs_parser_using(fixture1, fixture2, build_kwargs_with_parser_using):
    def build_kwargs_with_parser_using(parsed_args):
       parser = MarionetteArguments()
       argv = ['marionette', '--addon', 'something']
       parsed_args = parser.parse_args(argv)
       parser.verify_usage(parsed_args)
       return parsed_args

Comment 17

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

Hi Leni, glad to see you working on this. :)

> I'm a little confused about how to incorporate your reference to the code here [1] into the test. Pointers would be appreciated. 

So [1] is an example of a pytest fixture (see https://docs.pytest.org/en/latest/fixture.html#fixtures-as-function-arguments). I don't expect you will need to use it directly in your test; instead you likely need to write a similar fixture.

> Also what arguments would I be passing? The list ['marionette', '--addon', 'something'] seems to be a template.

The above list is an example based on Bug 1315522. You can use that list as is. Once you have a working test, we can discuss expanding it to work with more examples of arguments.

(In reply to Leni Kadali from comment #16)
> > The attachment is a draft showing my approach. Once I've fine-tuned, then I
> > will modify test_marionette_runner.py directly. 
> Unfortunately, attaching this as an attachment makes you download the file,
> which I didn't anticipate. 
> So here's the draft code:
> @pytest.fixture
> def test_build_kwargs_parser_using(fixture1, fixture2,
> build_kwargs_with_parser_using):
>     def build_kwargs_with_parser_using(parsed_args):
>        parser = MarionetteArguments()
>        argv = ['marionette', '--addon', 'something']
>        parsed_args = parser.parse_args(argv)
>        parser.verify_usage(parsed_args)
>        return parsed_args

Feel free to push future work-in-progress to MozReview as if it was a real, final patch -- it's easier for us to discuss code in that tool and you can always make changes so pushing a draft is fine.

I think you want to pull build_kwargs_with_parser_using out of the test function like this:

@pytest.fixture
def build_kwargs_with_parser_using(...):
    // call MarionetteArguments etc and return result

Then use it in the test function by listing it in the parameters:

def test_my_thing(build_kwargs_with_parser_using, other_fixture_if_you_need, ...):
    result = build_kwargs_with_parser_using(['marionette', '--addon', 'something'])
    // check that result contains {'addon': 'something'}


As an extra tip, it might help to gain some familiarity with the thing you're testing. (a) Try playing around with MarionetteArguments in the python interpreter (./mach python) to see how it behaves. (b) Step through the ./mach marionette-test command with a debugger (e.g. ./mach --debug marionette-test --repeat --gecko-log=somefile) to see how the args are constructed.

Comment 18

[Leni Kadali [:leni1] [GMT+3]]

Hello Maja, thanks for responding. 
> I think you want to pull build_kwargs_with_parser_using out of the test
> function like this:
> 
> @pytest.fixture
> def build_kwargs_with_parser_using(...):
>     // call MarionetteArguments etc and return result
> 
> Then use it in the test function by listing it in the parameters:
> 
> def test_my_thing(build_kwargs_with_parser_using, other_fixture_if_you_need,
> ...):
>     result = build_kwargs_with_parser_using(['marionette', '--addon',
> 'something'])
>     // check that result contains {'addon': 'something'}
How exactly do I check that a list has a dictionary? I imagine that the build_kwargs_with_parser_using function is supposed to take care of that or there's some code I'm missing? 
Looking through test_marionette_runner.py, the options I could see were: 
* Using `keys`
* Alternatively using an if statement: `if result[0:1] == {'addon': 'something'}`
* The approach in test_build_kwargs_basic_args(build_kwargs_using) also looked interesting.
* Most other test_build_kwargs* functions seem to explicitly define the dictionary to be used. 
https://dxr.mozilla.org/mozilla-central/rev/6dccae211ae5fec6a1c1244b878ce0b93860154f/testing/marionette/harness/marionette_harness/tests/harness_unit/test_marionette_runner.py#137-150
 
> As an extra tip, it might help to gain some familiarity with the thing
> you're testing. (a) Try playing around with MarionetteArguments in the
> python interpreter (./mach python) to see how it behaves. (b) Step through
> the ./mach marionette-test command with a debugger (e.g. ./mach --debug
> marionette-test --repeat --gecko-log=somefile) to see how the args are
> constructed.

How do I go about trying out MarionetteArguments in the python interpreter? 
How do I see how the arguments are constructed? Do I run the test file I'm working on?

Comment 19

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

(In reply to Leni Kadali from comment #18)
> Hello Maja, thanks for responding. 
> How exactly do I check that a list has a dictionary? I imagine that the
> build_kwargs_with_parser_using function is supposed to take care of that or
> there's some code I'm missing? 

build_kwargs_with_parser returns the result of parse_args, which can be converted into
a dict.
 
> How do I go about trying out MarionetteArguments in the python interpreter?
> How do I see how the arguments are constructed? 

Try this:

./mach python
from marionette_harness.runtests import MarionetteArguments
parser = MarionetteArguments()
args = parser.parse_args(["marionette", "--repeat", "2"])

You can turn args into a dict with vars(args)

Comment 20

[Leni Kadali [:leni1] [GMT+3]]

Attachment: [marionette harness tests] Test that BaseMarionetteArguments constructs arguments as expected

For the if statement below:

if result == {'addon': 'something'}:
        pass

I thought that result being equal to {'addon': 'something'} is
supposed to result in something more meaningful so I've used `pass` as a placeholder for that code.
I considered just returning the result but I wasn't sure about that.
Changed how I was importing MarionetteArguments following the guidelines here:
http://ateam-bootcamp.readthedocs.io/en/latest/reference/python-style.html

Comment 21

[Leni Kadali [:leni1] [GMT+3]]

Comment on attachment 8882180 [details] [diff] [review]
[marionette harness tests] Test that BaseMarionetteArguments constructs arguments as expected

Seems like I decided against using `pass` in the if statement. I opted to use `print result` instead.

Comment 22

[Leni Kadali [:leni1] [GMT+3]]

Also, may I be assigned to the bug please? :)

Comment 23

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

Comment on attachment 8882180 [details] [diff] [review]
[marionette harness tests] Test that BaseMarionetteArguments constructs arguments as expected

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

This is better. I've made suggestions for next steps.

Please change the commit message to "Bug 1315704 - [marionette harness tests] Test that BaseMarionetteArguments constructs arguments as expected; r?maja_zf,whimboo"

Once you incorporate the other changes I've suggested, the next step is to generalize the test to all the optional arguments listed in here: http://searchfox.org/mozilla-central/source/testing/marionette/harness/marionette_harness/runner/base.py#288-300 for now. 

I'll let you think about how to do this, but you could potentially use @pytest.mark.parametrize with a list of args.

I might be away the next time you submit a patch or submit a review request, so when you set a need-info or r?, please include my teammate whimboo -- he'll be able to continue the mentoring while I'm away.

::: testing/marionette/harness/marionette_harness/tests/harness_unit/test_marionette_runner.py
@@ +6,5 @@
>  import pytest
>  
>  from mock import Mock, patch, mock_open, sentinel, DEFAULT
>  
> +from marionette_harness.runtests import MarionetteArguments, MarionetteTestRunner

Could you move all your changes to test_marionette_arguments.py instead? In the end, think your test will look more like test_parse_arg_socket_timeout.

@@ +47,5 @@
>          return built_kwargs
>      return kwarg_builder
>  
> [e-mail]
> +def build_kwargs_with_parser_using(parsed_args):

Now that I look at this again, I don't think you really need a separate fixture after all. :) Sorry for any confusion this causes. 

Instead, you can just call MarionetteArguments() etc directly in the test function below. (i.e. move the body of this fixture function into your test function.

@@ +54,5 @@
> +    parser.verify_usage(parsed_args)
> +    return vars(parsed_args)
> +
> +# Then use it in the test function by listing it in the parameters:
> +def test_build_kwargs_with_parser_using(build_kwargs_with_parser_using):

Let's call this function `test_parsing_optional_arguments`

@@ +57,5 @@
> +# Then use it in the test function by listing it in the parameters:
> +def test_build_kwargs_with_parser_using(build_kwargs_with_parser_using):
> +    result = build_kwargs_with_parser_using(['marionette', '--addon', 'something'])
> +    # check that result contains {'addon': 'something'}
> +    if result == {'addon': 'something'}:

Instead of printing the result use an `assert` statement so that the test passes or fails.

e.g. assert parsed_args.get('addon') == 'something'

Comment 24

[Leni Kadali [:leni1] [GMT+3]]

(In reply to Maja Frydrychowicz (:maja_zf) (away July 1-18)) from comment #23)
> Once you incorporate the other changes I've suggested, the next step is to
> generalize the test to all the optional arguments listed in here:
> http://searchfox.org/mozilla-central/source/testing/marionette/harness/
> marionette_harness/runner/base.py#288-300 for now. 
> 
> I'll let you think about how to do this, but you could potentially use
> @pytest.mark.parametrize with a list of args.

Hello Henrik. Have been doing some reading as regards how to test all of the arguments. 
I mostly read this: https://docs.python.org/2.7/tutorial/controlflow.html#more-on-defining-functions
I thought that maybe the Arbitrary Argument Lists section may be relevant, although the fact that 
tuples are immutable does make it to me rather unwieldy. 

If using @pytest.mark.parametrize is the most straightforward way (in terms of longevity of the 
solution), then I'll be happy to use that.

Comment 25

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

Sorry for the delay in my response for this issue. But I was only partly available last week and had to catch up with a couple of other needinfo/reviews too.

I haven't had the time to go through the whole conversation you had with Maja regarding your patch, but seeing the referenced content in comment 24 I would agree that using @pytest.mark.parametrize would be a good way to go forward. It would help you to minimize the amount of code you would have to write/duplicate. Maybe get a start on it and let me know early via a work in progress patch how it works and looks like. So I can give an early feedback.

Comment 26

[Leni Kadali [:leni1] [GMT+3]]

Attachment: Bug1315704.patch

Here's my attempt to generalize the test to all the optional arguments listed in [1] using @pytest.mark.parametrize with a list of args.
Moved all my changes to test_marionette_arguments.py.
I tried to make the test look more like test_parse_arg_socket_timeout, but I wasn't sure what the equivalent of the argument `socket_timeout` would be for our test. I have used `self.add_argument`as the placeholder for the timebeing. 
I apologize in advance for how messy the arguments may appear;
I couldn't think of a way to generalize everything except for putting them in tuples for the timebeing. Advice here would be appreciated. :-)

[1]http://searchfox.org/mozilla-central/source/testing/marionette/harness/marionette_harness/runner/base.py#288-300

Comment 27

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

Comment on attachment 8889056 [details] [diff] [review]
Bug1315704.patch

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

Thanks, Leni. I've added some suggestions (and reference) for how to use @pytest.mark.parametrize

Incorporate that please and we'll take it from there. 


You can run your tests locally with `./mach python-test testing/marionette/harness/marionette_harness/tests/harness_unit/test_marionette_arguments.py`

::: testing/marionette/harness/marionette_harness/tests/harness_unit/test_marionette_arguments.py
@@ +37,5 @@
> +('--preferences', action='append', dest='prefs_files',
> +help="read preferences from a JSON or INI file. For INI, use "
> +"'file.ini:section' to specify a particular section."),
> +('--addon', action='append', dest='addons', help="addon to install; repeat for multiple addons.")])
> +def test_parsing_optional_arguments():

Take at look at this example of using `parametrize`: 

```
@pytest.mark.parametrize("x,y", [(1,2), (3,4)])
def test_foo(x, y):
    #do something with x and y
```

There first time the test is called, x is 1 and y is 2. The second time the test is called, x is 3 and y is 4. Notice that x and y are also listed as params to the test_foo function. Details here: https://docs.pytest.org/en/latest/parametrize.html 


So you want to do something similar, but your parameters are the *name* of an argument, like "pref" and an example value like "foo".

@@ +39,5 @@
> +"'file.ini:section' to specify a particular section."),
> +('--addon', action='append', dest='addons', help="addon to install; repeat for multiple addons.")])
> +def test_parsing_optional_arguments():
> +    parser = MarionetteArguments()
> +    parsed_args = parser.parse_args(['marionette', '--addon', 'something'])

Then here you want to use your parameters instead of hard-coding "--addon" and "something". Let's say you named your parameters x and y as in the previous example, then you want to use them to build something like `['marionette', '--' + x, y]`]

Please use more descriptive identifiers than x and y, though. :)

Comment 28

[Leni Kadali [:leni1] [GMT+3]]

Attachment: [marionette harness tests] Test that BaseMarionetteArguments constructs arguments as expected

Here's my attempt to generalize the test to all the optional arguments listed in [1] using @pytest.mark.parametrize with a list of args.
Moved all my changes to test_marionette_arguments.py.
I tried to make the test look more like test_parse_arg_socket_timeout.
I've used the names `arg_name,arg_action,arg_destination,arg_help` for the corresponding options and what they (to me) seem to do.

[1] http://searchfox.org/mozilla-central/source/testing/marionette/harness/marionette_harness/runner/base.py#288-300

Output of `./mach python-test testing/marionette/harness/marionette_harness/tests/harness_unit/test_marionette_arguments.py`
 0:37.95 /home/user/src/mozilla-central/testing/marionette/harness/marionette_harness/tests/harness_unit/test_marionette_arguments.py
 0:38.01   File "/home/user/src/mozilla-central/testing/marionette/harness/marionette_harness/tests/harness_unit/test_marionette_arguments.py", line 35
 0:38.01     @pytest.mark.parametrize("arg_name,arg_action,arg_destination,arg_help", [('--pref', action='append', dest='prefs_args',
 0:38.01                                                                                                ^
 0:38.01 SyntaxError: invalid syntax
 0:38.03 TEST-UNEXPECTED-FAIL | No test output (missing mozunit.main() call?): /home/user/src/mozilla-central/testing/marionette/harness/marionette_harness/tests/harness_unit/test_marionette_arguments.py
 0:38.03 Setting retcode to 1 from /home/user/src/mozilla-central/testing/marionette/harness/marionette_harness/tests/harness_unit/test_marionette_arguments.py
 0:38.04 Return code from mach python-test: 1

As a result of this, I removed the assignment operator from the arguments.

After which I got this error:

 0:05.68 TEST-START | test_marionette_arguments.py::test_parsing_optional_arguments[--pref-append-prefs_args-A preference to set. Must be a key-value pair separated by a ':'.]
 0:05.90 TEST-UNEXPECTED-FAIL | test_marionette_arguments.py::test_parsing_optional_arguments[--pref-append-prefs_args-A preference to set. Must be a key-value pair separated by a ':'.] | SystemExit: 2 (line 2362)
 0:05.90 arg_name = '--pref', arg_action = 'append', arg_destination = 'prefs_args'
 0:05.90 arg_help = "A preference to set. Must be a key-value pair separated by a ':'."
 0:05.90
 0:05.90     @pytest.mark.parametrize("arg_name, arg_action, arg_destination, arg_help", [('--pref', 'append', 'prefs_args',
 0:05.90     "A preference to set. Must be a key-value pair separated by a ':'."),
 0:05.90     ('--preferences', 'append', 'prefs_files',
 0:05.90     "read preferences from a JSON or INI file. For INI, use "
 0:05.90     "'file.ini:section' to specify a particular section."),
 0:05.90     ('--addon', 'append', 'addons', "addon to install; repeat for multiple addons.")])
 0:05.90     def test_parsing_optional_arguments(arg_name, arg_action, arg_destination, arg_help):
 0:05.90         parser = MarionetteArguments()
 0:05.90 >       parsed_args = parser.parse_args(['marionette', arg_name, arg_action, arg_destination, arg_help])
 0:05.90
 0:05.90 testing/marionette/harness/marionette_harness/tests/harness_unit/test_marionette_arguments.py:43:
 0:05.90 _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
 0:05.90 /usr/lib/python2.7/argparse.py:1704: in parse_args
 0:05.90     self.error(msg % ' '.join(argv))
 0:05.90 /usr/lib/python2.7/argparse.py:2374: in error
 0:05.90     self.exit(2, _('%s: error: %s\n') % (self.prog, message))
 0:05.91 _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
 0:05.91
 0:05.91 self = MarionetteArguments(prog='test_marionette_arguments.py', usage=None, descripti...lass=<class 'argparse.HelpFormatter'>, conflict_handler='error', add_help=True)
 0:05.91 status = 2
 0:05.91 message = "test_marionette_arguments.py: error: unrecognized arguments: prefs_args A preference to set. Must be a key-value pair separated by a ':'.\n"
 0:05.91
 0:05.91     def exit(self, status=0, message=None):
 0:05.91         if message:
 0:05.91             self._print_message(message, _sys.stderr)
 0:05.91 >       _sys.exit(status)
 0:05.91 E       SystemExit: 2
 0:05.91
 0:05.91 /usr/lib/python2.7/argparse.py:2362: SystemExit
 0:05.91 TEST-INFO took 223ms
 0:05.91 TEST-START | test_marionette_arguments.py::test_parsing_optional_arguments[--preferences-append-prefs_files-read preferences from a JSON or INI file. For INI, use 'file.ini:section' to specify a particular section.]
 0:06.08 TEST-UNEXPECTED-FAIL | test_marionette_arguments.py::test_parsing_optional_arguments[--preferences-append-prefs_files-read preferences from a JSON or INI file. For INI, use 'file.ini:section' to specify a particular section.] | SystemExit: 2 (line 2362)
 0:06.09 arg_name = '--preferences', arg_action = 'append'
 0:06.09 arg_destination = 'prefs_files'
 0:06.09 arg_help = "read preferences from a JSON or INI file. For INI, use 'file.ini:section' to specify a particular section."
 0:06.09
 0:06.09     @pytest.mark.parametrize("arg_name, arg_action, arg_destination, arg_help", [('--pref', 'append', 'prefs_args',
 0:06.09     "A preference to set. Must be a key-value pair separated by a ':'."),
 0:06.09     ('--preferences', 'append', 'prefs_files',
 0:06.09     "read preferences from a JSON or INI file. For INI, use "
 0:06.09     "'file.ini:section' to specify a particular section."),
 0:06.09     ('--addon', 'append', 'addons', "addon to install; repeat for multiple addons.")])
 0:06.09     def test_parsing_optional_arguments(arg_name, arg_action, arg_destination, arg_help):
 0:06.09         parser = MarionetteArguments()
 0:06.09 >       parsed_args = parser.parse_args(['marionette', arg_name, arg_action, arg_destination, arg_help])
 0:06.09
 0:06.09 testing/marionette/harness/marionette_harness/tests/harness_unit/test_marionette_arguments.py:43:
 0:06.09 _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
 0:06.09 /usr/lib/python2.7/argparse.py:1704: in parse_args
 0:06.09     self.error(msg % ' '.join(argv))
 0:06.09 /usr/lib/python2.7/argparse.py:2374: in error
 0:06.09     self.exit(2, _('%s: error: %s\n') % (self.prog, message))
 0:06.09 _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
 0:06.09
 0:06.09 self = MarionetteArguments(prog='test_marionette_arguments.py', usage=None, descripti...lass=<class 'argparse.HelpFormatter'>, conflict_handler='error', add_help=True)
 0:06.09 status = 2
 0:06.09 message = "test_marionette_arguments.py: error: unrecognized arguments: prefs_files read preferences from a JSON or INI file. For INI, use 'file.ini:section' to specify a particular section.\n"
 0:06.09
 0:06.09     def exit(self, status=0, message=None):
 0:06.09         if message:
 0:06.09             self._print_message(message, _sys.stderr)
 0:06.09 >       _sys.exit(status)
 0:06.09 E       SystemExit: 2
 0:06.09
 0:06.09 /usr/lib/python2.7/argparse.py:2362: SystemExit
 0:06.09 TEST-INFO took 180ms
 0:06.09 TEST-START | test_marionette_arguments.py::test_parsing_optional_arguments[--addon-append-addons-addon to install; repeat for multiple addons.]
 0:06.23 TEST-UNEXPECTED-FAIL | test_marionette_arguments.py::test_parsing_optional_arguments[--addon-append-addons-addon to install; repeat for multiple addons.] | SystemExit: 2 (line 2362)
 0:06.23 arg_name = '--addon', arg_action = 'append', arg_destination = 'addons'
 0:06.23 arg_help = 'addon to install; repeat for multiple addons.'
 0:06.23
 0:06.23     @pytest.mark.parametrize("arg_name, arg_action, arg_destination, arg_help", [('--pref', 'append', 'prefs_args',
 0:06.23     "A preference to set. Must be a key-value pair separated by a ':'."),
 0:06.23     ('--preferences', 'append', 'prefs_files',
 0:06.23     "read preferences from a JSON or INI file. For INI, use "
 0:06.23     "'file.ini:section' to specify a particular section."),
 0:06.23     ('--addon', 'append', 'addons', "addon to install; repeat for multiple addons.")])
 0:06.23     def test_parsing_optional_arguments(arg_name, arg_action, arg_destination, arg_help):
 0:06.23         parser = MarionetteArguments()
 0:06.23 >       parsed_args = parser.parse_args(['marionette', arg_name, arg_action, arg_destination, arg_help])
 0:06.23
 0:06.23 testing/marionette/harness/marionette_harness/tests/harness_unit/test_marionette_arguments.py:43:
 0:06.23 _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
 0:06.23 /usr/lib/python2.7/argparse.py:1704: in parse_args
 0:06.23     self.error(msg % ' '.join(argv))
 0:06.23 /usr/lib/python2.7/argparse.py:2374: in error
 0:06.23     self.exit(2, _('%s: error: %s\n') % (self.prog, message))
 0:06.23 _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
 0:06.23
 0:06.23 self = MarionetteArguments(prog='test_marionette_arguments.py', usage=None, descripti...lass=<class 'argparse.HelpFormatter'>, conflict_handler='error', add_help=True)
 0:06.23 status = 2
 0:06.24 message = 'test_marionette_arguments.py: error: unrecognized arguments: addons addon to install; repeat for multiple addons.\n'
 0:06.24
 0:06.24     def exit(self, status=0, message=None):
 0:06.24         if message:
 0:06.24             self._print_message(message, _sys.stderr)
 0:06.24 >       _sys.exit(status)
 0:06.24 E       SystemExit: 2
 0:06.24
 0:06.24 /usr/lib/python2.7/argparse.py:2362: SystemExit
 0:06.24 TEST-INFO took 144ms
 0:06.24 SUITE-END | took 0s
 0:06.36 Setting retcode to 1 from /home/user/src/mozilla-central/testing/marionette/harness/marionette_harness/tests/harness_unit/test_marionette_arguments.py
 0:06.36 Return code from mach python-test: 1

Comment 29

[Leni Kadali [:leni1] [GMT+3]]

Comment on attachment 8889868 [details] [diff] [review]
[marionette harness tests] Test that BaseMarionetteArguments constructs arguments as expected

I've used the names `arg_name,arg_action,arg_destination,arg_help` for the corresponding options and what they (to me) seem to do. 
What should be standing between `marionette` and the parameters here: `['marionette', '--' + x, y]`?
How do I set multiple instances/options for `--addon`, `--pref` and `--preferences`.

Comment 30

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

Comment on attachment 8889868 [details] [diff] [review]
[marionette harness tests] Test that BaseMarionetteArguments constructs arguments as expected

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

Okay, the way you're using parametrize is correct, but some of the parameters are not right. I have added comments about what we want the test to do to help you figure out the next step. 

Beyond these hints, I feel that you should be able to put together the rest on your own by reading about the argparse Python library and the BaseMarionetteArguments code. You've made good progress so the rest should be clearer for you now. Next time you make a review request, please submit code that doesn't have any syntax errors and runs successfully. Thanks.

::: testing/marionette/harness/marionette_harness/tests/harness_unit/test_marionette_arguments.py
@@ +31,5 @@
>      import sys
>      sys.exit(pytest.main(
>          ['--log-tbpl=-', __file__]))
> +
> [e-mail]("arg_name, arg_action, arg_destination, arg_help", [('--pref', 'append', 'prefs_args',

All we want to test is arg_name and arg_destination. Add to that a suitable arg_value. For example, "preferences", "prefs_files" and "somefile.ini". (The help is just displayed to the user, which we don't need to test, and the action tells you how the data is stored, which can help you decide what a reasonable arg_value might be.)

The point of the test is to check that, for example, when the arguments "--preferences somefile.ini" are passed to marionette, that gets parsed and stored as `{prefs_files: ["somefile.ini"]}` in parsed_args.

@@ +39,5 @@
> +"'file.ini:section' to specify a particular section."),
> +('--addon', 'append', 'addons', "addon to install; repeat for multiple addons.")])
> +def test_parsing_optional_arguments(arg_name, arg_action, arg_destination, arg_help):
> +    parser = MarionetteArguments()
> +    parsed_args = parser.parse_args(['marionette', arg_name, arg_action, arg_destination, arg_help])

So here you want to pass in whatever the user would actually type when interacting with the marionette command. e.g. 'marionette --preferences somefile.ini', which is broken down as ['marionette', '--' + arg_name, arg_value].

@@ +41,5 @@
> +def test_parsing_optional_arguments(arg_name, arg_action, arg_destination, arg_help):
> +    parser = MarionetteArguments()
> +    parsed_args = parser.parse_args(['marionette', arg_name, arg_action, arg_destination, arg_help])
> +    parser.verify_usage(parsed_args)
> +    if result == {'addon': 'something'}:

Please address my comments from previous reviews about these lines.

Comment 31

[Leni Kadali [:leni1] [GMT+3]]

(In reply to Maja Frydrychowicz (:maja_zf) from comment #30)
> :::
> testing/marionette/harness/marionette_harness/tests/harness_unit/
> test_marionette_arguments.py
> @@ +31,5 @@
> >      import sys
> >      sys.exit(pytest.main(
> >          ['--log-tbpl=-', __file__]))
> > +
> > [e-mail]("arg_name, arg_action, arg_destination, arg_help", [('--pref', 'append', 'prefs_args',
> 
> All we want to test is arg_name and arg_destination. Add to that a suitable
> arg_value. For example, "preferences", "prefs_files" and "somefile.ini".
> (The help is just displayed to the user, which we don't need to test, and
> the action tells you how the data is stored, which can help you decide what
> a reasonable arg_value might be.)
> 
> The point of the test is to check that, for example, when the arguments
> "--preferences somefile.ini" are passed to marionette, that gets parsed and
> stored as `{prefs_files: ["somefile.ini"]}` in parsed_args.
> 
> @@ +39,5 @@
> > +"'file.ini:section' to specify a particular section."),
> > +('--addon', 'append', 'addons', "addon to install; repeat for multiple addons.")])
> > +def test_parsing_optional_arguments(arg_name, arg_action, arg_destination, arg_help):
> > +    parser = MarionetteArguments()
> > +    parsed_args = parser.parse_args(['marionette', arg_name, arg_action, arg_destination, arg_help])
> 
> So here you want to pass in whatever the user would actually type when
> interacting with the marionette command. e.g. 'marionette --preferences
> somefile.ini', which is broken down as ['marionette', '--' + arg_name,
> arg_value].

As I had stated in IRC, it seems that taking the above approach means that the test fails silently. It will only provide verbose output once we make arg_destination to be evaluated in addition to arg_name and arg_value. My way of testing was to write a small test.py file in my mozilla-central directory and assigning default arguments to arg_name, arg_destination and arg_file (=arg_value). The code is as follows:

import pytest
from marionette_harness.runtests import MarionetteArguments
parser = MarionetteArguments()
arg_file = 'somefile.ini'
arg_name = 'pref'
arg_destination = 'prefs_args'
parsed_args = parser.parse_args(['marionette', '--' + arg_name, arg_destination, arg_file])
print parsed_args
result = vars(parsed_args)
print result

Now the frequent print statements are to make sure that the output I expect is what is coming out at each step. With the introduction of arg_destination, this test only manages to print `parsed_args`, after which it fails with the following error message when I run `./mach python test.py`:

user@localhost:~/src/mozilla-central$: ./mach python test.py 
usage: test.py [-h] [--binary BINARY] [--address ADDRESS] [--emulator]
               [--app APP] [--app-arg APP_ARGS] [--profile PROFILE]
               [--pref PREFS_ARGS] [--preferences PREFS_FILES]
               [--addon ADDONS] [--repeat REPEAT] [--run-until-failure]
               [--testvars TESTVARS] [--symbols-path SYMBOLS_PATH]
               [--startup-timeout STARTUP_TIMEOUT] [--shuffle]
               [--shuffle-seed SHUFFLE_SEED] [--total-chunks TOTAL_CHUNKS]
               [--this-chunk THIS_CHUNK] [--server-root SERVER_ROOT]
               [--gecko-log GECKO_LOG] [--logger-name LOGGER_NAME]
               [--jsdebugger] [--pydebugger PYDEBUGGER]
               [--socket-timeout SOCKET_TIMEOUT] [--disable-e10s] [--headless]
               [--tag TEST_TAGS] [--workspace WORKSPACE] [-v]
               [--emulator-binary EMULATOR_BIN] [--adb ADB_PATH] [--avd AVD]
               [--avd-home AVD_HOME] [--device DEVICE_SERIAL]
               [--package PACKAGE_NAME]
               [--browsermob-script BROWSERMOB_SCRIPT]
               [--browsermob-port BROWSERMOB_PORT]
               [tests [tests ...]]
test.py: error: unrecognized arguments: somefile.ini

When I remove arg_destination from the arguments above, `./mach python` runs successfully, giving me the following output:

Namespace(adb_path=None, addons=None, address=None, app=None, app_args=[], avd=None, avd_home=None, binary=None, browsermob_port=None, browsermob_script=None, device_serial=None, e10s=True, emulator=False, emulator_bin=None, gecko_log=None, headless=False, jsdebugger=False, logger_name='Marionette-based Tests', package_name=None, prefs_args=['somefile.ini'], prefs_files=None, profile=None, pydebugger=None, repeat=None, run_until_failure=False, server_root=None, shuffle=False, shuffle_seed=5684732316316819051, socket_timeout=360.0, startup_timeout=60, symbols_path=None, test_tags=None, tests=['marionette'], testvars=None, this_chunk=None, total_chunks=None, verbose=None, workspace=None)
{'startup_timeout': 60, 'browsermob_port': None, 'shuffle': False, 'verbose': None, 'package_name': None, 'socket_timeout': 360.0, 'app': None, 'symbols_path': None, 'app_args': [], 'emulator_bin': None, 'avd': None, 'prefs_files': None, 'jsdebugger': False, 'adb_path': None, 'binary': None, 'total_chunks': None, 'avd_home': None, 'gecko_log': None, 'pydebugger': None, 'e10s': True, 'profile': None, 'tests': ['marionette'], 'repeat': None, 'logger_name': 'Marionette-based Tests', 'address': None, 'run_until_failure': False, 'browsermob_script': None, 'this_chunk': None, 'test_tags': None, 'testvars': None, 'server_root': None, 'headless': False, 'prefs_args': ['somefile.ini'], 'emulator': False, 'workspace': None, 'addons': None, 'device_serial': None, 'shuffle_seed': 5684732316316819051}

Your thoughts on this would be appreciated.

Comment 32

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

When you use marionette at the command-line, you only use arg_name, and possibly arg_value. For example: `marionette --pref somefile.ini`. arg_destination is an internal concept not exposed to the command-line user; it represents the variable name at which the value "somefile.ini" will get stored. In other words, when `marionette --pref somefile.ini` is parsed, argparse stores a reference to the list ["somefile.ini"] in prefs_args, which is what you see in the Namespace object you pasted.

In your call to parse_args above, you are effectively passing in "marionette --pref prefs_args somefile.ini". This gets parsed as `prefs_arg=["prefs_args"]` in the Namespace, as I explained in the previous paragraph. Then "somefile.ini" is left over so the parser thinks that it's a whole other argument name, and it doesn't recognize it, which is why you see that error.

You should read about how arguments are defined here: https://docs.python.org/2.7/library/argparse.html#argparse.ArgumentParser.add_argument

In the test you want to write, arg_destination should be used later on when you check that the value actually got stored when instantiating BaseMarionetteTestRunner: that it defines self.app_args, self.addons, self.e10s, self.test_tags, as well as adb_path, emulator_bin, and package_name in self.extra_kwargs.

Comment 33

[Leni Kadali [:leni1] [GMT+3]]

Attachment: [marionette harness tests] Test that BaseMarionetteArguments constructs arguments as expected

Here's my attempt to generalize the test to all the optional arguments listed in [1] using @pytest.mark.parametrize with a list of args.
Moved all my changes to test_marionette_arguments.py.
I've used the names `arg_name` for the argument name passed to marionette and `arg_file` for the file option.
Running `./mach python-test testing/marionette/harness/marionette_harness/tests/harness_unit/test_marionette_arguments.py`
goes through successfully as well.

[1] http://searchfox.org/mozilla-central/source/testing/marionette/harness/marionette_harness/runner/base.py#288-300

Comment 34

[Leni Kadali [:leni1] [GMT+3]]

Comment on attachment 8896641 [details] [diff] [review]
[marionette harness tests] Test that BaseMarionetteArguments constructs arguments as expected

After feeling stumped, I decided to re-read the comments and the associated references. It seems I got a little lost around the removal of fixtures and the usage of parametrize for the arguments. Re-reading Comment 27 made me realize that I had misunderstood how parametrize is to be used. Also Maya's explanation when I showed my demo approach in the test.py file further clarified what I had been doing wrong. So thanks for your patience Maya. I appreciate it. This should be good to go, barring any linting/PEP8 issues.

Comment 35

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

Comment on attachment 8896641 [details] [diff] [review]
[marionette harness tests] Test that BaseMarionetteArguments constructs arguments as expected

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

In addition to the changes below, please also implement parameters for the following BaseMarionetteTestRunner properties: self.app_args, self.addons, self.e10s, self.test_tags, as well as adb_path, emulator_bin, and package_name in self.extra_kwargs.

::: testing/marionette/harness/marionette_harness/tests/harness_unit/test_marionette_arguments.py
@@ +30,5 @@
>      import sys
>      sys.exit(pytest.main(
>          ['--log-tbpl=-', __file__]))
> +
> [e-mail]("arg_name, arg_file", [('pref', 'prefs_args'), ('preferences', 'prefs_files'), ('addon', 'addons')])

Please change "arg_file" to "arg_value", since it should represent any possible value for an argument, not necessarily a file.

You also need a parameter like "arg_dest" to represent the destination.

For example, look at the definition of `pref` in base.py to see what we mean by arg_name and arg_dest:

```
        self.add_argument('--pref', <===== arg_name
                          action='append',
                          dest='prefs_args', <==== arg_dest
                          help="A preference to set. Must be a key-value pair separated by a ':'.")
```

As for arg_value, you can infer this from the other info in the definition. For example, we know that pref is used like `--pref a:b`, so in parametrize, you would have "arg_name,arg_value,arg_dest" with ["pref","a:b","prefs_args"].

@@ +36,5 @@
> +    parser = MarionetteArguments()
> +    parsed_args = parser.parse_args(['marionette', '--' + arg_name, arg_file])
> +    result = vars(parsed_args)
> +    
> +    if result == {arg_name: arg_file}:

This if-condition should be removed, because `result` will also contain entries for args with default values. We always want to run the `assert` in the line below, unconditionally.

In other words, right now this test passes only because it's never actually reaching the `assert` statement.

@@ +37,5 @@
> +    parsed_args = parser.parse_args(['marionette', '--' + arg_name, arg_file])
> +    result = vars(parsed_args)
> +    
> +    if result == {arg_name: arg_file}:
> +        assert parsed_args.get(arg_name) == arg_file

`parsed_args` should be replaced with `result` here. 

The assert is also incorrect: it should be checking arg_dest. The role of arg_dest is explained in Comment 32.

Comment 36

[Leni Kadali [:leni1] [GMT+3]]

(In reply to Maja Frydrychowicz (:maja_zf) from comment #35)
> Comment on attachment 8896641 [details] [diff] [review]
> [marionette harness tests] Test that BaseMarionetteArguments constructs
> arguments as expected
> 
> Review of attachment 8896641 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> In addition to the changes below, please also implement parameters for the
> following BaseMarionetteTestRunner properties: self.app_args, self.addons,
> self.e10s, self.test_tags, as well as adb_path, emulator_bin, and
> package_name in self.extra_kwargs.
So for this, I'm assuming that I should import BaseMarionetteTestRunner into testing/marionette/harness/marionette_harness/tests/harness_unit/test_marionette_arguments.py right?

> :::
> testing/marionette/harness/marionette_harness/tests/harness_unit/
> test_marionette_arguments.py
> @@ +30,5 @@
> >      import sys
> >      sys.exit(pytest.main(
> >          ['--log-tbpl=-', __file__]))
> > +
> > [e-mail]("arg_name, arg_file", [('pref', 'prefs_args'), ('preferences', 'prefs_files'), ('addon', 'addons')])
> 
> Please change "arg_file" to "arg_value", since it should represent any
> possible value for an argument, not necessarily a file.
> 
> You also need a parameter like "arg_dest" to represent the destination.
> As for arg_value, you can infer this from the other info in the definition.
> For example, we know that pref is used like `--pref a:b`, so in parametrize,
> you would have "arg_name,arg_value,arg_dest" with
> ["pref","a:b","prefs_args"].
For this, what would the generic argument be, because I can't see an equivalent for `a:b` in base.py? Or is it to be produced as part of the test's code?

> assert result.get(arg_name) == arg_dest
I'm assuming that we remain evaluating `arg_name` and `arg_dest`.

Comment 37

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

(In reply to Leni Kadali [:leni1] [GMT+3] from comment #36)
> (In reply to Maja Frydrychowicz (:maja_zf) from comment #35)
> > Comment on attachment 8896641 [details] [diff] [review]
> > [marionette harness tests] Test that BaseMarionetteArguments constructs
> > arguments as expected
> > 
> > Review of attachment 8896641 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > In addition to the changes below, please also implement parameters for the
> > following BaseMarionetteTestRunner properties: self.app_args, self.addons,
> > self.e10s, self.test_tags, as well as adb_path, emulator_bin, and
> > package_name in self.extra_kwargs.
> So for this, I'm assuming that I should import BaseMarionetteTestRunner into
> testing/marionette/harness/marionette_harness/tests/harness_unit/
> test_marionette_arguments.py right?

No, you don't need BaseMarionetteTestRunner in this test. However, the args
I listed that are used in BaseMarionetteTestRunner should be checked in your test;
they correspond to args created by BaseMarionetteArguments.
In other words, reading BaseMarionetteTestRunner gives you context for what we're
testing (the args I mentioned) and how they are used in the actual application.

> 
> > :::
> > testing/marionette/harness/marionette_harness/tests/harness_unit/
> > test_marionette_arguments.py
> > @@ +30,5 @@
> > >      import sys
> > >      sys.exit(pytest.main(
> > >          ['--log-tbpl=-', __file__]))
> > > +
> > > [e-mail]("arg_name, arg_file", [('pref', 'prefs_args'), ('preferences', 'prefs_files'), ('addon', 'addons')])
> > 
> > Please change "arg_file" to "arg_value", since it should represent any
> > possible value for an argument, not necessarily a file.
> > 
> > You also need a parameter like "arg_dest" to represent the destination.
> > As for arg_value, you can infer this from the other info in the definition.
> > For example, we know that pref is used like `--pref a:b`, so in parametrize,
> > you would have "arg_name,arg_value,arg_dest" with
> > ["pref","a:b","prefs_args"].
> For this, what would the generic argument be, because I can't see an
> equivalent for `a:b` in base.py? Or is it to be produced as part of the
> test's code?

It's normal not to see it in base.py because it's supplied by the user. The description for prefs_args says "A preference to set. Must be a key-value pair separated by a ':'.", so your tests code could use any value like "something:somethingelse" and it should work.

> 
> > assert result.get(arg_name) == arg_dest
> I'm assuming that we remain evaluating `arg_name` and `arg_dest`.

Yes.

Comment 38

[David Burns :automatedtester]

[mass update] Setting priority

Comment 39

[Leni Kadali [:leni1] [GMT+3]]

Attachment: [marionette harness tests] Test that BaseMarionetteArguments constructs arguments as expected

Removed if condition, now assert runs unconditionally.
Evaluating `assert result.get(arg_name) == arg_dest` always returns false.
Inspecting the execution indicates that the corresponding key:value pair in result is
`'prefs_args':['prefs_args']`, the values stored in `arg_name` and `arg_dest` are read as
`pref` and `prefs_args` respectively, meaning that `arg_name` is never equal to `arg_dest`.
Something to try would be changing the string assigned to arg_dest, so that it comes out as
`'prefs_args'` rather than just `prefs_args` and then changing the assert statement to
`assert result.get(arg_dest) == arg_dest` may work.

Added the parameters for the following BaseMarionetteTestRunner properties:
self.app_args, self.addons, self.e10s, self.test_tags.

Comment 40

[Leni Kadali [:leni1] [GMT+3]]

Comment on attachment 8911038 [details] [diff] [review]
[marionette harness tests] Test that BaseMarionetteArguments constructs arguments as expected

The parameters for the following BaseMarionetteTestRunner properties:
adb_path, emulator_bin, and package_name in self.extra_kwargs don't seem to be in self.add_arguments in base.py. How exactly am I supposed to go about including them?

Comment 41

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

Comment on attachment 8911038 [details] [diff] [review]
[marionette harness tests] Test that BaseMarionetteArguments constructs arguments as expected

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

::: testing/marionette/harness/marionette_harness/tests/harness_unit/test_marionette_arguments.py
@@ +30,5 @@
>      import sys
>      sys.exit(pytest.main(
>          ['--log-tbpl=-', __file__]))
>  
> [e-mail]("arg_name, arg_dest, arg_value",

You're missing values for arg_value.
Writing "arg_name, arg_dest, arg_value" means the each item in the list of parameters below needs to be a 3-tuple, but you only provide pairs.

So for example, you need to provide a fake value for preferences, like ('preferences', 'prefs_files', 'examplefilename.txt').

This will make your test simulate the situation of a user passing |--preferences examplefilename.txt| to the marionette harness.

@@ +40,2 @@
>      parser = MarionetteArguments()
> +    parsed_args = parser.parse_args(['marionette', '--' + arg_name, arg_dest])

You should be able to remove 'marionette' from the list here. That is, only list the argument strings.

@@ +42,2 @@
>      result = vars(parsed_args)
> +    assert result.get(arg_name) == [arg_dest]

This is not comparing the correct things, which is why it fails. The goal of the test is to check that result has a key called arg_dest and the value at that key is arg_value.

Comment 42

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

(In reply to Leni Kadali [:leni1] [GMT+3] from comment #40)
> Comment on attachment 8911038 [details] [diff] [review]
> [marionette harness tests] Test that BaseMarionetteArguments constructs
> arguments as expected
> 
> The parameters for the following BaseMarionetteTestRunner properties:
> adb_path, emulator_bin, and package_name in self.extra_kwargs don't seem to
> be in self.add_arguments in base.py. How exactly am I supposed to go about
> including them?

They are, but they're grouped together in a different class is which probably why you didn't find them. If you search base.py for "adb_path" for example, you'll find this: https://dxr.mozilla.org/mozilla-central/source/testing/marionette/harness/marionette_harness/runner/base.py#463 

They are added in a separate step, which you don't need to worry about, and they'll be recognized by the argument parser if you include them in your test.

Thanks for working on this. I think we're almost there. :)

Comment 43

[Leni Kadali [:leni1] [GMT+3]]

Attachment: [marionette harness tests] Test that BaseMarionetteArguments constructs arguments as expected

Added the parameters for the following BaseMarionetteTestRunner properties:
self.app_args, self.addons, self.e10s, self.test_tags.
Added parameters for adb_path, emulator_bin, and package_name in self.extra_kwargs
Added a fourth parameter to store the value changed by argparse named `expected_value`.
This is because the "action" for pref is "append" [1]
Changed the assert to `assert result.get(arg_dest) == expected_value` from `assert result.get(arg_name) == [arg_dest]`

disable-e10s (arg_name) is a boolean flag that stores false (arg_value) in e10s (arg_dest). So its parameters are set as False
"samplevalue" is used so as to not mislead a future reader into thinking that we actually expect files for all those options.

All tests pass when `./mach python-test testing/marionette/harness/marionette_harness/tests/harness_unit/test_marionette_arguments.py`
is run

[1]: http://searchfox.org/mozilla-central/source/testing/marionette/harness/marionette_harness/runner/base.py#285

Comment 44

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

Comment on attachment 8912409 [details] [diff] [review]
[marionette harness tests] Test that BaseMarionetteArguments constructs arguments as expected

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

Great, so you have some basic functionality working, and now we need to test the actual piece we're interested in: _build_kwargs.

Your test needs to check that when we create a MarionetteTestRunner and call its _build_kwargs method, the arg we parsed appears correctly in the result. So you need to use the mach_parsed_kwargs fixture, update it with the arg parsed by your test, instantiate MarionetteTestRunner, call _build_kwargs, check its return value:

```
result = vars(parsed_args)
mach_parsed_kwargs[arg_dest] = result[arg_dest]
runner = MarionetteTestRunner(**mach_parsed_kwargs)
built_kwargs = runner._build_kwargs()
assert built_kwargs[arg_dest] == expected_value
```

In parametrize, you only need the following: app-arg, app, symbols-path, gecko-log

There are many other examples of using mach_parsed_kwargs in test_marionette_runner.py

You can also make a second test that is very similar, but sets emulator to True: mach_parsed_kwargs["emulator"] = True first and checks the following in parametrize: adb, avd, avd-home, package

::: testing/marionette/harness/marionette_harness/tests/harness_unit/test_marionette_arguments.py
@@ +30,5 @@
>      import sys
>      sys.exit(pytest.main(
>          ['--log-tbpl=-', __file__]))
>  
> [e-mail]("arg_name, arg_dest, arg_value, expected_value",

Please move your test to be above |if __name__ == '__main__'|

Comment 45

[Leni Kadali [:leni1] [GMT+3]]

So first off, sorry for the lack of activity here
(In reply to Maja Frydrychowicz (:maja_zf) from comment #44)
> In parametrize, you only need the following: app-arg, app, symbols-path,
> gecko-log
I notice that these arguments are not like the previous ones we've been using
in parametrize. The ones I have require 4 values yet, with the exception of
app-arg, the ones we're going to use only seem to have an arg_name only. 
Should I modify parametrize accordingly?

Comment 46

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

Yes

Comment 47

[Leni Kadali [:leni1] [GMT+3]]

Attachment: [marionette harness tests] Test that BaseMarionetteArguments constructs arguments as expected

I'm not sure how to make sure `mach_parsed_kwargs` is successfully defined. Should I define the fixture afresh in
test_marionette_arguments.py? When I tried importing it from conftest.py, I got errors saying that it couldn't be
used to modify data.

0:04.38 TEST-UNEXPECTED-FAIL | test_marionette_arguments.py::test_parsing_optional_arguments[app-arg-app_args-samplevalue-expected_value0] | NameError: global
name 'mach_parsed_kwargs' is not defined (line 40)

Also I'm having a problem getting the modified test to work since only `app-args` has an `arg_dest` value. The rest don't.
My approach was to have a placeholder empty value, but that causes the test to fail. Since even in the dict, the corresponding
arguments have a value of `None`, is there a way to test that instead?
 0:04.38 TEST-UNEXPECTED-FAIL | test_marionette_arguments.py::test_parsing_optional_arguments[symbols-path--samplevalue-samplevalue] | AssertionError: assert
None == 'samplevalue'
 0:04.38  +  where None = <built-in method get of dict object at 0x7f29a641ec58>('')
 0:04.38  +    where <built-in method get of dict object at 0x7f29a641ec58> = {'adb_path': None, 'addons': None, 'address': None, 'app': None, ...}.get (line
39)
 0:04.38 arg_name = 'symbols-path', arg_dest = '', arg_value = 'samplevalue'
 0:04.38 expected_value = 'samplevalue'
 0:04.38
 0:04.38     @pytest.mark.parametrize("arg_name, arg_dest, arg_value, expected_value",
 0:04.38                              [('app-arg', 'app_args', 'samplevalue', ['samplevalue']),
 0:04.38                               ('symbols-path', '', 'samplevalue', 'samplevalue')])
 0:04.38                               #('gecko-log', '', 'samplevalue', 'samplevalue'),
 0:04.38                               #('app', '', 'samplevalue', 'samplevalue')
 0:04.38     def test_parsing_optional_arguments(arg_name, arg_dest, arg_value, expected_value):
 0:04.38         parser = MarionetteArguments()
 0:04.38         parsed_args = parser.parse_args(['--' + arg_name, arg_value])
 0:04.38         result = vars(parsed_args)
 0:04.38 >       assert result.get(arg_dest) == expected_value
 0:04.38 E       AssertionError: assert None == 'samplevalue'
 0:04.38 E        +  where None = <built-in method get of dict object at 0x7f29a641ec58>('')
 0:04.38 E        +    where <built-in method get of dict object at 0x7f29a641ec58> = {'adb_path': None, 'addons': None, 'address': None, 'app': None, ...}.get

I have commented out the lines for easier troubleshooting. Will restore them once the test works for the first two.

Comment 48

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

pytest fixtures, like mach_parsed_kwargs, are not like other functions -- they don't need to be imported. Instead, pytest takes care of "finding" them. All you need to do is list mach_parsed_kwargs with the parameters of your test function: 

> test_parsing_optional_arguments(mach_parsed_kwargs, arg_name, arg_dest, arg_value, expected_value)


For future reference, see: https://docs.pytest.org/en/latest/fixture.html


Regarding arg_dest, when there is no arg_dest listed, it has a default value that is the same as the arg name, but with an underscore. So arg_dest for "symbols-path" is "symbols_path". 


See: https://docs.python.org/3/library/argparse.html#dest

Comment 49

[Leni Kadali [:leni1] [GMT+3]]

Attachment: [marionette harness tests] Test that BaseMarionetteArguments constructs arguments as expected

Substituted the values of `symbols-path` with `symbols_path` in cases where the argument has no
value for `arg_dest` as per [1].
Added `mach_parsed_kwargs` to the arguments for `test_parsing_optional_arguments`
Added the new test for the emulator. Modified the 'samplevalue' argument for `adb`
The new test for the emulator isn't passing.

[1] https://docs.python.org/3/library/argparse.html#dest

Comment 50

[Leni Kadali [:leni1] [GMT+3]]

Comment on attachment 8936405 [details] [diff] [review]
[marionette harness tests] Test that BaseMarionetteArguments constructs arguments as expected

Wasn't sure about what exactly we're to be testing as regards the emulator test. According to the dict that appears in the error, `adb_path: None`, so I wasn't sure if we're testing for a null value or something else.

Comment 51

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

Your last patch only shows a partial diff of changes. Please post a patch that shows a full diff of adding all the new tests.

The reason the tests are failing is that you're not passing the argument you parse (e.g. gecko-log) to MarionetteTestRunner. You need to update mach_parsed_kwargs with the new argument: `mach_parsed_kwargs[arg_dest] = result[arg_dest]`

Comment 52

[Leni Kadali [:leni1] [GMT+3]]

Attachment: [marionette harness tests] Test that BaseMarionetteArguments constructs arguments as expected

Patch that shows a full diff of adding all the new tests.
Updated mach_parsed_kwargs with the new argument: `mach_parsed_kwargs[arg_dest] = result[arg_dest]`
Tests pass successfully.

Comment 53

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

Try push is green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=48356a7c12f6c5cb56637b33550af2369c7bd9dc

Comment 54

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

This looks good, Leni. Could you just re-submit your patch with a corrected commit message? As follows: Bug 1315704 - Test that BaseMarionetteArguments constructs arguments as expected; r?maja_zf

Comment 55

[Leni Kadali [:leni1] [GMT+3]]

Attachment: [marionette harness tests] Test that BaseMarionetteArguments constructs arguments as expected

Patch that shows a full diff of adding all the new tests.
Updated mach_parsed_kwargs with the new argument: `mach_parsed_kwargs[arg_dest] = result[arg_dest]`
Tests pass successfully.

Comment 56

[Leni Kadali [:leni1] [GMT+3]]

Comment on attachment 8936744 [details] [diff] [review]
[marionette harness tests] Test that BaseMarionetteArguments constructs arguments as expected

Modified the commit message accordingly.

Comment 57

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

Attachment: 1315704.patch

Fixed some flake8 formatting.

Comment 58

[Cosmin Sabou [:CosminS]]

Please ask someone else to review your patch before it could be landed. Thanks!

Comment 59

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

As indicated inside the patch file and this bug history, the author is Leni Kadali, and I am the reviewer. Since Leni is a relatively new contributor, I added checkin-needed on their behalf and re-attached their patch file for them to correct the format.

Comment 60

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a83942bf8ddd
Test that BaseMarionetteArguments constructs arguments as expected; r=maja_zf

Comment 61

[Raul Gurzau (:RaulG)]

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