Closed Bug 1256996 Opened 4 years ago Closed 4 years ago

Firefox ui mach commands must call parse_args and verify_usage

Categories

(Testing :: Firefox UI Tests, defect)

defect
Not set

Tracking

(firefox48 fixed)

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: whimboo, Assigned: totallygloria, Mentored)

Details

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

Attachments

(1 file, 3 obsolete files)

Not sure why the debugger doesn't show up. We definitely push True into Marionette. I will check that tomorrow.
This is because of bug 1258505.

As a workaround, the mach command needs to call parse_args and verify_usage.
Summary: Firefox ui mach commands ignore --jsdebugger → Firefox ui mach commands must call parse_args and verify_usage
Mentor: mjzffr
Whiteboard: [lang=py][good-first-bug]
This looks like a great first bug, I'm still getting my system up and running, but can jump on it tomorrow.
Assignee: nobody → gloriaanholt
Attached patch mach_commands.py (obsolete) — Splinter Review
I've changed mach_commands.py to call parser.parse_args() and parser.verify_usage(). This patch passes all firefox-ui-functional tests for me except for one, but I can't find a connection back to these changes:

0:27.33 TEST_END: MainThread ERROR, expected PASS
Traceback (most recent call last):
  File "/home/driel/src/mozilla-central/testing/marionette/harness/marionette/marionette_test.py", line 351, in run
    testMethod()
  File "/home/driel/src/mozilla-central/testing/firefox-ui/tests/functional/locationbar/test_access_locationbar.py", line 48, in test_access_locationbar_history
    Wait(self.marionette).until(lambda _: self.autocomplete_results.is_open)
  File "/home/driel/src/mozilla-central/testing/marionette/client/marionette_driver/wait.py", line 143, in until
    cause=last_exc)
TimeoutException: TimeoutException: Timed out after 5.1 seconds


Submitting what I have so you can see if you get the same error.
It's not related to your changes. If the tests start running, that's good. You can also check a few of the command-line options like --e10s and --jsdebugger.

Regardless, please go ahead and push your patch up for review, as described in the "New Contributors" wiki page you've been following.

Side-note: when your bug comment is a question for someone, it's a good idea to check the "Need for information from __" box at the bottom of the bug form.
(In reply to Gloria Guy  (:totallygloria) from comment #4)
> locationbar/test_access_locationbar.py", line 48, in
> test_access_locationbar_history
>     Wait(self.marionette).until(lambda _: self.autocomplete_results.is_open)
>   File
[..]
> TimeoutException: TimeoutException: Timed out after 5.1 seconds

This is a problem when you switch away from Firefox while the tests are running. So leave alone the keyboard and mouse and the failure will no longer happen.
Attached file id_rsa.pub (obsolete) —
I have read, and agree to abide by, the Commit Access Requirements.
Flags: needinfo?(mjzffr)
Thanks for the submission. :)
A few things to fix. We need you to tie in your additions (processing args) with what the rest of this method does -- e.g some of your additions aren't being used; things need to be reordered. Here's the overall idea:
(1) make an args thing by calling parse_args (done!)
(2) add extras to kwargs (done, but too late in the method)
(3) put all the kwargs stuff into args (done -- in the wrong place)
(4) verify usage (done!)
(5) pass args to cli()

Also, important, please change your commit message to be the following format:

> Bug 123 - short description (like bug summary); r?reviewer
>
> More details on next lines if necessary.

So for example: "Bug 1256996 - Firefox ui mach commands must call parse_args and verify_usage; r?maja_zf"

You can modify your commit and commit message using |hg histedit|.
Flags: needinfo?(mjzffr)
Could you make sure that the leading indents throught the file are made of exactly 4 spaces per indentation level and that there is no extra trailing whitespace? Mozreview shows where there is unusual whitespace throughout the file.

Change the call to parse_args to parse_args(tests=tests)

Another example of indentation to fix: exactly 4 spaces should precede test_types here, no other characters.

The assignments to kwargs should all happen before `for k,v in kwargs.iteritems()` loop.

Move this bit to before the call to `parse_args`, but don't assign it to `kwargs['tests']`, just `tests` instead.
>kwargs['tests'] = [os.path.join(fxui_dir, 'tests', test)
>                   for test in test_types[testtype]['default_tests']]
Comment on attachment 8734564 [details]
MozReview Request: Bug 1256996 - Firefox ui mach commands must call parse_args and verify_usage, with args as dict; r=maja_zf

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42365/diff/1-2/
Attachment #8734564 - Attachment description: MozReview Request: Bug 1256996 - Updated the testing/firefox-ui/mach_commands.py to call parser.parse_args() and parser.verify_usage() so that arguments are fully processed before being registers with BaseMarionetteArguments. Passes local test in firefox → MozReview Request: Bug 1256996 - Firefox ui mach commands must call parse_args and verify_usage; r?maja_zf
Attachment #8734564 - Flags: review?(mjzffr)
Comment on attachment 8734564 [details]
MozReview Request: Bug 1256996 - Firefox ui mach commands must call parse_args and verify_usage, with args as dict; r=maja_zf

