Closed Bug 1147129 Opened 7 years ago Closed 7 years ago

Upgrade mochitest from optparse to argparse

Categories

(Testing :: Mochitest, defect)

x86_64
Linux
defect
Not set
normal

Tracking

(firefox40 fixed)

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: ahal, Assigned: ahal)

References

Details

Attachments

(1 file, 1 obsolete file)

We no longer have any python 2.6 dependencies, so let's upgrade to argparse. It's easy to do and will make it easier to do future work around option handling (namely using configman if we decide to do that).
Assignee: nobody → ahalberstadt
Status: NEW → ASSIGNED
Depends on: 1150175
Attached file MozReview Request: bz://1147129/ahal (obsolete) —
/r/6659 - Bug 1147129 - upgrade mochitest from optparse to argparse and move android cli to mochitest_options.py, r=chmanchester

Pull down this commit:

hg pull -r 74d61d9755f21dfb01f74accbeae6e6fba67f60e https://reviewboard-hg.mozilla.org/gecko/
Comment on attachment 8588595 [details]
MozReview Request: bz://1147129/ahal

/r/6659 - Bug 1147129 - upgrade mochitest from optparse to argparse and move android cli to mochitest_options.py, r=chmanchester

Pull down this commit:

hg pull -r 74d61d9755f21dfb01f74accbeae6e6fba67f60e https://reviewboard-hg.mozilla.org/gecko/
Attachment #8588595 - Flags: review?(cmanchester)
Comment on attachment 8588595 [details]
MozReview Request: bz://1147129/ahal

Here's a passing try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=47c127009e76

Geoff, this patch moves the android command line option handling over to mochitest_options.py alongside the desktop and b2g options, but otherwise shouldn't have any effect on how they get run. Is this ok with you? I'll likely be refactoring that file into a subdirectory + config files later on.
Attachment #8588595 - Flags: feedback?(gbrown)
Comment on attachment 8588595 [details]
MozReview Request: bz://1147129/ahal

I have no objection -- it looks good to me.
Attachment #8588595 - Flags: feedback?(gbrown) → feedback+
Attachment #8588595 - Flags: review?(cmanchester) → review+
Comment on attachment 8588595 [details]
MozReview Request: bz://1147129/ahal

https://reviewboard.mozilla.org/r/6657/#review5545

Ship It!

::: testing/mochitest/mochitest_options.py
(Diff revision 1)
> -        defaults["httpPort"] = DEFAULT_PORTS['http']
> -        defaults["sslPort"] = DEFAULT_PORTS['https']
>          defaults["logFile"] = "mochitest.log"
>          defaults["autorun"] = True
>          defaults["closeWhenDone"] = True
> -        defaults["testPath"] = ""

Any particular reason to move some of these defaults and not others? Can we just make them all inline or set them all here?

::: testing/mochitest/runtests.py
(Diff revision 1)
> -    options, args = parser.parse_args()
> +    options = parser.parse_args()

Nit: Here and elsewhere we could call the result of this method call "args"

::: testing/mochitest/mochitest_options.py
(Diff revision 1)
> +        options.app = __file__

Moving this changes the value which looks suspicious, but I guess it's just a throwaway value, right?
https://reviewboard.mozilla.org/r/6657/#review5551

> Any particular reason to move some of these defaults and not others? Can we just make them all inline or set them all here?

Ideally they'd all be inlined, but the ones left over are needed to override the default of an option defined by the super class.

> Moving this changes the value which looks suspicious, but I guess it's just a throwaway value, right?

Yeah, it's getting set back to the proper value at the bottom of that function. Good eye though, I didn't notice that.

> Nit: Here and elsewhere we could call the result of this method call "args"

I agree, but the problem is that that options dict then gets passed around literally everywhere in mochitest. So renaming it to args here either means we'd have to s/options/args everywhere else, or we'd be inconsistent with the rest of the harness. Personally I think leaving it as "options" for now is easiest.
https://hg.mozilla.org/mozilla-central/rev/0ab6e1161f94
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Attachment #8588595 - Attachment is obsolete: true
Attachment #8619866 - Flags: review+
You need to log in before you can comment on or make changes to this bug.