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

RESOLVED FIXED

Status

Release Engineering
Platform Support
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: kmoir, Assigned: kmoir)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 7 obsolete attachments)

(Assignee)

Description

5 years ago
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
(Assignee)

Updated

5 years ago
Assignee: nobody → kmoir
(Assignee)

Comment 1

5 years ago
Created attachment 686824 [details] [diff] [review]
buildbotcustom patch
(Assignee)

Comment 2

5 years ago
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.
(Assignee)

Comment 3

5 years ago
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?
(Assignee)

Updated

5 years ago
Attachment #686824 - Flags: feedback?(bugspam.Callek)
(Assignee)

Updated

5 years ago
Attachment #686829 - Flags: feedback?(bugspam.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.
Attachment #686829 - Flags: feedback?(bugspam.Callek) → feedback+
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.
Attachment #686824 - Flags: feedback?(bugspam.Callek) → feedback+
(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.
(Assignee)

Comment 7

5 years ago
Created attachment 687868 [details]
buildbotcustom patch
Attachment #686824 - Attachment is obsolete: true
Attachment #687868 - Flags: review?(bugspam.Callek)
(Assignee)

Comment 8

5 years ago
Created attachment 687869 [details] [diff] [review]
buildbot-configs patch

The extra lines in the first patch were just a cut and paste error.
Attachment #686829 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #687869 - Flags: review?(bugspam.Callek)
(Assignee)

Comment 9

5 years ago
Regarding comment 6, let me confirm with :jmaher that this is a panda-only requirement.
this is a must have for pandas, but we should do it for tegras as well!
(Assignee)

Comment 11

5 years ago
Created attachment 688416 [details] [diff] [review]
buildbotcustom patch
Attachment #687868 - Attachment is obsolete: true
Attachment #687868 - Flags: review?(bugspam.Callek)
(Assignee)

Comment 12

5 years ago
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 :-)
Attachment #687869 - Attachment is obsolete: true
Attachment #687869 - Flags: review?(bugspam.Callek)
(Assignee)

Comment 13

5 years ago
Created attachment 689770 [details] [diff] [review]
buildbotcustom patch
Attachment #688416 - Attachment is obsolete: true
Attachment #689770 - Flags: review?(bugspam.Callek)
(Assignee)

Comment 14

5 years ago
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.
Attachment #688419 - Attachment is obsolete: true
Attachment #689774 - Flags: review?(bugspam.Callek)

Updated

5 years ago
Attachment #689770 - Flags: review?(bugspam.Callek) → review+
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?
Attachment #689774 - Flags: review?(bugspam.Callek) → review-
(Assignee)

Comment 16

5 years ago
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?
(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.
(Assignee)

Comment 18

5 years ago
Created attachment 691040 [details] [diff] [review]
buildbot-configs patch
Attachment #689774 - Attachment is obsolete: true
Attachment #691040 - Flags: review?(bugspam.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
Attachment #691040 - Flags: review?(bugspam.Callek)
Attachment #691040 - Flags: review+
Attachment #691040 - Flags: feedback?(edmorley.bz)
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).
Attachment #691040 - Flags: feedback?(edmorley.bz) → feedback+
(Assignee)

Updated

5 years ago
Attachment #689770 - Flags: checked-in+
(Assignee)

Updated

5 years ago
Attachment #691040 - Flags: checked-in+

Comment 21

5 years ago
This is in production.
in production
is this closeable now?
(Assignee)

Comment 24

5 years ago
Yes, just verified that this is working in tbpl.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
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'):
Never mind, that change is already there. I must've updated one repo and not the other or something.
Depends on: 821728

Updated

5 years ago
Depends on: 830868

Updated

5 years ago
Depends on: 830877
Product: mozilla.org → Release Engineering
You need to log in before you can comment on or make changes to this bug.