https://reviewboard.mozilla.org/r/42365/#review39791

::: testing/firefox-ui/mach_commands.py:32
(Diff revisions 1 - 2)
> +<<<<<<< local
> +    import argparse
> +=======

Areas delimited by `<<<<<<<` and `======` should not be part of your patch. They indicate a merge conflict that needs to be resolved. 

Please go in the files and manually edit them until they look right, then commit again and push to review.
Attachment #8734564 - Flags: review?(mjzffr)
Comment on attachment 8734564 [details]
MozReview Request: Bug 1256996 - Firefox ui mach commands must call parse_args and verify_usage, with args as dict; r=maja_zf

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42365/diff/2-3/
Attachment #8734564 - Attachment description: MozReview Request: Bug 1256996 - Firefox ui mach commands must call parse_args and verify_usage; r?maja_zf → MozReview Request: Bug 1256996 - Firefox ui mach commands must call parse_args and verify_usage, with args as dict; r=maja_zf
Attachment #8734564 - Flags: review?(mjzffr)
Comment on attachment 8734564 [details]
MozReview Request: Bug 1256996 - Firefox ui mach commands must call parse_args and verify_usage, with args as dict; r=maja_zf

This patch passes mach tests for firefox-ui-functional without flags, and with --e10s and --jsdebugger.

My firefox-ui-update tests are giving new unclear errors, so I'm not sure if will work on testing. Hopefully the merge errors are cleared up - it looks correct in the diff.

Thanks!
Flags: needinfo?(mjzffr)
Comment on attachment 8734564 [details]
MozReview Request: Bug 1256996 - Firefox ui mach commands must call parse_args and verify_usage, with args as dict; r=maja_zf

https://reviewboard.mozilla.org/r/42365/#review40155

Looks good, just a couple more small things to improve.

::: testing/firefox-ui/mach_commands.py:35
(Diff revision 3)
> -    import argparse
> -
>      from mozlog.structured import commandline
>      import firefox_ui_harness
>  
> +    parser = setup_argument_parser_functional()

Set the parser based on `testtype` here. (update versus functional)

::: testing/firefox-ui/mach_commands.py:62
(Diff revision 3)
>          kwargs['server_root'] = os.path.join(fxui_dir, 'resources')
>  
>      # If no tests have been selected, set default ones
> -    if not kwargs.get('tests'):
> -        kwargs['tests'] = [os.path.join(fxui_dir, 'tests', test)
> +    if kwargs.get('tests'):
> +        tests = kwargs.get('tests')
> +    elif not kwargs.get('tests'):

This can just be 'else'
Attachment #8734564 - Flags: review?(mjzffr)
Flags: needinfo?(mjzffr)
https://reviewboard.mozilla.org/r/42365/#review40155

> Set the parser based on `testtype` here. (update versus functional)

This issue is still not resolved. What I mean in my original comment is that if the testtype is functional, set up the functional parser; otherwise if the testtype is update, use the update parser.
Comment on attachment 8737018 [details]
MozReview Request: Bug 1256996 - Firefox ui mach commands must call parse_args and verify_usage, with args as dict; r=maja_zf

https://reviewboard.mozilla.org/r/43651/#review40375

I clarified the remaining issue to resolve. Also, note that you have two separate commits up for review now for a single task, but these should be combined to just one: please use |hg histedit| to "roll" any extra commits into your first one before pushing up for review. Thanks!
Comment on attachment 8734564 [details]
MozReview Request: Bug 1256996 - Firefox ui mach commands must call parse_args and verify_usage, with args as dict; r=maja_zf

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42365/diff/3-4/
Attachment #8734564 - Flags: review?(mjzffr)
Attachment #8737018 - Attachment is obsolete: true
The parser should now match the test type -- I just missed what you were saying the first time, but looking at it now it makes sense.

I get a failure on one firefox-ui-update test which I think is unrelated to this issues (comes from an x86_64 linux bug, I believe). If I missed something and it's this patch, I'm happy to dig around more.

Thanks again for all the guidance!
Comment on attachment 8734564 [details]
MozReview Request: Bug 1256996 - Firefox ui mach commands must call parse_args and verify_usage, with args as dict; r=maja_zf

https://reviewboard.mozilla.org/r/42365/#review40567

Yay! Looks good! Thanks :)
Attachment #8734564 - Flags: review?(mjzffr) → review+
https://hg.mozilla.org/mozilla-central/rev/8e7f918c59ea
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.