Closed Bug 1194166 Opened 9 years ago Closed 9 years ago

Update mozharness configs for mach try changes

Categories

(Testing :: General, defect)

Product:
Testing
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox43 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla43
Tracking Flags:
Tracking Status
firefox43 --- fixed

People

(Reporter: jgraham, Unassigned)

Details

Attachments

(1 file)

MozReview Request: Bug 1194166 - Update unittest mozconfigs for all platforms
40 bytes, text/x-review-board-request
chmanchester
: review+
jlund
: review+
Details 
This shouldn't be a seperate bug, but I'm working around reviewboard limitations.
Bug 1194166 - Update unittest mozconfigs for all platforms
Attachment #8647458 - Flags: review?(ahalberstadt)
Attachment #8647458 - Flags: review?(ahalberstadt) → review?(cmanchester)
Comment on attachment 8647458 [details]
MozReview Request: Bug 1194166 - Update unittest mozconfigs for all platforms

Bug 1194166 - Update unittest mozconfigs for all platforms
Comment on attachment 8647458 [details]
MozReview Request: Bug 1194166 - Update unittest mozconfigs for all platforms

https://reviewboard.mozilla.org/r/15989/#review15779

::: testing/mozharness/configs/unittests/linux_unittest.py:94
(Diff revision 1)
> -                                'MOZ_DISABLE_CONTEXT_SHARING_GLX': '1'},
> -                        'options': ['--setpref=browser.tabs.remote=true',
> +        "crashtest": {
> +            'options': ["--suite=crashtest"],
> +            'tests': ["tests/reftest/tests/testing/crashtest/crashtests.list"]

You've introduced a mix of single and double quotes here.

::: testing/mozharness/configs/unittests/linux_unittest.py:98
(Diff revision 1)
> +        "jsreftest": {
> +            'options':["--extra-profile-file=tests/jsreftest/tests/user.js"],

Do jsreftests need a "suite" argument?

::: testing/mozharness/configs/unittests/linux_unittest.py:108
(Diff revision 1)
> +                        '--setpref=browser.tabs.remote=true',

This pref has been defunct since bug 1072417

::: testing/mozharness/configs/unittests/linux_unittest.py:120
(Diff revision 1)
> +                'MOZ_OMTC_ENABLED': '1',
> -                                  'MOZ_DISABLE_CONTEXT_SHARING_GLX': '1'},
> +                'MOZ_DISABLE_CONTEXT_SHARING_GLX': '1'},

This dictionary's formatting is inconsistent with the one above.

::: testing/mozharness/scripts/android_panda.py:226
(Diff revision 1)
> +            "xpcshell": [("xpcshell", "xpcshell")],

One of these android platforms has xpcshell in multiple chunks, is that dealt with somewhere?

::: testing/mozharness/scripts/b2g_desktop_unittest.py:221
(Diff revision 1)
> -        cmd = self.append_harness_extra_args(cmd)
> +        try_options, try_tests = self.try_args(suite_name)
> +        cmd.extend(try_options)
> +
> +        if tests_list:
> +            cmd.append("--")
> +            cmd.extend(tests_list)

This is really a lot of copy paste... the only significant difference I see is some platforms have a second level of naming for suite, can we stick some of this into a method in testbase with an optional second parameter?

This mostly looks good. It would be helpful to look at this side by side with the mozharness parts of the other bug.

This represents a pretty widespread change to how to do options in mozharness, it would be good to make sure someone like Andrew or Jordan is at least aware of it before landing.
Attachment #8647458 - Flags: review?(cmanchester)
https://reviewboard.mozilla.org/r/15989/#review15779

> This pref has been defunct since bug 1072417

OK, but that doesn't seem like a change that should happen as part of this bug since the pref is still there on inbound. Filed bug 1200547

> One of these android platforms has xpcshell in multiple chunks, is that dealt with somewhere?

They run in multiple chunks on try, at least: https://treeherder.mozilla.org/#/jobs?repo=try&revision=28d458e59a18

> This is really a lot of copy paste... the only significant difference I see is some platforms have a second level of naming for suite, can we stick some of this into a method in testbase with an optional second parameter?

This got moved to a later patch in the series; let's discuss it there.
Attachment #8647458 - Flags: feedback?(jlund)
Attachment #8647458 - Flags: feedback?(ahalberstadt)
ahal, jlund: chmanchester suggested that I make you aware of the changes in this patch.
Comment on attachment 8647458 [details]
MozReview Request: Bug 1194166 - Update unittest mozconfigs for all platforms

Bug 1194166 - Update unittest mozconfigs for all platforms
Attachment #8647458 - Flags: review?(cmanchester)
Attachment #8647458 - Flags: feedback?(jlund)
Attachment #8647458 - Flags: feedback?(ahalberstadt)
Comment on attachment 8647458 [details]
MozReview Request: Bug 1194166 - Update unittest mozconfigs for all platforms

Bug 1194166 - Update unittest mozconfigs for all platforms
https://reviewboard.mozilla.org/r/15989/#review15779

> They run in multiple chunks on try, at least: https://treeherder.mozilla.org/#/jobs?repo=try&revision=28d458e59a18

I guess this moved to a different commit so I dropped the issue.
Comment on attachment 8647458 [details]
MozReview Request: Bug 1194166 - Update unittest mozconfigs for all platforms

https://reviewboard.mozilla.org/r/15989/#review16091

::: testing/mozharness/configs/android/androidarm.py:190
(Diff revision 3)
> +            "tests": ["../jsreftest/tests/jstests.list",]

Some of these one-liner lists have a trailing comma inside the list, some outside. There should be only be one outside.

::: testing/mozharness/scripts/b2g_desktop_unittest.py:166
(Diff revision 3)
> +
> +        if tests:
> +            cmd.append("--")
> +            cmd.extend(item % str_format_values for item in tests)
> +

I can't quite tell what happened here, were some of these chunks meant to be moved to a different commit?
Attachment #8647458 - Flags: review?(cmanchester)
https://reviewboard.mozilla.org/r/15989/#review16095

::: testing/mozharness/configs/b2g/emulator_automation_config.py
(Diff revision 3)
> -                "tests/testing/crashtest/crashtests.list"
>              ],
> +            "tests": ["tests/testing/crashtest/crashtests.list",],

