650880 - port desktop unit tests to mozharness

Status: RESOLVED FIXED

(Release Engineering :: Applications: MozharnessCore, defect, P4)

Description

[Aki Sasaki (not active)]

This should get complexity out of buildbotcustom and allow the a-team, developers, and releng to all use the same scripts.

Comment 2

[Aki Sasaki (not active)]

By "port" I mean wrap run_tests.py in mozharness, for now.

Comment 3

[Armen [:armenzg]]

For talos jobs see bug 650887.

Comment 4

[Armen [:armenzg]]

I have interested on helping with this.
I will wait to see some changes on bug 650887 to help me make this faster.
By then I should have time to make this a P2.

Comment 5

[Armen [:armenzg]]

I'm not going to be able to get to this on this quarter.
I would like to be involved in the reviews if I don't get back fast enough to this and someone else grabs it.

Comment 6

[Armen [:armenzg]]

I'll be working on this.

Comment 7

[Armen [:armenzg]]

Not working on this actively. Jordan might be picking this up on April.

Comment 8

[(not currently active) Ted Mielczarek]

We're working on getting all our test harnesses to use mozbase via a virtualenv in the objdir (bug 661908). That's easy enough, but it complicates running them from the test package. Conveniently, mozharness has support for setting up a virtualenv, which we're already using for peptest:
http://hg.mozilla.org/build/mozharness/file/eed9746c412f/scripts/peptest.py

The peptest script is remarkably straightforward. We should replicate that for our other test harnesses, since that solves the virtualenv problem for packaged tests, and it's the right direction to move anyway.

(I was actually going to file a bug for this, but bugzilla showed me this bug existed.)

Comment 9

[Aki Sasaki (not active)]

Progress is here: https://github.com/lundjordan/mozharness/compare/master...linux64-unittesting

Comment 10

[Jordan Lund (:jlund)]

Attachment: patch for mozharness repo

here is my proposed patch for mozharness with desktop unittesting implementation of desktop unittesting

Comment 11

[Jordan Lund (:jlund)]

Attachment: patch for buildbot-configs repo

My proposed patch that adds config options for mozharness unittests.
The changes are from line 83 - 150 of the patch.

Comment 12

[Armen [:armenzg]]

Comment on attachment 628084 [details] [diff] [review]
patch for buildbot-configs repo

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

For the next patch. Would you mind separating the white space changes?
I will also be doing style comments. I bet we have a style guide somewhere but I will do it by heart.

Good job. This looks good. A patch addressing the style comments and the reboot_command should get you the r+.

::: mozilla-tests/config.py
@@ +1176,5 @@
> +# outputs with Mozharness inputs. The 'suite_name' and 'sub_categories' mimics
> +# what buildbot (mainly misc) considers as 'suite_name' and 'suite'
> +# respectively. 'suite_category' is used for an argument in mozharness's extra-args
> +mozharness_unittest_suites = [
> +

No new line please.

@@ +1177,5 @@
> +# what buildbot (mainly misc) considers as 'suite_name' and 'suite'
> +# respectively. 'suite_category' is used for an argument in mozharness's extra-args
> +mozharness_unittest_suites = [
> +
> +        {'suite_name' : 'mozharn_mochitests-1/5', 'suite_category' : 'mochitests', 'sub_categories' : ['chunk1']},

4 spaces rather than 8.

@@ +1182,5 @@
> +        {'suite_name' : 'mozharn_mochitests-2/5', 'suite_category' : 'mochitests', 'sub_categories' : ['chunk2']},
> +        {'suite_name' : 'mozharn_mochitests-3/5', 'suite_category' : 'mochitests', 'sub_categories' : ['chunk3']},
> +        {'suite_name' : 'mozharn_mochitests-4/5', 'suite_category' : 'mochitests', 'sub_categories' : ['chunk4']},
> +        {'suite_name' : 'mozharn_mochitests-5/5', 'suite_category' : 'mochitests', 'sub_categories' : ['chunk5']},
> +        {'suite_name' : 'mozharn_mochitests-other', 'suite_category' : 'mochitests', 'sub_categories' : [

I think you can add a "\" at the end of the line and move the "[" to the next line so the opening square bracket and the closing one are in the same line.

@@ +1183,5 @@
> +        {'suite_name' : 'mozharn_mochitests-3/5', 'suite_category' : 'mochitests', 'sub_categories' : ['chunk3']},
> +        {'suite_name' : 'mozharn_mochitests-4/5', 'suite_category' : 'mochitests', 'sub_categories' : ['chunk4']},
> +        {'suite_name' : 'mozharn_mochitests-5/5', 'suite_category' : 'mochitests', 'sub_categories' : ['chunk5']},
> +        {'suite_name' : 'mozharn_mochitests-other', 'suite_category' : 'mochitests', 'sub_categories' : [
> +                                'browser-chrome', 'chrome', 'a11y', 'plugins']},

Can you please remove the white spaces so it is aligned with the previous line + 4 white spaces?

@@ +1189,5 @@
> +        {'suite_name' : 'mozharn_jsreftest', 'suite_category' : 'reftests', 'sub_categories' : ['jsreftest']},
> +        {'suite_name' : 'mozharn_crashtest', 'suite_category' : 'reftests', 'sub_categories' : ['crashtest']},
> +        {'suite_name' : 'mozharn_xpcshell', 'suite_category' : 'xpcshell', 'sub_categories' : ['xpcshell']}
> +
> +        ]

No new line.
"]" starting from beginning of the line.

@@ +1191,5 @@
> +        {'suite_name' : 'mozharn_xpcshell', 'suite_category' : 'xpcshell', 'sub_categories' : ['xpcshell']}
> +
> +        ]
> +
> +for branch in ['mozilla-central']:

I like that you are adding this to be run side-by-side for "mozilla-central". We might choose another more quiet branch like "elm" or another so we don't cause too much overload. We will see later on.

@@ +1201,5 @@
> +            continue
> +        if pf.startswith("win"):
> +            hg_bin = 'c:\\mozilla-build\\hg\\hg'
> +            config_file = "test/win_unittest.py"
> +            reboot_command = ['c:/mozilla-build/python25/python', '-u']

I don't see the variable "reboot_command" used anywhere.

Did you want to modify PLATFORMS[pf]['mozharness_python']?

@@ +1214,5 @@
> +            for suite in mozharness_unittest_suites:
> +                extra_args = ["--cfg", config_file]
> +                for sub_category in suite['sub_categories']:
> +                    extra_args += ["--{cat}-suite".format(cat=suite['suite_category']),
> +                            sub_category]

I think this would look cleaner ["--%s-suite" % suite['suite_category'], sub_category] but I am fine if you use the format() function.

Instead of "cat" can you use "category"? The first thought was "cat" as the command or the pet.

@@ +1226,5 @@
> +                            '-f', '../reboot_count.txt',
> +                            '-n', '1', '-z'],
> +                        'hg_bin': hg_bin,
> +                })]
> +###################### END OF MOZHARNESS UNITTEST CONFIGS

I don't see any changes to misc.py.
Wouldn't all of these opt_unittests_suites be added to UnittestPackagedBuildFactory in misc.py#l438?
Or would it go to line 410 (elif mozharness:)

Oh nice! Aki landed some stuff there!

Comment 13

[Armen [:armenzg]]

Comment on attachment 628048 [details] [diff] [review]
patch for mozharness repo

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

What is the function of configs/test/desktop_unittest.py?

The configs/test files have few inconsistencies from one $platform_unittest.py file to another (e.g. "all_mochitest_suites" vs "all_mochi_suites".
Not sure what aki said but I would prefer of having common values be loaded from a common file (_common.py) like "global_*", "repos" and similar.

I will submit this comment and continue the review of desktop_unittest.py.

::: configs/test/desktop_unittest.py
@@ +13,5 @@
> +BINARY = "firefox-{version}.en-US.{OS_ARCH}.{OS_EXTENSION}"
> +TESTS = "firefox-{version}.en-US.{OS_ARCH}.tests.zip"
> +
> +config = {
> +

No empty lines please.

@@ +16,5 @@
> +config = {
> +
> +        # for develepors in future. Not implemented yet.
> +        "url_base" : FTP,
> +        "file_archives" : {"bin_archive" : BINARY, "tests_archive" :  TESTS},

I would call this "files", "binary_files" and "tests_file" but that it is only my opinion.
Just the word "archive" does not ring with me (I might have used it previously though)

@@ +25,5 @@
> +            "dest": "tools"
> +        }],
> +
> +        # I will put these in configs in the case that someone wants to try one
> +        # of there own dirs

Being explicit is good.

@@ +43,5 @@
> +            "symbols_path" : "--symbols-path={symbols_path}"
> +        },
> +
> +        #global mochitest options
> +        "global_mochi_options" : {

I would use "global_mochitest_options" to be more consistent.

@@ +58,5 @@
> +            'xpcshell_name' : 'firefox/{xpcshell_name}'
> +        },
> +
> +        #local mochi suites
> +        "all_mochi_suites" :

"all_mochitest_suites"

::: configs/test/linux_unittest.py
@@ +5,5 @@
> +#####
> +
> +config = {
> +
> +        ###### paths/urls can be implemented on command line as well

4 spaces of indentation please. No new line.

@@ +7,5 @@
> +config = {
> +
> +        ###### paths/urls can be implemented on command line as well
> +        "installer_url" : None, # eg: "http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/
> +                                                # mozilla-central-win32/1334941863/firefox-14.0a1.en-US.win32.zip"

The e.g. refers to a windows file instead of a linux tar ball being this the "linux_unittest.py" file.

@@ +20,5 @@
> +        #######
> +
> +
> +        "symbols_url" : None, # eg: "http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/
> +                                        # mozilla-central-win32/1334941863/firefox-14.0a1.en-US.crashreporter-symbols.zip"

What are all these "None" values?
The comments are kind of noisy and broken into multiple lines. Could you make them one line? and just add comments for the ones that can be more confusing?

Comment 14

[Jordan Lund (:jlund)]

Attachment: fixed style and unused reboot variable for buildbot-configs repo

Comment 15

[Jordan Lund (:jlund)]

(In reply to Armen Zambrano G. [:armenzg] - Release Engineer from comment #13)
> Comment on attachment 628048 [details] [diff] [review]
> patch for mozharness repo
> 
> Review of attachment 628048 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> What is the function of configs/test/desktop_unittest.py?

Arg, the inconsistances derive from this file not supposed to be in the patch. I missed it. Sorry about that. This is a legacy config file. Yesterday I showed you how I am using seperate config files only per platform and that old one was for an old style. There are comments however from your review for configs/desktop_unittests.py that will spill over into my other configs. So I will change all them to your suggestions.

Thanks

Comment 16

[Aki Sasaki (not active)]

Comment on attachment 628470 [details] [diff] [review]
fixed style and unused reboot variable for buildbot-configs repo

My main 2 concerns here, outside of Armen's nits, are:

1) branch selection (Armen already pointed this out).  This should be fine for testing, however.
2) wondering whether we have flexibility to have branch-specific configurations here.  But this is a good start.  When we end up replacing the unit test configs in UNITTEST_SUITES that may become a more pressing need; for now this should work.

Comment 17

[Aki Sasaki (not active)]

Comment on attachment 628048 [details] [diff] [review]
patch for mozharness repo

> diff --git a/configs/test/linux_unittest.py b/configs/test/linux_unittest.py

Hm, I've been using the configs/test/ directory for mozharness-testing config files rather than firefox-testing config files (e.g. test.illegal_suffix).

I'm open, but I think we should keep the two in separate locations.
Do you think I should move the mozharness-testing files elsewhere?  Or put these in a configs/unittest/ or something?

As an non-blocker enhancement, we might want user-friendly standalone configs for these at some point.

>+config = {
>+
>+        ###### paths/urls can be implemented on command line as well
>+        "installer_url" : None, # eg: "http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/

I tend to omit the lines that are None in production configs to keep them smaller and more readable.  The comments + empty config items are ok for now; they do belong in the standalone configs where users might hand-edit.

>+        #global unittest options
>+        "global_test_options" : {
>+            "app_name" : "--appname={binary_path}",
>+            "util_path" : "--utility-path=tests/bin",
>+            "extra_prof_path" : "--extra-profile-file=tests/bin/plugins",
>+            "symbols_path" : "--symbols-path={symbols_path}"
>+        },
>+
>+        #global mochitest options

I have the feeling we'll need to revisit this layout at some point.
For instance, when we add a new command line option on one branch but don't want it on the others.
However, I'm not sure how to guide you at this point; just noting.
I think we shouldn't change anything until we know what to change.

>+    # TODO find out which modules I do and dont need here
>+    virtualenv_modules = [

You probably need mozinstall and all of its dependencies.
That's actually in the test zip, so we're good.

I marked bug 701506 as a blocker, but it's probably not a hard blocker, as long as mozbase stays inside the test zip.

>+                    'pull-other-repos',

Haha.
Any reason you're naming it "pull-other-repos"?
Also, what do you need in build/tools?
I wasn't able to find a need for it.  If we need it, great; if we don't, let's take this out.  Lean&mean.

>+        if not self.check_if_valid_config():
>+            self.fatal("""Config options are not valid.
>+                    Please ensure that if the '--run-all-suites' flag was enabled
>+                    then do not specify to run only specific suites like '--mochitest-suite browser-chrome'""")

We could do this inside of self._pre_config_lock() if you want.

>+        self.glob_test_options = []
>+        self.glob_mochi_options = []

Glob can be shorthand for a wildcard or '*'.
https://en.wikipedia.org/wiki/Glob_%28programming%29
I appreciate the attempt to reduce the amount of typing, but let's say 'global' here :)

>+        self.ran_preflight_run_commands = False

Do you need this anymore?
Afaict, this is checked and set in preflight_run_tests(), which is called once.

>+        self.test_url = self.config.get('test_url')

c.get is equivalent to self.config.get with less typing; no preference here.

>+        self.installer_path = c.get('installer_path') or self.guess_installer_path()

I'd like to avoid this.
I haven't gotten to bug 753547 yet, but when I do, we'll specify installer_path's.

Here's the code I'll be leveraging:

http://hg.mozilla.org/build/mozharness/file/636e0f7b6ab6/mozharness/mozilla/testing/testbase.py#l143

Essentially, if we say we want the installer_path to be installer.exe, it'll download the installer to work_dir/installer.exe, even if it was firefox-15.0a1.en-US.win32.installer.exe.  That's good for knowing where the path to the installer is, without having to guess.

>+    def check_if_valid_config(self):
>+        suite_categories = ['mochitests', 'reftests', 'xpcshell']

These three suites are kind of hardcoded in this script, which might be ok.
This list might be nice to define as a global.

>+    def query_abs_dirs(self):
>+        if self.abs_dirs:
>+            return self.abs_dirs
>+        abs_dirs = super(DesktopUnittest, self).query_abs_dirs()
>+
>+        # TODO not make this so ugly

Wow.  Do you need all of these directories to be specified?

>+        c = self.config
>+        dirs = {}
>+        dirs['abs_app_install_dir'] = os.path.join(
>+            abs_dirs['abs_work_dir'], 'application')
>+
>+        dirs['abs_app_dir'] = os.path.join(
>+            dirs['abs_app_install_dir'], 'firefox')

I'd rather not hardcode firefox anywhere in here.
Can this go in the config files without making things too ugly?

>+    def guess_installer_path(self):

Yeah, still don't like this :)

>+    def query_glob_options(self, **kwargs):

I think I'll have to look deeper into how this runs before I have a solid opinion here, other than spelling out 'global'.

>+                    (1) specifing it in the config file under binary_path
>+                    (2) specifing it on command line with the '--binary-path' flag""")

sp: 'specifying' :)

>+    def copy_tree(self, src, dest, log_level='info', error_level='error'):

We should probably use INFO and ERROR here (import them from mozharness.base.log like this:
http://hg.mozilla.org/build/mozharness/file/636e0f7b6ab6/mozharness/base/script.py#l33

>+        try:
>+            files = os.listdir(src)
>+            files.sort()
>+            for f in files:
>+                abs_src_f = os.path.join(src, f)
>+                abs_dest_f = os.path.join(dest, f)
>+                self.copyfile(abs_src_f , abs_dest_f)

It looks like this isn't recursive?
If we can make this recursive, I think we can move this to OSMixin.


Overall, this looks good.  It feels a bit complex dealing with all the suites like this; I'm wondering if we'll need to refactor this later for simplicity, but I can't say for sure.  I'd like to see this running in staging!

Comment 18

[Aki Sasaki (not active)]

Comment on attachment 628470 [details] [diff] [review]
fixed style and unused reboot variable for buildbot-configs repo

One thing to note here:

In bug 758988, I added a script_maxtime setting for mozharness-based tests.
This is currently capped at 1200 seconds (20min) globally, but we can override it by setting a script_maxtime in the suites dict.

1200 is pretty strict; we can bump this to 3600 or 7200, but if we need to allow for longer-running unit tests, we'll need to do that per-suite.

Comment 19

[Armen [:armenzg]]

Comment on attachment 628048 [details] [diff] [review]
patch for mozharness repo

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

Overall this is great.

Giving feedback+ for now. We can have formal review when we see this running on staging.

::: scripts/desktop_unittest.py
@@ +31,5 @@
> +            {
> +                "action" : "append",
> +                "dest" : "specified_mochitest_suites",
> +                "type": "string",
> +                "help": """Specify which mochi suite to run.

"mochitest" for consistency.

@@ +85,5 @@
> +                "default": False,
> +                "help": """Does nothing ATM but I would like it to run a 'quick' set of test suites to
> +                see if there are any serious issues that can be noticed immediately.""",
> +            }
> +        ],

If it is not implemented we should take it in another bug.

@@ +100,5 @@
> +     {'mozprofile': os.path.join('tests', 'mozbase', 'mozprofile')},
> +     {'mozprocess': os.path.join('tests', 'mozbase', 'mozprocess')},
> +     {'mozrunner': os.path.join('tests', 'mozbase', 'mozrunner')},
> +     {'peptest': os.path.join('tests', 'peptest')},
> +    ]

You can probably start with a minimum set and add as you see it fail. Probably easier when you trigger this on buildbot on all platforms.

@@ +126,5 @@
> +                    Please ensure that if the '--run-all-suites' flag was enabled
> +                    then do not specify to run only specific suites like '--mochitest-suite browser-chrome'""")
> +        self.glob_test_options = []
> +        self.glob_mochi_options = []
> +        self.xpcshell_options = []

I don't see 'self.xpcshell_options' being used.

@@ +139,5 @@
> +
> +    ###### helper methods
> +
> +    def check_if_valid_config(self):
> +        suite_categories = ['mochitests', 'reftests', 'xpcshell']

I wonder if this list should be defined explicitly in the config files rather than being a magic list.

@@ +145,5 @@
> +        if not c.get('run_all_suites'):
> +            return True # configs are valid
> +
> +        is_valid = True
> +        for cat in suite_categories:

I prefer "category" or "c" but this is just an opinion.

@@ +149,5 @@
> +        for cat in suite_categories:
> +            specific_suites = c.get('specified_{cat}_suites'.format(cat=cat))
> +            if specific_suites:
> +                if specific_suites != 'all':
> +                    is_valid = False

I am not sure what are we validating in this for loop.

Are we just checking that we don't run the script with something like this?
--xpcshell-suite all

Is this what we are trying to avoid?

@@ +169,5 @@
> +            dirs['abs_app_install_dir'], 'firefox')
> +        dirs['abs_app_plugins_dir'] = os.path.join(
> +            dirs['abs_app_dir'], 'plugins')
> +        dirs['abs_app_components_dir'] = os.path.join(
> +            dirs['abs_app_dir'], 'components')

Would this look better?

dirs = {
   'abs_app_install_dir': os.path.join(abs_dirs['abs_work_dir'], 'application'),
   'abs_app_dir': os.path.join(dirs['abs_app_install_dir'], 'firefox'),
   ...
}

I think we can look at this section together and try to figure out a cleaner way.
I believe you are trying to check that if keys in *dirs* does not exist in *abs_dirs* then add it to *abs_dir*.
You could probably do it on a for loop or doing something like abs_dir.get(key_you_want, value_you_want_if_key_you_want_does_not_exist_in_abs_dir) (if a in dictionary return dict['a'] else return value b)

@@ +274,5 @@
> +            Please make sure you are either:
> +                    (1) specifing it in the config file under binary_path
> +                    (2) specifing it on command line with the '--binary-path' flag""")
> +
> +    def query_glob_mochi_options(self, **kwargs):

"query_glob_mochitest_options"

Are "query_glob_{reftest,xpcshell}_options" needed?

@@ +318,5 @@
> +        return suites
> +
> +    def copy_tree(self, src, dest, log_level='info', error_level='error'):
> +        """an implementation of shutil.copytree however it allows
> +        you to copy to a 'dest' that already exists"""

Should this be in a library class? I might not be making sense so feel free to discuss back or disregard.

@@ +349,5 @@
> +    def pull_other_repos(self):
> +        dirs = self.query_abs_dirs()
> +
> +        if self.config.get('repos'):
> +            dirs = self.query_abs_dirs()

IIUC it seems that you are running this twice.

@@ +380,5 @@
> +
> +    def run_tests(self):
> +        self.mochitests()
> +        self.reftests()
> +        self.xpcshell()

I almost feel that we should not have to have categories at all.
I almost feel that we should only call mozharness with --suite and let the script determine what to do.

The functions mochitests(), reftests() and xpcshell() are very much alike.
I feel that we might be making things a little more difficult for us in the long term or fixing things in one place rather than another.

aki, what do you think?
I know this would be substantial change but I believe worth considering it now rather than later.

I am also OK with getting off my horse since both approaches would still work.

@@ +448,5 @@
> +        """run tests for xpcshell"""
> +        c = self.config
> +        dirs = self.query_abs_dirs()
> +        app_xpcshell_path = os.path.join(dirs['abs_app_dir'], c['xpcshell_name'])
> +        bin_xpcshell_path = os.path.join(dirs['abs_test_bin_dir'], c['xpcshell_name'])

I wouldn't create these 2 variables just to use it once in:
self.copyfile(bin_xpcshell_path, app_xpcshell_path)
but that's just my opinion.

Comment 20

[Armen [:armenzg]]

jlund, I am adding a new step to UnittestPackagedFactory on bug 712630.

This patch should give you an idea of what we are doing:
https://bugzilla.mozilla.org/attachment.cgi?id=631495

Could you please add this to your mozharness work?

Notice that the step runs for all Windows slaves *but* the code only does something for Windows 7 32-bit machines:
http://hg.mozilla.org/build/tools/file/default/scripts/support/mouse_and_screen_resolution.py#l43

Let me know if you have any questions.

Comment 21

[Jordan Lund (:jlund)]

Attachment: mozharness patch for unittests

Comment 22

[Jordan Lund (:jlund)]

Attachment: mozharness unittests patch for buildbot-configs

made the mozharness unittests for only 'opt' ATM and on the 'elm' branch
also added new reboot format

Comment 23

[Jordan Lund (:jlund)]

Attachment: extended maxtime for ScriptFactory

Comment 24

[Armen [:armenzg]]

Comment on attachment 639559 [details] [diff] [review]
mozharness patch for unittests

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

This is very very good.
r- because there are a bunch of nits that I would like to see addressed before landing on production.

::: configs/unittests/mac_unittest.py
@@ +11,5 @@
> +    "installer_path" : INSTALLER_PATH,
> +    "binary_path" : APP_NAME_DIR + "/" + BINARY_PATH,
> +    "xpcshell_name" : XPCSHELL_NAME,
> +    # TODO find out if I need simple_json_url
> +    "buildbot_json_path": "buildprops.json",

I don't see where you use this.

@@ +69,5 @@
> +    "all_xpcshell_suites" : {
> +        "xpcshell" : ["--manifest=tests/xpcshell/tests/all-test-dirs.list",
> +            "application/" + APP_NAME_DIR + "/" + XPCSHELL_NAME]
> +    },
> +    "preflight_run_cmd_suites" : [

Adding a note for Aki.
jlund and I spoke about this section. jlund wanted to keep all three cfg files the same and keep differences at the top.
I thought of asking to keep the "disable_screen_saver" for Linux and keep "run mouse and screen adjustment" for Windows.

Let me know what you prefer.

::: mozharness/base/script.py
@@ +197,5 @@
> +        'nothing' will keep all(any) existing files in destination tree
> +        'update' will only overwrite destination paths that have
> +                   the same path names relative to the root of the src and
> +                   destination tree
> +        'all' will replace the whole destination tree(clobber) if it exists"""

I suggest "no_overwrite/overwrite_if_exists/clobber". Your call.

Or just reduce to the cases that you really need.

::: scripts/desktop_unittest.py
@@ +34,5 @@
> +            {
> +                "action" : "append",
> +                "dest" : "specified_mochitest_suites",
> +                "type": "string",
> +                "help": """Specify which mochi suite to run.

*mochitest

@@ +82,5 @@
> +    ] + copy.deepcopy(testing_config_options)
> +
> +    error_list = [
> +            {'regex': re.compile(r'''^TEST-UNEXPECTED-FAIL'''), 'level': WARNING,
> +                'explanation' : "this unittest unexpectingly failed. This is a harness error"},

Can you please align the white spacing of this line?

@@ +122,5 @@
> +        c = self.config
> +        self.global_test_options = []
> +        self.abs_dirs = None
> +
> +        self.installer_url = c.get('installer_url')

I sometimes wonder if installer is the correct term to refer the Firefox binaries since on Linux we have no installer but a tar ball with the binaries (unlike Windows and Mac).
Not that I have suggestion of a better name.

@@ +141,5 @@
> +            if specific_suites:
> +                if specific_suites != 'all':
> +                    self.fatal("""Config options are not valid.
> +Please ensure that if the '--run-all-suites' flag was enabled
> +then do not specify to run only specific suites like '--mochitest-suite browser-chrome'""")

We can also take --run-all-suites as overwriting all other options if you prefer.

Is _pre_config_lock a meaningful name for what we're doing inside of it?

Can you please add a comment before the for loop saying "We're just ensuring that no other options have been 

Instead of "specific_suites" I think it makes more sense to name the variable "specified_suite" (IIUC).
You can probably put the two if lines in the same one "if specific_suites != None and specific_suites != 'all'" (I believe you want the != None)

I also believe you want to do:
for category in SUITE_CATEGORIES:
   for specified_suite in c.get('specified_%s_suites' % (category), []):
       if specified_suite != 'all':
           blah blah

My understanding is that you can call the script with more than one --reftest_suite and that specified_reftest_suites is a list.

@@ +209,5 @@
> +            options = []
> +            run_file = c['run_file_names'][suite_category]
> +            base_cmd = [self.query_python_path('python')]
> +            # TODO in buildbot only xpcshell is run with the '-u' (force stdin)
> +            # flag? should all unittests be run with this in mozharness?

jmaher/ctalbert could probably answer this.

@@ +292,5 @@
> +            self._extract_test_zip(target_unzip_dirs=unzip_tests_dirs)
> +        self._download_installer()
> +
> +    def pull(self):
> +        dirs = self.query_abs_dirs()

Why do you initialize "dirs" in here? It seems you do it again down inside the "if statement".

@@ +312,5 @@
> +                # platforms like mac as excutable files may be universal
> +                # files containing multiple architectures
> +                # NOTE 'enabled' is only here while we have unconsolidated configs
> +                if suite['enabled'] and \
> +                        platform.architecture()[0] in suite['architectures']:

This is a good reason to make the "preflight_run_cmd_suites" on each config file to contain only what applies for that platform.

@@ +319,5 @@
> +                        'name' : suite['name'],
> +                        'cmd' : ' '.join(suite['cmd'])})
> +                    # TODO rather then checking for formatting on every string
> +                    # in every preflight enabled cmd: find a better solution!
> +                    # maybe I can implement WithProperties in mozharness?

Perhaps you can file a follow up bug for this. Not necessary to be tackled before end of internship.

@@ +330,5 @@
> +            self.warning("""Proceeding without running prerun test commands.
> +These are often OS specific and disabling them may result in spurious test results!""")
> +
> +    def run_tests(self):
> +

Extra white line.

@@ +383,5 @@
> +                    level = ERROR
> +                self.add_summary("The %s suite: %s test ran with return code \
> +                        %s: %s" % (suite_category, suites[num], code, status),
> +                        level=level)
> +                ####

Not sure what to say about this section.
If it is still WIP then remove it until completed and leave what is needed.

Comment 25

[Armen [:armenzg]]

Comment on attachment 639562 [details] [diff] [review]
mozharness unittests patch for buildbot-configs

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

::: mozilla-tests/config.py
@@ +1249,5 @@
> +    {'suite_name' : 'mozharness_jsreftest', 'suite_category' : 'reftest', 'sub_categories' : ['jsreftest']},
> +    {'suite_name' : 'mozharness_crashtest', 'suite_category' : 'reftest', 'sub_categories' : ['crashtest']},
> +    {'suite_name' : 'mozharness_xpcshell', 'suite_category' : 'xpcshell', 'sub_categories' : ['xpcshell']}
> +]
> +# TODO elm?

Is this still TODO? Or does it work for you?

@@ +1268,5 @@
> +            config_file = "unittests/win_unittest.py"
> +        elif pf.startswith("mac"):
> +            config_file = "unittests/mac_unittest.py"
> +        else:
> +            config_file = "unittests/linux_unittest.py"

Can you please add an elif for linux?
Also add an else case to complain that we should have not reach the else statement and abort.

@@ +1279,5 @@
> +                for sub_category in suite['sub_categories']:
> +                    extra_args += ["--%s-suite" % suite['suite_category'], sub_category]
> +                BRANCHES[branch]['platforms'][pf][slave_pf]['opt_unittest_suites'] += [
> +                    (suite['suite_name'], {
> +                        'mozharness_repo': 'http://hg.mozilla.org/users/jlund_mozilla.com/mozharness',

We need to point to build/mozharness.

@@ +1283,5 @@
> +                        'mozharness_repo': 'http://hg.mozilla.org/users/jlund_mozilla.com/mozharness',
> +                        'script_path': 'scripts/desktop_unittest.py',
> +                        'extra_args': extra_args,
> +                        'reboot_command': reboot_command,
> +                        'hg_bin': hg_bin,

I think we should specify hg_bin and reboot_command on the config files.
If we wanted to run it manually.

Correct me if I understand this incorrectly.

Comment 26

[Aki Sasaki (not active)]

(In reply to Armen Zambrano G. [:armenzg] - Release Engineer from comment #25)
> > +                        'hg_bin': hg_bin,
> 
> I think we should specify hg_bin and reboot_command on the config files.
> If we wanted to run it manually.
> 
> Correct me if I understand this incorrectly.

I think hg_bin is only for the ScriptFactory to check out mozharness to begin with.
Once that happens it's based on the config file + PATH.

Comment 27

[Aki Sasaki (not active)]

(In reply to Armen Zambrano G. [:armenzg] - Release Engineer from comment #24)
> > +    "buildbot_json_path": "buildprops.json",
> 
> I don't see where you use this.

We use this in the read-buildbot-config action:
http://hg.mozilla.org/build/mozharness/file/d3c3f3d4a935/mozharness/mozilla/buildbot.py#l39

> ::: mozharness/base/script.py
> @@ +197,5 @@
> > +        'nothing' will keep all(any) existing files in destination tree
> > +        'update' will only overwrite destination paths that have
> > +                   the same path names relative to the root of the src and
> > +                   destination tree
> > +        'all' will replace the whole destination tree(clobber) if it exists"""
> 
> I suggest "no_overwrite/overwrite_if_exists/clobber". Your call.

If naming becomes an issue here, we could make this 0,1,2 or -1,0,1.


> Is _pre_config_lock a meaningful name for what we're doing inside of it?

This is actually a mozharness built-in.
Happy to rename this or have _pre_config_lock call a dummy function with a more descriptive name if it's an issue.


There, I should do my own review passes now that I've commented on Armen's :)

Comment 28

[Aki Sasaki (not active)]

Comment on attachment 639562 [details] [diff] [review]
mozharness unittests patch for buildbot-configs

Lgtm with Armen's nits.
Do you have a staging master where I can look at this?

Comment 29

[Jordan Lund (:jlund)]

Attachment: parsing and logging accordingly for unittests

this is still not perfect but I think it is working enough to 'bring to the table'

Comment 30

[Jordan Lund (:jlund)]

Attachment: diffed against upstream/master

mozharness desktop unittests, parsing/logging features, and reflected changes based on above feedback.

Comment 31

[Jordan Lund (:jlund)]

woops. sorry Aki. I made the previous patch obselete as I thought your comment 28 was feedback for mozharness not buildbot-configs. I can remove my latest full patch(comment 30) if you would like to continue reviewing my previous one(comment 21). All that is different is the addition of attachment 642861 [details] [diff] [review] and Armen's feedback. Whatever is easiest.

Comment 32

[Jordan Lund (:jlund)]

Comment on attachment 642863 [details] [diff] [review]
diffed against upstream/master

WIP updated patch coming today

Comment 33

[Aki Sasaki (not active)]

No worries, I can review whatever you need reviewed.

Comment 34

[Aki Sasaki (not active)]

Comment on attachment 642861 [details] [diff] [review]
parsing and logging accordingly for unittests

First off, sorry for the delay; I didn't notice this was still pending feedback.

>      def __init__(self, config=None, log_obj=None, error_list=None,
> -                 log_output=True):
> +                 log_output=True, status_levels=None):
>          self.config = config
>          self.log_obj = log_obj
>          self.error_list = error_list
>          self.log_output = log_output
>          self.num_errors = 0
> +        self.num_warnings = 0
>          # TODO context_lines.
>          # Not in use yet, but will be based off error_list.
>          self.context_buffer = []
>          self.num_pre_context_lines = 0
>          self.num_post_context_lines = 0
> -        # TODO set self.error_level to the worst error level hit
> -        # (WARNING, ERROR, CRITICAL, FATAL)
> -        # self.error_level = INFO
> +        self.result_log_level = INFO
> +        if status_levels:
> +            self.result_status_level = status_levels[-1]

Is there a reason we ask for status_levels as a list, and then ignore all of it but the last value?

If we go this route, maybe save self.status_levels = status_levels, and you can stop passing status_levels to add_lines().  More on this later.

> +                        if error_check.get('status_level'):
> +                            status_level = error_check['status_level']
> +                            if not status_levels:
> +                                self.fatal("result_status requires status_levels" + \
> +                                        "to determine worst status_level")
> +                            self.result_status_level = self.worst_level(status_level,
> +                                    self.result_status_level, levels=status_levels)

We may end up calling worst_level() thousands or millions of times here.

Would it work if we:

a) dropped status_levels completely
b) dropped worst_level
c) kept self.num_warnings
d)

    parser = OutputParser(config=c, log_obj=self.log_obj,
            error_list=suite_category_error_list)
    parser.add_lines(output)
    if parser.num_errors:
        tbpl_status = TBPL_FAILURE
        # or TBPL_RETRY or TBPL_EXCEPTION depending on what we want
    elif parser.num_warnings:
        tbpl_status = TBPL_WARNING

This would be pretty lightweight, though it would require that we put this little type of logic in every place we want it.

However, status_levels might be useful later if implemented properly.
I'm largely concerned with lack of comments/docs as to how to use it, passing status_levels in multiple times, and calling worst_level per line of output.

More on this below.

> +                    # TODO ask Aki
> +                    # if level is FATAL then will any lines below ever happen?

Not unless someone is catching the exception.  I generally assume fatal is fatal, and if someone wants to break the harness, they can deal with the mess themselves ;-)

> +def create_tinderbox_summary(suite_name, pass_count, fail_count,
> +        known_fail_count=False, crashed=False, leaked=False):

This might belong in mozharness/mozilla/testing/.

> +    def log_tinderbox_println(self, suite_name, output, full_re_substr, pass_name,
> +            fail_name, known_fail_name=None):
> +        """appends 'TinderboxPrint: foo, summary' to the output"""
> +        full_re = re.compile(full_re_substr)
> +        harness_errors_re = re.compile(r"TEST-UNEXPECTED-FAIL \| .* \| (Browser crashed \(minidump found\)|missing output line for total leaks!|negative leaks caught!|leaked \d+ bytes during test execution)")

Do these not change the tbpl status? (asking, not sure)
If they do, we should add to the error_list.  Also, see below.

> +            for suite in suites:
> +                cmd =  abs_base_cmd + suites[suite]
> +                output = self.get_output_from_command(cmd,
> +                        cwd=dirs['abs_work_dir'], silent=True)

I'm concerned about a few things:
1) that this is going to be a *lot* of output on some jobs,
2) that we're then parsing all that output twice, once for the error_list and once for TinderboxPrint stuff, and
3) that we're going to run the tests which are going to have no output to the logs or the screen for the duration of the test.

I was hoping that you'd get everything you needed by adding num_warnings to OutputParser and adding a 'parser' return_type to run_command().

Looking at your code, I see that you need additional information saved: how many tests out of how many passed/known/failed, whether there was a crash, etc.

I'm still kind of hoping we can do the run_command and parser route, and maybe add a feature to error_lists: save_line=True?
So you would add those to the error lines that you need to parse. e.g.

    'mochitest' : BaseTestError  + [
        {'regex' : re.compile(r'''(\tFailed: [^0]|\d+ INFO Failed: [^0])'''),
         'level' : WARNING, 'explanation' : "One or more unittests failed",
         'save_line': True,
        },
        {'regex' : re.compile(r'''(\tFailed: 0|\d+ INFO Failed: 0)'''),
         'level' : INFO, 'save_line': True,
        },

You'd have a similar check for the crashed regex, negative leaks caught, etc.

OutputParser would then have self.saved_lines, and you could iterate over those handful instead of having to go through the entire output again.

(If we need to put lambdas into error_lists for smarter error checking, I'll consider it but won't necessarily be thrilled.)


This is a pretty large change from what you're doing.
I'm hoping this will make things a lot cleaner, but I'm worried I might be overlooking something that your code fixes and my suggestion will break again; does this sound correct?

If this will work, and you need help, I'm happy to help.  Or explain further.
If this will not work, let me know where my thinking is wrong?

Comment 35

[Armen [:armenzg]]

Comment on attachment 642861 [details] [diff] [review]
parsing and logging accordingly for unittests

I will let you deal with aki's comments first.
I would prefer if he gives the review and I give the feedback as he understands better what you're doing.

Comment 36

[Aki Sasaki (not active)]

(In reply to Aki Sasaki [:aki] from comment #34)
> I was hoping that you'd get everything you needed by adding num_warnings to
> OutputParser and adding a 'parser' return_type to run_command().

Thought about this last night.
We could also pass in a parser to run_command(..., parser=None), which would allow us to clone a child of OutputParser or otherwise tweak parser settings before calling run_command.

Comment 37

[Jordan Lund (:jlund)]

Attachment: mozharness patch: diffed against github's mozilla/master

This is a proposed patch for my latest mozharness changes. This includes everything done this far. 

For notes see 'working on/known issues' in: https://etherpad.mozilla.org/8IoZrTUXAG

still WIP: 

1) configs are unconsolidated so major redundancy. eventually, after we see if we need branch/arch/platformVersion specific configs, I could make an initial config file with the duplicate code.

2) for buildbot-configs set mozharness unittests to be run on cedar

3) after WIP #1 is addressed, I would like to figure out solution for developer configs. ATM we have a developer config for linux working but this needs to be run for any OS.

4) I am not parsing for PythonErrorLists as its triggering match's when it shouldn't during certain suites. 
I added the following to elem 0 in error_list:
{'regex': re.compile('''(TEST-INFO|TEST-KNOWN-FAIL|TEST-PASS|INFO \| )'''), 'level': INFO},
however this regex is growing contsantly with new finds on new tests. For example, the latest 'false' PythonErrorList match was for this line:
14:42:45    ERROR -  *** WARN addons.xpi: Download failed: TypeError: aChannel.securityInfo is null

for those with build vpn and are curious to see logs of this script running in testing:
http://dev-master01.build.scl1.mozilla.com:8038/builders 
*note leopard is not supported with firefox 17 which explains the oranges for that slave. The other reds are for the PythonErrorList mentioned above (of which I disabled for this patch)

this patch lives in the following repo: https://github.com/lundjordan/mozharness/tree/linux64-unittesting

Comment 38

[Jordan Lund (:jlund)]

Attachment: same as before with the addition of one line

this is the same as my previous attachment bar one line addition. I should have known better then to test and then change the code slightly.

What I did was I removed error_list from my custom OutputParser(error_list=None). By doing so this breaks everything. This is because run_command normally instantiates OutputParser and, by doing so, it will pass error_list as [] if it was None.

Comment 39

[Jordan Lund (:jlund)]

I should also note that I am still getting a clobber error on the occasional windows build (talos-r3-w7-010): http://dev-master01.build.scl1.mozilla.com:8038/builders/Rev3%20WINNT%206.1%20mozilla-central%20opt%20test%20mozharness_mochitests-5%2F5/builds/1/steps/run_script/logs/stdio

It is always from .hg:
WindowsError: [Error 145] The directory is not empty: 'c:\\talos-slave\\test\\build\\tools\\.hg\\store\\data\\mozilla\\tools'

the rmtree uses this method: http://hg.mozilla.org/build/mozharness/file/d16f44376c54/mozharness/base/script.py#l55

rmtree uses _rmdir_recursive() for windows

Comment 40

[Jordan Lund (:jlund)]

(In reply to Jordan Lund (:jlund) from comment #39)
> I should also note that I am still getting a clobber error on the occasional
> windows build (talos-r3-w7-010):

this could be non related to my patch. I think it might be the way I was testing on my dev-master. ie, stopping, changing, and starting my master while slaves are in mid-build.

Comment 41

[Aki Sasaki (not active)]

(In reply to Jordan Lund (:jlund) from comment #40)
> (In reply to Jordan Lund (:jlund) from comment #39)
> > I should also note that I am still getting a clobber error on the occasional
> > windows build (talos-r3-w7-010):
> 
> this could be non related to my patch. I think it might be the way I was
> testing on my dev-master. ie, stopping, changing, and starting my master
> while slaves are in mid-build.

If that's it, a reboot before the next test run could help.
We also see similar issues on Windows in production, outside of mozharness, so it could be that as well.

Comment 42

[Aki Sasaki (not active)]

Comment on attachment 651981 [details] [diff] [review]
same as before with the addition of one line

>diff --git a/mozharness/mozilla/buildbot.py b/mozharness/mozilla/buildbot.py
>index 034ea87..e141b09 100755
>--- a/mozharness/mozilla/buildbot.py
>+++ b/mozharness/mozilla/buildbot.py
>@@ -16,6 +16,7 @@ sys.path.insert(1, os.path.dirname(sys.path[0]))
> 
> from mozharness.base.config import parse_config_file
> from mozharness.base.log import INFO, WARNING, ERROR
>+from mozharness.base.log import OutputParser

pyflakes says "mozharness/mozilla/buildbot.py:19: 'OutputParser' imported but unused"
This means we don't have to change mozharness/mozilla/buildbot.py at all.

>diff --git a/mozharness/mozilla/testing/testbase.py b/mozharness/mozilla/testing/testbase.py
>index 232f158..4f5f727 100755
>--- a/mozharness/mozilla/testing/testbase.py
>+++ b/mozharness/mozilla/testing/testbase.py
>@@ -94,7 +94,7 @@ class TestingMixin(VirtualenvMixin, BuildbotMixin):
>             if missing:
>                 self.fatal("%s!" % (message % ('+'.join(missing))))
>         else:
>-            self.fatal("self.buildbot_config isn't set after running read_buildbot_config!")
>+            self.warning("self.buildbot_config isn't set after running read_buildbot_config!")

Is there a reason you're changing this to warning()?
I think I explicitly wanted read_buildbot_config to work, or kill the test run.
If you don't want to read the buildbot config, you can skip the action via --no-read-buildbot-config or removing it from the default list of actions.

Is there a different reason this is here?

>@@ -186,15 +188,17 @@ Did you run with --create-virtualenv? Is mozinstall in virtualenv_modules?""")
>         # Remove the below when we no longer need to support mozinstall 0.3
>         self.info("Detecting whether we're running mozinstall >=1.0...")
>         output = self.get_output_from_command(cmd + ['-h'])
>-        if '--source' in output:
>-            cmd.append('--source')
>         # End remove
>         dirs = self.query_abs_dirs()
>         target_dir = dirs.get('abs_app_install_dir',
>                               os.path.join(dirs['abs_work_dir'],
>                              'application'))
>         self.mkdir_p(target_dir)
>-        cmd.extend([self.installer_path,
>-                    '--destination', target_dir])
>+        cmd.extend(['--destination', target_dir, self.installer_path])
>+        # TODO find out if --source is deprectiated

SP deprecated :)

>+        if '--source' in output:
>+            for index, value in enumerate(cmd):
>+                if value == self.installer_path:
>+                    cmd.insert(index, '--source')

It seems like these changes do the exact same thing as before, except the --source and --destination options are reversed.  Are you hitting a scenario where the order is wrong?

>diff --git a/mozharness/base/log.py b/mozharness/base/log.py
>+    def worst_level(self, target_level, existing_level, levels=None):
>+        """returns either existing_level or target level.
>+        This depends on which is closest to levels[0]
>+        By default, levels is the list of log levels"""
>+        if not levels:
>+            levels = [FATAL, CRITICAL, ERROR, WARNING, INFO]
>+        if target_level not in levels:
>+            self.fatal("'%s' not in %s'." % (target_level, levels))
>+        for l in levels:
>+            if l in (target_level, existing_level):
>+                return l

Hm.
I'm not an expert at optimizing number of operations, but would this be faster if this were a dict rather than iterating over a list?

levels = {FATAL: 5, CRITICAL: 4, ERROR: 3 ...
then compare levels[target_level] against levels[existing_level] ?

Also, should this allow for DEBUG and IGNORE?

>diff --git a/scripts/desktop_unittest.py b/scripts/desktop_unittest.py
>+from mozharness.base.errors import PythonErrorList, BaseErrorList
>+from mozharness.mozilla.testing.errors import TinderBoxPrintRe, TestPassed

Errors from pyflakes:
scripts/desktop_unittest.py:19: 'PythonErrorList' imported but unused
scripts/desktop_unittest.py:20: 'TestPassed' imported but unused

This looks like fallout from the last minute error_list change, no worries.

>+        else:
>+            self.fatal("self.installer_url was not found in self.config") 

Nit: trailing whitespace.

>+            else:
>+                self.warning("""Suite options for %s could not be determined.
>+If you meant to have options for this suite, please make sure they are specified
>+in your config under %s_options""" % suite_category, suite_category)

I miss these parens all the time too.
...""" % (suite_category, suite_category))

>+        else:
>+            self.fatal("""'binary_path' could not be determined.
>+            This should be something like '/root/path/with/build/application/firefox/firefox-bin'
>+            If you are running this script without the 'install' action (where binary_path is set),
>+            Please make sure you are either:
>+                    (1) specifying it in the config file under binary_path
>+                    (2) specifying it on command line with the '--binary-path' flag""")

I'd love to figure out how to either specify this, and have mozinstall put the binary where we specify, or to predict where it'll be based on various things.

>+        # I am disabling parsing for PythonErrorList in run-tests as the suites themselves
>+        # are conficting and triggering match's when there shouldn't be
>+        # error_list = TestPassed + PythonErrorList

You can skip PythonErrorList, but I think you should replace it with something.

Ideally any TEST-UNEXPECTED-'s or the like would be marked as WARNING at the very least.  I'd also like known infrastructure errors marked accordingly.  But yes, I imagine PythonErrorList's error levels are wrong for what you're trying to do.

(Is it mainly BaseErrorList that's tripping you up? Or are some of the other lines triggering false positives as well?)


I re-read this.  I think overall the main issue I'd balk at r+ing is the read_buildbot_config() fatal->warning.  Could you get back to me on that?

Seems like we're close to being able to land and test on cedar.

Comment 43

[Jordan Lund (:jlund)]

Attachment: made some changes as to Aki's latest review. Explanation coming in next comment

Comment 44

[Jordan Lund (:jlund)]

> pyflakes says "mozharness/mozilla/buildbot.py:19: 'OutputParser' imported
> but unused"
> This means we don't have to change mozharness/mozilla/buildbot.py at all.

woops, should have caught that. Fixed

> Is there a reason you're changing this to warning()?
> I think I explicitly wanted read_buildbot_config to work, or kill the test
> run.
> If you don't want to read the buildbot config, you can skip the action via
> --no-read-buildbot-config or removing it from the default list of actions.
> 
> Is there a different reason this is here?

I meant to add a comment in the code and review (I only had it on the etherpad buried)
Sorry I know I should have brought it up. My thought process was I wanted this action to be able to be skipped if buildbot_json_path was not set or found. After seeing this: https://github.com/mozilla/mozharness/blob/c12e59857de26d4a57cfc264fb5c3bbbfdfe409d/mozharness/mozilla/buildbot.py#L43

I thought that was what you wanted as well but I realize now that you can always manually set the buildbot_config without the json path. My reasoning for wanting this action to not collapse the script was that I wanted to elliminate the need for developers to always explicitly know and say not to run that action. Maybe I should have looked into making this action not run by default.



> SP deprecated :)

> It seems like these changes do the exact same thing as before, except the
> --source and --destination options are reversed.  Are you hitting a scenario
> where the order is wrong?

this was when moxinstall had a bug in it. I was reading a bit of the source and I thought order might matter. I am testing it now after reverting it back to the current master branch state (but without SP). Will let you know findings.

> Hm.
> I'm not an expert at optimizing number of operations, but would this be
> faster if this were a dict rather than iterating over a list?
> 
> levels = {FATAL: 5, CRITICAL: 4, ERROR: 3 ...
> then compare levels[target_level] against levels[existing_level] ?
> 
> Also, should this allow for DEBUG and IGNORE?

Right makes sense, before I implement this I just wanted to mention that I wanted this method to work for not just log levels but say buildbot status levels like I am doing here:
https://github.com/lundjordan/mozharness/blob/bba1535fc0dc09b934e6286d5ba55dd46d51c30d/scripts/desktop_unittest.py#L68

Maybe I should not be doing so. Although, If you think this is OK, then I suppose I should change the param 'levels' type to a dict as well so that this method is optimized for everything.

> Errors from pyflakes:
> scripts/desktop_unittest.py:19: 'PythonErrorList' imported but unused
> scripts/desktop_unittest.py:20: 'TestPassed' imported but unused
> 
> This looks like fallout from the last minute error_list change, no worries.

removed for now

> >+        else:
> >+            self.fatal("self.installer_url was not found in self.config") 
> 
> Nit: trailing whitespace.

woops. fixed

> >+            else:
> >+                self.warning("""Suite options for %s could not be determined.
> >+If you meant to have options for this suite, please make sure they are specified
> >+in your config under %s_options""" % suite_category, suite_category)
> 
> I miss these parens all the time too.
> ...""" % (suite_category, suite_category))

ahh had to google why that is dangerous and can cause spurious results. 


> I'd love to figure out how to either specify this, and have mozinstall put
> the binary where we specify, or to predict where it'll be based on various
> things.

I recall a bug opened on this. Maybe I should take a look into this.

> You can skip PythonErrorList, but I think you should replace it with
> something.
> 
> Ideally any TEST-UNEXPECTED-'s or the like would be marked as WARNING at the
> very least.  I'd also like known infrastructure errors marked accordingly. 
> But yes, I imagine PythonErrorList's error levels are wrong for what you're
> trying to do.

So I thought I was catching all 'TEST-UNEXPECTED' here: https://github.com/lundjordan/mozharness/blob/bba1535fc0dc09b934e6286d5ba55dd46d51c30d/scripts/desktop_unittest.py#L76

My custom OutputParser should be doing all the suite fails/harness errors. The base OutputParser I was using for mozharness errors with PythonErrorList since the suites are python files: https://github.com/lundjordan/mozharness/blob/bba1535fc0dc09b934e6286d5ba55dd46d51c30d/scripts/desktop_unittest.py#L92

> (Is it mainly BaseErrorList that's tripping you up? Or are some of the other
> lines triggering false positives as well?)

It is mainly PythonErrorList that is tripping me. I have not used BaseErrorList. things like 'TypeError:' are riddled in the test suites. and the start of each output line that contains 'TypeError:' can be pretty random ie: 'known-test-fail', '*** warning', 'info-test', etc. (see comment 37, WIP number 4)

> 
> I re-read this.  I think overall the main issue I'd balk at r+ing is the
> read_buildbot_config() fatal->warning.  Could you get back to me on that?
> 
> Seems like we're close to being able to land and test on cedar.

Cool, as always, thanks for the detailed review. :)

Comment 45

[Jordan Lund (:jlund)]

Attachment: made some changes as to Aki's latest review. Explanation coming in next comment

sorry, missed a couple lines in this diff.

I am testing this out on my slaves tonight making sure I broke nothing and will follow up with a full diff after I address the rest of Aki's concerns and my questions from comment 44.

Comment 46

[Aki Sasaki (not active)]

(In reply to Jordan Lund (:jlund) from comment #44)
> I meant to add a comment in the code and review (I only had it on the
> etherpad buried)
> Sorry I know I should have brought it up. My thought process was I wanted
> this action to be able to be skipped if buildbot_json_path was not set or
> found. After seeing this:
> https://github.com/mozilla/mozharness/blob/
> c12e59857de26d4a57cfc264fb5c3bbbfdfe409d/mozharness/mozilla/buildbot.py#L43

Yeah.
I suppose my thinking was:
a) you have two outs: don't set the buildbot_json_path, or don't run the action.  If both are set, you're requiring it, and we should fail out.
b) we don't set the buildbot_json_path or run the action by default.  It's only the production config files that set those.
c) We can create developer-centric configs that don't set those if that's helpful.

There might be better ways of doing this, but we need a way to require the buildbot json in production.

Comment 47

[Jordan Lund (:jlund)]

Attachment: hg diff of buildbot-configs to support mozharness desktop unittests with cedar

Comment 48

[Jordan Lund (:jlund)]

Attachment: hg diff of mozharness desktop unittests

everything up to date with latest tested changes.

a detailed summary of flow/logic can accompany this if you feel that would help. 

Aki, if you would like I can produce another diff of all the latest changes so you can see what is new. Thanks in advance :)

Comment 49

[Jordan Lund (:jlund)]

Attachment: hg diff of mozharness desktop unittests (with new jsreftest changes)

Comment 50

[Aki Sasaki (not active)]

Comment on attachment 658573 [details] [diff] [review]
hg diff of mozharness desktop unittests (with new jsreftest changes)

I think I want to see this landed after the two non-nits below are resolved.

>+    def worst_level(self, target_level, existing_level, levels=None):
>+        """returns either existing_level or target level.
>+        This depends on which is closest to levels[0]
>+        By default, levels is the list of log levels"""
>+        if not levels:
>+            levels = [IGNORE, FATAL, CRITICAL, ERROR, WARNING, INFO, DEBUG]

I think you want IGNORE after DEBUG (least worst status, as opposed to worse-than-FATAL).

>     def install(self):
>         """ Dependent on mozinstall """
>         # install the application
>         cmd = self.query_exe("mozinstall", default=self.query_python_path("mozinstall"), return_type="list")
>         # Remove the below when we no longer need to support mozinstall 0.3
>         self.info("Detecting whether we're running mozinstall >=1.0...")
>         output = self.get_output_from_command(cmd + ['-h'])
>-        if '--source' in output:
>-            cmd.append('--source')

Is there a reason you're removing the --source option?
I think this will break mozinstall >= 1.0.

>+# DesktopUnittest {{{1
>+class DesktopUnittest(TestingMixin, MercurialScript):
>+
>+    config_options = [
>+        [['--mochitest-suite', ], {
>+            "action": "append",
>+            "dest": "specified_mochitest_suites",
>+            "type": "string",
>+            "help": """Specify which mochi suite to run.
>+            Suites are defined in the config file.
>+            Examples: 'all', 'plain1', 'plain5', 'chrome', or 'a11y'"""}

Nit: This indentation, though easier to read in code, is harder to read in --help, which appears to ignore newlines.

This isn't a blocking issue by any means; we can clean up the help output later.

Comment 51

[Aki Sasaki (not active)]

Attachment: fix those 2 nits

If only to avoid another pass of reviews of that huge patch :)

Comment 52

[Aki Sasaki (not active)]

Comment on attachment 658341 [details] [diff] [review]
hg diff of buildbot-configs to support mozharness desktop unittests with cedar

I think this works.

Two things:

* I'd like to talk about python 2.5 vs 2.7 -- we're using python 2.5 in mozharness_python in mozilla-tests/config.py, and you have python 2.5 in one place in the windows config in mozharness.  I'm leaning towards moving everything to 2.7 if it works.

* I think some people are unhappy with this jetpack/peptest style for loop.  Maybe we should go through all the branches and only do this if branch.get('mozharness_unittests') instead of |for branch in ['cedar']:|, but in a way I'd like to see this live and make tweaks later, especially since once this is rolled out fully we'll be able to get rid of the non-mozharness unittest config.

Comment 53

[Jordan Lund (:jlund)]

> >+    def worst_level(self, target_level, existing_level, levels=None):
> >+        """returns either existing_level or target level.
> >+        This depends on which is closest to levels[0]
> >+        By default, levels is the list of log levels"""
> >+        if not levels:
> >+            levels = [IGNORE, FATAL, CRITICAL, ERROR, WARNING, INFO, DEBUG]
> 
> I think you want IGNORE after DEBUG (least worst status, as opposed to
> worse-than-FATAL).
> 

ya, I wasn't sure where you wanted IGNORE. I suppose I put it there in case you were thinking of order when you defined mozharness log levels:
https://github.com/mozilla/mozharness/blob/d53e34c5beb8d8b46f18bcf40e4c65d4a7610343/mozharness/base/log.py#L25

I will change it now.

Comment 54

[Jordan Lund (:jlund)]

> Is there a reason you're removing the --source option?
> I think this will break mozinstall >= 1.0.
>

Don't laugh. I thought on your last review when you wrote:
"""
> >+        # TODO find out if --source is deprectiated
> SP deprecated :)
"""
I actually read that as you were affirming for me 'Source Path is deprecated' not 'Spelling nit'.

> Nit: This indentation, though easier to read in code, is harder to read in
> --help, which appears to ignore newlines.

Ya. I tried cleaning up all my long strings. I have found that rather then using the triple quote (which allows pretty indentation but ugly output), I started using strings like the help string in run-all-suites which addresses both concerns. Will this be alright? Is there a better way?:

[['--run-all-suites', ], {
            "action": "store_true",
            "dest": "run_all_suites",
            "default": False,
            "help": "This will run all suites that are specified"
                    "in the config file. You do not need to specify "
                    "any other suites.\nBeware, this may take a while."}
],

Comment 55

[Jordan Lund (:jlund)]

Attachment: same patch with: aki's 2 non-nits patch plus 'help string' nit

Comment 56

[Aki Sasaki (not active)]

(In reply to Jordan Lund (:jlund) from comment #54)
> > Is there a reason you're removing the --source option?
> > I think this will break mozinstall >= 1.0.
> >
> 
> Don't laugh. I thought on your last review when you wrote:
> """
> > >+        # TODO find out if --source is deprectiated
> > SP deprecated :)
> """
> I actually read that as you were affirming for me 'Source Path is
> deprecated' not 'Spelling nit'.

Haha.

> Ya. I tried cleaning up all my long strings. I have found that rather then
> using the triple quote (which allows pretty indentation but ugly output), I
> started using strings like the help string in run-all-suites which addresses
> both concerns. Will this be alright? Is there a better way?:
> 
> [['--run-all-suites', ], {
>             "action": "store_true",
>             "dest": "run_all_suites",
>             "default": False,
>             "help": "This will run all suites that are specified"
>                     "in the config file. You do not need to specify "
>                     "any other suites.\nBeware, this may take a while."}
> ],

1) this results in no space between 'specified' and 'in'
2) When I need newlines, I tend to use triple quotes and just start the next line at the beginning of the line, e.g.

                      """this is line 1
this is line 2
this is line 3"""

and when I need an extra long line I tend to ignore wrapping rules.

                      "this is an extra long line that will wrap in my editor, but i'm not going to bother splitting it up."

But this is fine.

Comment 57

[Aki Sasaki (not active)]

Comment on attachment 658885 [details] [diff] [review]
same patch with: aki's 2 non-nits patch plus 'help string' nit

http://hg.mozilla.org/build/mozharness/rev/ca3b37a3b8ff

Comment 58

[Jordan Lund (:jlund)]

> * I'd like to talk about python 2.5 vs 2.7 -- we're using python 2.5 in
> mozharness_python in mozilla-tests/config.py,   I'm leaning towards moving
> everything to 2.7 if it works.
> 
Looking at mozilla-tests/config.py:
['mozharness_python'] points to '/tools/buildbot/bin/python' for both mac and linux and 'c:/mozilla-build/python25/python' for windows

looking at my current slaves I am getting:

for linux:
[cltbld@talos-r3-fed64-010 ~]$ /tools/buildbot/bin/python --version
Python 2.6.2
[cltbld@talos-r3-fed64-010 ~]$ whereis python
python: /usr/bin/python2.6-config /usr/bin/python2.6 /usr/bin/python /usr/lib/python2.6 /usr/lib64/python2.6 /usr/include/python2.6 /usr/share/man/man1/python.1.gz

for my mac machine: python 2.7.x

for windows (obviously): python 2.5.x

so it seems that pointing to '/tools/buildbot/bin/python' will point to 2.7 or 2.6 depending on if its installed (in my linux64's case its not). should I leave it as is for both OS's?

for windows I can switch 'c:/mozilla-build/python25/python' to 'c:/mozilla-build/python27/python' and test it out tonight.

> and you have python 2.5 in one
> place in the windows config in mozharness.

I put them there as the mouse_and_screen step for windows requires a pywin32 module that is only in 2.5. wlach is working at removing that module dependency: https://bugzilla.mozilla.org/show_bug.cgi?id=711299#c22

> * I think some people are unhappy with this jetpack/peptest style for loop.  Maybe we 
> should go through all the branches and only do this if 
> branch.get('mozharness_unittests') instead

done.

Comment 59

[Jordan Lund (:jlund)]

Attachment: hg diff of buildbot-configs from mozharness desktop unittests

same as attachment 658341 [details] [diff] [review] but with mozharness_python pointing to 2.7 for windows and added 'mozharness_unittests' key to cedar branch.

This was successfully tested with yesterday before I lost access to my master.

I can not say how changing windows to python 2.7 will affect other configs using mozharness_python.

Feel free to ask questions/concerns. I can follow up in my spare time.

:)

Comment 60

[Aki Sasaki (not active)]

Comment on attachment 659528 [details] [diff] [review]
hg diff of buildbot-configs from mozharness desktop unittests

This is close.  Sorry I didn't catch the below earlier:
1) we don't want to keep using your user repo now that it landed on build/mozharness :)
2) this adds a 2nd set of unittests for mozharness, rather than replacing the old unittests.

I think we can fix (2) by removing the mozharness_ name in the unittest suites, and zeroing out opt_unittest_suites before the |for suite in| loop.

We'll have to do the same for debug at some point, but if we can get opt running first that's a great first step.

I can do the above unless you're still wanting to work on this post-internship :)

Comment 61

[Aki Sasaki (not active)]

Attachment: this fixes it

Applies on top of your configs patch, and removes the other opt unittests in favor of the mozharness ones.

Comment 62

[Aki Sasaki (not active)]

Attachment: oops, without user repo

Comment 63

[Jordan Lund (:jlund)]

Comment on attachment 659918 [details] [diff] [review]
oops, without user repo

>+                BRANCHES[branch]['platforms'][pf][slave_pf]['opt_unittest_suites'] = []

I had this in my loop for testing. I took it out as I thought originally we were going to run this in parallel against buildbot's suites.
I probably understood that wrong. mozharness desktop unittests are probably perfect anyway so there is no need (eyes roll).

>-                            'mozharness_repo': 'http://hg.mozilla.org/users/jlund_mozilla.com/mozharness',
>+                            'mozharness_repo': MOZHARNESS_REPO,

woops! I should have caught that. Maybe subconsciously I just wanted my nick (jlund) somewhere in my src code

Comment 64

[Jordan Lund (:jlund)]

Comment on attachment 659918 [details] [diff] [review]
oops, without user repo

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

looks good to me!

Comment 65

[Jordan Lund (:jlund)]

(In reply to Jordan Lund (:jlund) from comment #64)
> Comment on attachment 659918 [details] [diff] [review]
> oops, without user repo
> 
> Review of attachment 659918 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> looks good to me!

hmm seems I have 'lost' the power to review (give the 'r+'). Hopefully this is suffice: r+ ;) r=jlund

Comment 66

[Aki Sasaki (not active)]

Comment on attachment 659918 [details] [diff] [review]
oops, without user repo

Thanks Jordan!

I think there are some bugzilla privs your account needs set to r... did you change accounts or just change the email address for the same account?

Comment 67

[Aki Sasaki (not active)]

Comment on attachment 659528 [details] [diff] [review]
hg diff of buildbot-configs from mozharness desktop unittests

r+ with my patch applied.
http://hg.mozilla.org/build/buildbot-configs/rev/8e8ea80b4029

Comment 68

[Aki Sasaki (not active)]

Comment on attachment 659918 [details] [diff] [review]
oops, without user repo

http://hg.mozilla.org/build/buildbot-configs/rev/f26688327b38

This should go live on Cedar the next reconfig (tomorrow?)
Now to rebase my talos patches.

Comment 69

[Aki Sasaki (not active)]

We have a number of green mozharness opt unit tests on Cedar:
https://tbpl.mozilla.org/?tree=Cedar&rev=2e3c1ce191f5

We still need to deal with debug tests, deal with any issues, and roll out to the other branches when ready.

I think I'm going to file new bugs for those.

Comment 70

[Aki Sasaki (not active)]

Filed bug 793017 and bug 793022.
I blocked the mozbase bugs on those, since it looks like those will want all desktop unit tests on mozharness before we switch over.  Let me know if I missed other blocked bugs.

Comment 71

[Jeff Hammel]

I'm actually not sure why we need to block on this.  mozbase is already in the tree and usable in that form, unless there is something i am missing

Comment 72

[Aki Sasaki (not active)]

Hm, I thought we couldn't assume we could use mozbase until buildbot-run unit tests could set up a venv, but ok.  Feel free to unblock if it's not blocking.

Comment 73

[(not currently active) Ted Mielczarek]

(In reply to Aki Sasaki [:aki] from comment #72)
> Hm, I thought we couldn't assume we could use mozbase until buildbot-run
> unit tests could set up a venv, but ok.  Feel free to unblock if it's not
> blocking.

No, you're correct. We can't use mozbase for the unit tests until we have a virtualenv on the test slaves.