811723 - change android reftests to run with --ignore-window-size for panda only

Status: RESOLVED FIXED

(Infrastructure & Operations Graveyard :: CIDuty, task)

Description

[Kim Moir [:kmoir] ET]

From :jmaher

Right now we will not be able to run reftests on panda boards until we add a --ignore-window-size to the job.  This will be safe for m-c based branches because we have had a few patches land which make this work when our resolution is smaller.  

The problem is this won't work on Aurora and other branches, so we cannot do a blanket addition of --ignore-window-size and expect things to work.

Since we do not require the larger resolution on m-c, it would save 80 minutes of tegra time per push if we could stop changing the resolution for R1-R4.  The only logical way to do this is to change the job name from reftest to something else, then our sut_tools and buildbot scripts can treat this new job type appropriately.

Talked to Ben and he said it looks like that's a runreftest.py option: http://mxr.mozilla.org/mozilla-central/source/layout/tools/reftest/runreftest.py#227

Comment 1

[Kim Moir [:kmoir] ET]

Attachment: buildbotcustom patch

Comment 2

[Kim Moir [:kmoir] ET]

Attachment: buildbot-configs patch

Not sure if this is the best way to implement this but it stops the resolution change from occurring on reftests on pandas.  I haven't added the changes to limit it to mozilla-central based branches, not sure the list of branches this would entail.

Comment 3

[Kim Moir [:kmoir] ET]

So I'm not sure how to implement this to so changing the reftest only runs on m-c, m-i, project branches, and try.  I looked at the example of the mozharness tests where they set a flag mozharness_unittests if this applies to a branch, not sure if this would be a good approach here.  Suggestions?

Comment 4

[Justin Wood (:Callek)]

Comment on attachment 686829 [details] [diff] [review]
buildbot-configs patch

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

::: mozilla-tests/config.py
@@ +682,5 @@
>  
> +for suite in ANDROID_UNITTEST_DICT['opt_unittest_suites']:
> +    if suite[0].startswith('reftest') or suite[0].startswith('crashtest'):
> +        continue
> +    ANDROID_NOION_UNITTEST_DICT['opt_unittest_suites'].append(suite)

I'm confused on why this is here (I don't see any specific change that is requiring it).

@@ +729,5 @@
> +        continue
> +    ANDROID_PANDA_UNITTEST_DICT['opt_unittest_suites'].append(suite)
> +
> +for suite in ANDROID_PANDA_REFTEST_DICT['opt_unittest_suites']:
> +    ANDROID_PANDA_UNITTEST_DICT['opt_unittest_suites'].append(suite)

I suspect ben won't be too happy with the proliferation of 'magic'/foreach type stuff we have here, but I can't think of a not-too-much-work alternative.

Comment 5

[Justin Wood (:Callek)]

Comment on attachment 686824 [details] [diff] [review]
buildbotcustom patch

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

::: steps/unittest.py
@@ +664,5 @@
>                         ]
>          if suite == 'jsreftest' or suite == 'crashtest':
>              self.command.append('--ignore-window-size')
> +        if suite == 'reftest' and extra_args:
> +           self.command.append(extra_args)

nit, just check on extra_args and use it if it exists? rather than needing to manually check the suite.

Comment 6

[Justin Wood (:Callek)]

(In reply to Kim Moir [:kmoir] from comment #3)
> So I'm not sure how to implement this to so changing the reftest only runs
> on m-c, m-i, project branches, and try.  I looked at the example of the
> mozharness tests where they set a flag mozharness_unittests if this applies
> to a branch, not sure if this would be a good approach here.  Suggestions?

If I'm understanding correctly that this is panda-only need, and we don't need to worry about it with tegras until all our tegra-caring branches pass through then this should be enough for pandas. If we do need to use this for tegras we'll need to try and think of how best to pass this for trunk branches only.

Or at least that's accurate as long as pandas won't skip to a closer-to-release train on us.

Comment 7

[Kim Moir [:kmoir] ET]

Attachment: buildbotcustom patch

Comment 8

[Kim Moir [:kmoir] ET]

Attachment: buildbot-configs patch

The extra lines in the first patch were just a cut and paste error.

Comment 9

[Kim Moir [:kmoir] ET]

Regarding comment 6, let me confirm with :jmaher that this is a panda-only requirement.

Comment 10

[Joel Maher ( :jmaher ) (UTC -8)]

this is a must have for pandas, but we should do it for tegras as well!

Comment 11

[Kim Moir [:kmoir] ET]

Attachment: buildbotcustom patch

Comment 12

[Kim Moir [:kmoir] ET]

Attachment: patch for buildbot-configs

I changed the name of the suite today so the reftests could be removed from the appropriate branches and reftestsmall could be added.  However, I can't determine how to enable this on a per branch basis, I tried this several different ways but they don't work, so any suggestions on how to implement this would be welcomed :-)

Comment 13

[Kim Moir [:kmoir] ET]

Attachment: buildbotcustom patch

Comment 14

[Kim Moir [:kmoir] ET]

Attachment: buildbot-configs patch

I think this solves the problems with the branches, this doesn't apply to most branches yet for pandas because they are only enabled on cedar at present.

The builders look good on my dev-master.

I'll run some tests once the pandas are reimaged and up again on our test master.

Comment 15

[Justin Wood (:Callek)]

Comment on attachment 689774 [details] [diff] [review]
buildbot-configs patch

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

the tldr; is based on what I'm reading I feel doing a new dict might be the wrong idea here, and we could/should then just extend the normal dict and then use MERGE_DAY related branch checks to delete the items that do not belong [branch dependant] e.g. reftestsmall on aurora+ and reftest[-normal] on trunk-based-branches.

marking r- based on my opinion here, but I can be convinced on the current approach/choice[s] if you disagree with my thoughts.

::: mozilla-tests/config.py
@@ +835,5 @@
>              'host_utils_url': 'http://bm-remote.build.mozilla.org/tegra/tegra-host-utils.%%(foopy_type)s.742597.zip',
>              'enable_opt_unittests': True,
>              'enable_debug_unittests': False,
>              'remote_extras': ANDROID_UNITTEST_REMOTE_EXTRAS,
> +            'tegra_android': deepcopy(ANDROID_PANDA_UNITTEST_DICT),

umm, _PANDA_ for tegra_android feels wrong. Intended?

@@ +1091,5 @@
>  
> +# Load reftests small for m-c based branches and exclude them for the rest
> +for branch in ('mozilla-aurora', 'mozilla-beta', 'mozilla-release'):
> +    BRANCHES[branch]["platforms"]["android"]["panda_android"]["opt_unittest_suites"] = deepcopy(ANDROID_UNITTEST_DICT["opt_unittest_suites"])
> +    BRANCHES[branch]["platforms"]["android"]["tegra_android"]["opt_unittest_suites"] = deepcopy(ANDROID_UNITTEST_DICT["opt_unittest_suites"])

Given this I might be inclined to add the reftestsmall to the plain ANDROID_UNITESTS_DICT and trim out the wrong ones depending on branch, and attach it as part of merge-day work. 

Thoughts?

Comment 16

[Kim Moir [:kmoir] ET]

I can change the name ANDROID_PANDA_UNITTEST_DICT to ANDROID_PLAIN_UNITTEST_DICT.

I tried your suggestions this morning - one dictionary worked fine of course, 
but I couldn't get per branch deletes by suite type to work.  I tried using removeSuite but that didn't work. How would you suggest deleting suites by branch?

Comment 17

[Justin Wood (:Callek)]

(In reply to Kim Moir [:kmoir] from comment #16)
> I tried your suggestions this morning - one dictionary worked fine of
> course, 
> but I couldn't get per branch deletes by suite type to work.  I tried using
> removeSuite but that didn't work. How would you suggest deleting suites by
> branch?

Ok after spending the last 20ish minutes trying to come up with the right magic to remove these on a branch-specific means I couldn't come up with a good way, so I'm inclined to accept your patch rather than have either of us spend more time wrestling with this.

Effort vs Reward doesn't feel correct to me for my ideal to have either of us spend more time.

Comment 18

[Kim Moir [:kmoir] ET]

Attachment: buildbot-configs patch

Comment 19

[Justin Wood (:Callek)]

Comment on attachment 691040 [details] [diff] [review]
buildbot-configs patch

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

Looks good, f? to ed for the "does tbpl need adjustments"

The magic here is "plain-reftest-N" is a replacement for "reftest-N" for android so we can use a new arg for branches that can ride the train.

::: mozilla-tests/config.py
@@ +1101,5 @@
>              BRANCHES[branch]['platforms'][pf][slave_pf]['opt_unittest_suites'] += [('jetpack', ['jetpack'])]
>              BRANCHES[branch]['platforms'][pf][slave_pf]['debug_unittest_suites'] += [('jetpack', ['jetpack'])]
>  
> +# Load reftests small for m-c based branches and exclude them for the rest
> +for branch in ('mozilla-aurora', 'mozilla-beta', 'mozilla-release'):

Nit: Add a MERGE DAY comment here

Comment 20

[Ed Morley [:emorley]]

Comment on attachment 691040 [details] [diff] [review]
buildbot-configs patch

Yup, this should be fine - thank you for checking :-)

(The two places we match use /reftest/ and /(?:mochitest|reftest|crashtest)\-([0-9]+)/ respectively, so both will be fine.)

OrangeFactor will likely need updating for the testsuite filter dropdowns, but they already need overhauling, so I'll just add to the list (bug 817269).

Comment 21

[Aki Sasaki (not active)]

This is in production.

Comment 22

[[e-mail] (:bhearsum)]

in production

Comment 23

[Joel Maher ( :jmaher ) (UTC -8)]

is this closeable now?

Comment 24

[Kim Moir [:kmoir] ET]

Yes, just verified that this is working in tbpl.

Comment 25

[Steve Fink [:sfink] [:s:]]

I haven't confirmed directly, but I think this caused some test-masters.sh failures. I needed to do this in buildbotcustom/steps/unittest.py to fix them:

-        elif suite in ('reftest', 'direct3D', 'opengl'):
+        elif suite in ('reftest', 'direct3D', 'opengl', 'reftestsmall'):

Comment 26

[Steve Fink [:sfink] [:s:]]

Never mind, that change is already there. I must've updated one repo and not the other or something.