Closed
Bug 811723
Opened 12 years ago
Closed 12 years ago
change android reftests to run with --ignore-window-size for panda only
Categories
(Infrastructure & Operations Graveyard :: CIDuty, task)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: kmoir, Assigned: kmoir)
References
Details
Attachments
(2 files, 7 obsolete files)
(deleted),
patch
|
Callek
:
review+
kmoir
:
checked-in+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Callek
:
review+
emorley
:
feedback+
kmoir
:
checked-in+
|
Details | Diff | Splinter Review |
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•12 years ago
|
Assignee: nobody → kmoir
Assignee | ||
Comment 1•12 years ago
|
||
Assignee | ||
Comment 2•12 years ago
|
||
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•12 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•12 years ago
|
Attachment #686824 -
Flags: feedback?(bugspam.Callek)
Assignee | ||
Updated•12 years ago
|
Attachment #686829 -
Flags: feedback?(bugspam.Callek)
Comment 4•12 years ago
|
||
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 5•12 years ago
|
||
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+
Comment 6•12 years ago
|
||
(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•12 years ago
|
||
Attachment #686824 -
Attachment is obsolete: true
Attachment #687868 -
Flags: review?(bugspam.Callek)
Assignee | ||
Comment 8•12 years ago
|
||
The extra lines in the first patch were just a cut and paste error.
Attachment #686829 -
Attachment is obsolete: true
Assignee | ||
Updated•12 years ago
|
Attachment #687869 -
Flags: review?(bugspam.Callek)
Assignee | ||
Comment 9•12 years ago
|
||
Regarding comment 6, let me confirm with :jmaher that this is a panda-only requirement.
Comment 10•12 years ago
|
||
this is a must have for pandas, but we should do it for tegras as well!
Assignee | ||
Comment 11•12 years ago
|
||
Attachment #687868 -
Attachment is obsolete: true
Attachment #687868 -
Flags: review?(bugspam.Callek)
Assignee | ||
Comment 12•12 years ago
|
||
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•12 years ago
|
||
Attachment #688416 -
Attachment is obsolete: true
Attachment #689770 -
Flags: review?(bugspam.Callek)
Assignee | ||
Comment 14•12 years ago
|
||
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•12 years ago
|
Attachment #689770 -
Flags: review?(bugspam.Callek) → review+
Comment 15•12 years ago
|
||
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•12 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?
Comment 17•12 years ago
|
||
(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•12 years ago
|
||
Attachment #689774 -
Attachment is obsolete: true
Attachment #691040 -
Flags: review?(bugspam.Callek)
Comment 19•12 years ago
|
||
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 20•12 years ago
|
||
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•12 years ago
|
Attachment #689770 -
Flags: checked-in+
Assignee | ||
Updated•12 years ago
|
Attachment #691040 -
Flags: checked-in+
Comment 21•12 years ago
|
||
This is in production.
Comment 22•12 years ago
|
||
in production
Comment 23•12 years ago
|
||
is this closeable now?
Assignee | ||
Comment 24•12 years ago
|
||
Yes, just verified that this is working in tbpl.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment 25•12 years ago
|
||
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•12 years ago
|
||
Never mind, that change is already there. I must've updated one repo and not the other or something.
Updated•11 years ago
|
Product: mozilla.org → Release Engineering
Updated•6 years ago
|
Component: Platform Support → Buildduty
Product: Release Engineering → Infrastructure & Operations
Updated•5 years ago
|
Product: Infrastructure & Operations → Infrastructure & Operations Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•