Last Comment Bug 811723 - change android reftests to run with --ignore-window-size for panda only
: change android reftests to run with --ignore-window-size for panda only
Status: RESOLVED FIXED
:
Product: Release Engineering
Classification: Other
Component: Platform Support (show other bugs)
: other
: x86 Mac OS X
: -- normal (vote)
: ---
Assigned To: Kim Moir [:kmoir]
: Chris Cooper [:coop]
Mentors:
Depends on: 821728 830868 830877
Blocks:
  Show dependency treegraph
 
Reported: 2012-11-14 07:20 PST by Kim Moir [:kmoir]
Modified: 2013-08-12 21:55 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
buildbotcustom patch (3.97 KB, patch)
2012-11-29 15:17 PST, Kim Moir [:kmoir]
bugspam.Callek: feedback+
Details | Diff | Splinter Review
buildbot-configs patch (3.40 KB, patch)
2012-11-29 15:24 PST, Kim Moir [:kmoir]
bugspam.Callek: feedback+
Details | Diff | Splinter Review
buildbotcustom patch (3.95 KB, text/plain)
2012-12-03 11:00 PST, Kim Moir [:kmoir]
no flags Details
buildbot-configs patch (3.18 KB, patch)
2012-12-03 11:02 PST, Kim Moir [:kmoir]
no flags Details | Diff | Splinter Review
buildbotcustom patch (4.96 KB, patch)
2012-12-04 12:44 PST, Kim Moir [:kmoir]
no flags Details | Diff | Splinter Review
patch for buildbot-configs (3.20 KB, patch)
2012-12-04 12:47 PST, Kim Moir [:kmoir]
no flags Details | Diff | Splinter Review
buildbotcustom patch (5.96 KB, patch)
2012-12-07 09:24 PST, Kim Moir [:kmoir]
bugspam.Callek: review+
kmoir: checked‑in+
Details | Diff | Splinter Review
buildbot-configs patch (4.71 KB, patch)
2012-12-07 09:29 PST, Kim Moir [:kmoir]
bugspam.Callek: review-
Details | Diff | Splinter Review
buildbot-configs patch (4.71 KB, patch)
2012-12-11 13:32 PST, Kim Moir [:kmoir]
bugspam.Callek: review+
emorley: feedback+
kmoir: checked‑in+
Details | Diff | Splinter Review

Description Kim Moir [:kmoir] 2012-11-14 07:20:17 PST
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] 2012-11-29 15:17:33 PST
Created attachment 686824 [details] [diff] [review]
buildbotcustom patch
Comment 2 Kim Moir [:kmoir] 2012-11-29 15:24:37 PST
Created attachment 686829 [details] [diff] [review]
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] 2012-12-03 06:05:49 PST
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) 2012-12-03 10:04:01 PST
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) 2012-12-03 10:05:20 PST
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) 2012-12-03 10:07:20 PST
(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] 2012-12-03 11:00:46 PST
Created attachment 687868 [details]
buildbotcustom patch
Comment 8 Kim Moir [:kmoir] 2012-12-03 11:02:05 PST
Created attachment 687869 [details] [diff] [review]
buildbot-configs patch

The extra lines in the first patch were just a cut and paste error.
Comment 9 Kim Moir [:kmoir] 2012-12-03 11:09:17 PST
Regarding comment 6, let me confirm with :jmaher that this is a panda-only requirement.
Comment 10 Joel Maher ( :jmaher) 2012-12-03 11:14:22 PST
this is a must have for pandas, but we should do it for tegras as well!
Comment 11 Kim Moir [:kmoir] 2012-12-04 12:44:01 PST
Created attachment 688416 [details] [diff] [review]
buildbotcustom patch
Comment 12 Kim Moir [:kmoir] 2012-12-04 12:47:34 PST
Created attachment 688419 [details] [diff] [review]
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] 2012-12-07 09:24:30 PST
Created attachment 689770 [details] [diff] [review]
buildbotcustom patch
Comment 14 Kim Moir [:kmoir] 2012-12-07 09:29:41 PST
Created attachment 689774 [details] [diff] [review]
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) 2012-12-11 06:26:28 PST
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] 2012-12-11 10:57:57 PST
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) 2012-12-11 12:20:34 PST
(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] 2012-12-11 13:32:55 PST
Created attachment 691040 [details] [diff] [review]
buildbot-configs patch
Comment 19 Justin Wood (:Callek) 2012-12-11 13:37:08 PST
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] 2012-12-11 13:42:49 PST
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 [:aki] 2012-12-12 13:21:51 PST
This is in production.
Comment 22 Ben Hearsum (:bhearsum) 2012-12-13 08:30:24 PST
in production
Comment 23 Joel Maher ( :jmaher) 2012-12-13 10:11:49 PST
is this closeable now?
Comment 24 Kim Moir [:kmoir] 2012-12-13 10:30:30 PST
Yes, just verified that this is working in tbpl.
Comment 25 Steve Fink [:sfink] [:s:] (PTO Sep23-28) 2012-12-19 15:58:13 PST
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:] (PTO Sep23-28) 2012-12-20 14:04:16 PST
Never mind, that change is already there. I must've updated one repo and not the other or something.

Note You need to log in before you can comment on or make changes to this bug.