Missing suite=crashtest ?
https://reviewboard.mozilla.org/r/15989/#review16091

> I can't quite tell what happened here, were some of these chunks meant to be moved to a different commit?

I think it's OK? It looks for a "tests" key in self.config["suite_definitions"][suite] and if it finds one adds the list of tests to the command. In later commits this is extended to also consider try_tests.
Comment on attachment 8647458 [details]
MozReview Request: Bug 1194166 - Update unittest mozconfigs for all platforms

Bug 1194166 - Update unittest mozconfigs for all platforms
Attachment #8647458 - Flags: review?(cmanchester)
https://reviewboard.mozilla.org/r/15989/#review16091

> I think it's OK? It looks for a "tests" key in self.config["suite_definitions"][suite] and if it finds one adds the list of tests to the command. In later commits this is extended to also consider try_tests.

Then we're back to the issue of introducing duplicate code.
https://reviewboard.mozilla.org/r/15989/#review16091

> Then we're back to the issue of introducing duplicate code.

OK, but can we look at that in the later commit where this code will change again?
https://reviewboard.mozilla.org/r/15989/#review16091

> OK, but can we look at that in the later commit where this code will change again?

Sure.
Attachment #8647458 - Flags: review?(cmanchester) → review+
Comment on attachment 8647458 [details]
MozReview Request: Bug 1194166 - Update unittest mozconfigs for all platforms

https://reviewboard.mozilla.org/r/15989/#review16227

I still think we should get jlund to sign off on this before landing.

::: testing/mozharness/scripts/android_emulator_unittest.py:488
(Diff revision 4)
> +            tests = self.test_suite_definitions[self.test_suite].get("tests")

No need to use get, the condition checks for the key.

::: testing/mozharness/scripts/android_emulator_unittest.py:494
(Diff revision 4)
> +            cmd.extend([test % str_format_values for test in tests])

None of the strings in the "tests" are format strings, isn't using str_format_values here and elsewhere a noop?

::: testing/mozharness/scripts/androidx86_emulator_unittest.py:455
(Diff revision 4)
> +        if "tests" in self.suite_definitions[suite_name]:
> +            cmd.append("--")
> +            cmd.extend(self.suite_definitions[suite_name]["tests"])
> +        elif self.tree_config["suite_definitions"][suite_category].get("tests"):

Inconsistent use of "get" and "in" for equivalent checks.

