Closed
Bug 1256996
Opened 8 years ago
Closed 8 years ago
Firefox ui mach commands must call parse_args and verify_usage
Categories
(Testing :: Firefox UI Tests, defect)
Testing
Firefox UI Tests
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
More context for new contributors, we need to do something like this (https://dxr.mozilla.org/mozilla-central/rev/f14898695ee0dd14615914f3e1401f17df57fdd7/dom/media/test/external/mach_commands.py#42-47) in testing/firefox-ui/mach_commands.py
Mentor: mjzffr
Whiteboard: [lang=py][good-first-bug]
Assignee | ||
Comment 3•8 years ago
|
||
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
Assignee | ||
Comment 4•8 years ago
|
||
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.
Reporter | ||
Comment 6•8 years ago
|
||
(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.
Assignee | ||
Comment 7•8 years ago
|
||
I have read, and agree to abide by, the Commit Access Requirements.
Assignee | ||
Comment 8•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/42365/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/42365/
Attachment #8734444 -
Attachment is obsolete: true
Attachment #8734523 -
Attachment is obsolete: true
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']]
Assignee | ||
Comment 11•8 years ago
|
||
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)
Assignee | ||
Comment 13•8 years ago
|
||
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)
Assignee | ||
Comment 14•8 years ago
|
||
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)
Assignee | ||
Comment 16•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/43651/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/43651/
Attachment #8737018 -
Flags: review?(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.
Attachment #8737018 -
Flags: review?(mjzffr)
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!
Assignee | ||
Comment 19•8 years ago
|
||
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)
Assignee | ||
Updated•8 years ago
|
Attachment #8737018 -
Attachment is obsolete: true
Assignee | ||
Comment 20•8 years ago
|
||
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+
Comment 23•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/8e7f918c59ea
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in
before you can comment on or make changes to this bug.
Description
•