Closed Bug 1091280 Opened 10 years ago Closed 10 years ago

Move automationutils.addCommonOptions

Categories

(Testing :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla36

People

(Reporter: jgriffin, Assigned: nazpanj, Mentored)

References

Details

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

User Story

The goal is to remove this function [1] and this function [2].

To do this, you need to look at all of the call sites in [3] and add the options defined in [1] and [2] to the various parser objects used by the call sites. There are three call sites, mochitest, reftest and xpcshell. Each of these will need to be updated.

[1] http://dxr.mozilla.org/mozilla-central/source/build/automationutils.py#152
[2] http://dxr.mozilla.org/mozilla-central/source/build/automation.py.in#388
[3] http://dxr.mozilla.org/mozilla-central/search?q=addCommonOptions&case=true&redirect=true

Attachments

(1 file, 3 obsolete files)

automationutils:addCommonOptions is used in a few places, and we have a method of the same name in automation.py.in:

http://dxr.mozilla.org/mozilla-central/search?q=addCommonOptions&case=true&redirect=true

We want to move this out of automationutils (so we can kill it); in the absence of a central option management module, should we just move this into each of the respective harnesses?
+1 to moving them to each individual harness. I believe there are some cases where duplicating stuff is a good thing, and that command line options are one of those cases :).

This isn't quite as trivial as some of the other good first bugs, but I think it's pretty do-able and I'd be willing to mentor.
Mentor: ahalberstadt
User Story: (updated)
Whiteboard: [good first bug][lang=python]
Could someone please assign this bug to me?? Thanks!
Assignee: nobody → panjwaninazma
Attachment #8523946 - Flags: review?(dminor)
Comment on attachment 8523946 [details] [diff] [review]
moved options from AutomationUtils.addCommonOptions to runreftest.py, mochitest_options.py, runxpcshelltests.py

Thanks for the patch! Since ahal is mentoring, he is probably in a better position to review this than I am.
Attachment #8523946 - Flags: review?(dminor) → review?(ahalberstadt)
Attachment #8523946 - Attachment is obsolete: true
Attachment #8523946 - Flags: review?(ahalberstadt)
Attachment #8524020 - Flags: review?(ahalberstadt)
Re-submitted the patch for review. The previous patch did not remove the original function addCommonOptions from automationUtils.py and automation.py.in.
Comment on attachment 8524020 [details] [diff] [review]
moved options from AutomationUtils.addCommonOptions to runreftest.py, mochitest_options.py, runxpcshelltests.py

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

Excellent patch, this is almost perfect! Functionally this should be exactly what we need, but I just have a few style suggestions before submitting this patch to production. Please address the review comments, re-upload the patch and flag me for review again. Almost there!

::: layout/tools/reftest/runreftest.py
@@ +618,5 @@
>    def __init__(self):
>      OptionParser.__init__(self)
>      defaults = {}
> +    defaults['SYMBOLS_PATH'] = None
> +    

nit: whitespace

(nit means an unimportant style fix. We like to try to make sure that there is no trailing whitespace anywhere. If you look at this line in the "review" view you'll see what I mean).

@@ +626,5 @@
> +                    default = None,
> +                    help = "absolute path to directory containing XRE (probably xulrunner)")
> +    self.add_option("--symbols-path",
> +                    action = "store", type = "string", dest = "symbolsPath",
> +                    default = defaults['SYMBOLS_PATH'],

I know this is just copy pasted, but it would be slightly cleaner if you just put None here and removed the defaults['SYMBOLS_PATH'] above. Same for the other two harnesses.

::: testing/mochitest/mochitest_options.py
@@ +447,5 @@
>                  value["default"] = []
>              self.add_option(*option, **value)
> +        defaults={}
> +        defaults['SYMBOLS_PATH'] = None
> +        

nit: whitespace

@@ +467,5 @@
> +                           "the application on the command line")
> +        self.add_option("--debugger-interactive",
> +                    action = "store_true", dest = "debuggerInteractive",
> +                    help = "prevents the test harness from redirecting "
> +                        "stdout and stderr for interactive debuggers")

mochitest_options.py is set up a little bit differently from the other two. Notice how the options are defined like this:

 [["--appname"],
        { "action": "store",
          "type": "string",
          "dest": "app",
          "default": None,
          "help": "absolute path to application, overriding default",
  }],

Please also define the newly added options in a similar fashion. You can just add them at the end of the pre-existing list.

::: testing/xpcshell/runxpcshelltests.py
@@ +1424,5 @@
> +                        default = None,
> +                        help = "absolute path to directory containing XRE (probably xulrunner)")
> +        self.add_option("--symbols-path",
> +                        action = "store", type = "string", dest = "symbolsPath",
> +                        default = defaults['SYMBOLS_PATH'],

Same comment as before, just put None here directly.
Attachment #8524020 - Flags: review?(ahalberstadt) → feedback+
Thanks Andrew. Can I remove the lines listed below from runxpcshelltests.py and mochitest_options.py? I don't see it being used anywhere (one I make the changes above)

defaults={}
defaults['SYMBOLS_PATH'] = None
Yes, please do! :)
Attachment #8524020 - Attachment is obsolete: true
Attachment #8525004 - Flags: review?(ahalberstadt)
Comment on attachment 8525004 [details] [diff] [review]
moved options from AutomationUtils.addCommonOptions to runreftest.py, mochitest_options.py, runxpcshelltests.py and removed the addCommonOptions from automationUtils and automation.py.in

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

Thank you so much! I'll push this to our try server just to be safe. In the mean time can you upload one last patch and update the commite message to say "r=ahal" at the end of it? (For more info on commit message conventions, see https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#Commit_Message_Conventions )
Attachment #8525004 - Flags: review?(ahalberstadt) → review+
Attachment #8525004 - Attachment is obsolete: true
thanks for your help andrew:)  I have uploaded the final patch!
Comment on attachment 8525557 [details] [diff] [review]
moved options from AutomationUtils.addCommonOptions to runreftest.py, mochitest_options.py, runxpcshelltests.py and removed the addCommonOptions from automationUtils and automation.py.in,

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

Thanks, once we see that the try run doesn't break anything, we just have to add a checkin-needed flag and the sheriffs will land the patch for us.
Attachment #8525557 - Flags: review+
Status: NEW → ASSIGNED
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/68a47aac2628
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.