::: testing/mozharness/scripts/b2g_desktop_unittest.py:160
(Diff revision 4)
> +        tests = self.config["suite_definitions"][suite].get("tests")

Move this below the next block so it's closer to where it's used.
Comment on attachment 8647458 [details]
MozReview Request: Bug 1194166 - Update unittest mozconfigs for all platforms

Bug 1194166 - Update unittest mozconfigs for all platforms
Attachment #8647458 - Flags: review?(jlund)
Comment on attachment 8647458 [details]
MozReview Request: Bug 1194166 - Update unittest mozconfigs for all platforms

https://reviewboard.mozilla.org/r/15989/#review16311

thanks for informing me about the test-wide change. I only glanced at this as chris seems to have combed it over in detail. ateam has largely taken over the test infra so feel free to do as you like. In the future if you need suite wide change approval, ahal would probably be a better person to approve.

giving this a ship-it assuming you address chmanchester's final issues and the missing 'xpcshell-addons' key in win_unittest.py

::: testing/mozharness/configs/unittests/win_unittest.py:211
(Diff revision 4)
>      "all_xpcshell_suites": {
> -        "xpcshell": ["--manifest=tests/xpcshell/tests/all-test-dirs.list",
> -                     "%(abs_app_dir)s/" + XPCSHELL_NAME],
> -        "xpcshell-addons": ["--manifest=tests/xpcshell/tests/all-test-dirs.list",
> +        "xpcshell": {
> +            'options': ["--xpcshell=%(abs_app_dir)s/" + XPCSHELL_NAME,
> +                        "--manifest=tests/xpcshell/tests/all-test-dirs.list"],
> +            'tests': []
> +        },
> +        "xpcshell": {
> +            'options': ["--xpcshell=%(abs_app_dir)s/" + XPCSHELL_NAME,

looks like there are dupe keys here, should the second 'xpcshell' be be 'xpcshell-addons' ?

::: testing/mozharness/scripts/b2g_emulator_unittest.py:286
(Diff revision 4)
> +            cmd.append("--")

so I understand it, if len(tests) > 1: the cmd will look like ```options + '--' + test1 + test2 + ...```

so all the test harness suites know that any cmd that has a '--' will finish with a list of tests to run?

::: testing/mozharness/scripts/desktop_unittest.py:561
(Diff revision 4)
> -                    options_list = suites[suite]['options']
> +                    options_list = suites[suite]['options'] + suites[suite].get("tests", [])

ooc, how come the desktop unittests don't have a '--' inbetween options and tests like the other platforms do?
Attachment #8647458 - Flags: review?(jlund) → review+
https://reviewboard.mozilla.org/r/15989/#review16311

> so I understand it, if len(tests) > 1: the cmd will look like ```options + '--' + test1 + test2 + ...```
> 
> so all the test harness suites know that any cmd that has a '--' will finish with a list of tests to run?

Yes, this patch is part of a series of patches that will ensure all the affected harnesses accept the same syntax.

> ooc, how come the desktop unittests don't have a '--' inbetween options and tests like the other platforms do?

Ah, I just realised that only this case is right and the others are wrong because of the way that append_harness_extra_args is used to modify the command later.
Comment on attachment 8647458 [details]
MozReview Request: Bug 1194166 - Update unittest mozconfigs for all platforms

Bug 1194166 - Update unittest mozconfigs for all platforms
Comment on attachment 8647458 [details]
MozReview Request: Bug 1194166 - Update unittest mozconfigs for all platforms

Chris: Mind having a final look over this?
Attachment #8647458 - Flags: review+ → review?(cmanchester)
Comment on attachment 8647458 [details]
MozReview Request: Bug 1194166 - Update unittest mozconfigs for all platforms

https://reviewboard.mozilla.org/r/15989/#review16565

::: testing/mozharness/scripts/b2g_emulator_unittest.py:278
(Diff revisions 4 - 5)
>          tests = self.config["suite_definitions"][suite].get("tests", [])

This should go below the next block, like the last chunk.
Attachment #8647458 - Flags: review?(cmanchester) → review+
I'm going to try landing this again. For the record I have a try run which looks reasonable: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0c0173143fe6 (but I thought I had last time too!)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f35a6da56d63&exclusion_profile=false is, I think now as green as these tests get. So I'm going to try landing. Again.
https://hg.mozilla.org/mozilla-central/rev/5c715c7f22c3
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox43: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: