Closed Bug 633054 Opened 10 years ago Closed 9 years ago

audit the mozilla/ mozilla-tests/ configs have a lot of duplicate code, is it possible get them into the same config?

Categories

(Release Engineering :: General, defect, P3)

x86
All
defect

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: lsblakk, Assigned: armenzg)

References

Details

Attachments

(3 files, 2 obsolete files)

Try to get them into one config file if possible or at least pull out some of the duplication - tinderbox_tree for example and also do we still need packaged_unittest_tinderbox_tree?
Feel free to pass reviews my way.
I had few patches months ago but I would have bit-rotten bear and aki's work.
A lot can be moved there but we will need catlee's review as he touched a lot of the code under mozilla/.

catlee what you say?
Assignee: lsblakk → armenzg
Status: NEW → ASSIGNED
Priority: P3 → P4
I started working on bug 660124 and chrome_mac started getting on the way for project branches so I started refactoring... I think I got a little carried away.

This is a no-op patch which I have run through test-masters.sh any trouble but I would like to run it through dump_master.sh.
Attachment #547089 - Flags: review?(catlee)
Comment on attachment 547089 [details] [diff] [review]
refactor mozilla-tests/config.py a little

Just to note, this is not noop since you're enabling the jetpack tests for mozilla-beta and mozilla-release and those are not currently on, though they are requested in bug 669428 so if you land this please feel free to go close that bug.
Duplicate of this bug: 602525
Comment on attachment 547089 [details] [diff] [review]
refactor mozilla-tests/config.py a little

lsblakk is right. I tested incorrectly. I will attach the correct patch soon.

lsblakk I would prefer to keep this patch as no-op and do a follow up for jetpack when we are ready; is that alright?
Attachment #547089 - Flags: review?(catlee)
> lsblakk I would prefer to keep this patch as no-op and do a follow up for
> jetpack when we are ready; is that alright?

fine by me, I can still wait for this to land since it will make enabling the other branches easier and cleaner :)
Attached patch refactor mozilla-tests/config.py (obsolete) — Splinter Review
After several iterations on dump_masters.sh and test-masters.sh, here is the no-op change for mozilla-tests/config.py (except for adding jetpack for Win7 x64).

It might be bitrotten by bug 674228 but I will adjust it again once I meet your review requirements.
Attachment #547089 - Attachment is obsolete: true
Attachment #548471 - Flags: review?(lsblakk)
Attachment #548475 - Flags: review?(lsblakk)
Comment on attachment 548471 [details] [diff] [review]
refactor mozilla-tests/config.py

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

::: mozilla-tests/config.py
@@ +857,5 @@
> +    for pf in PLATFORMS:
> +        if pf in ('linux-android', ):
> +            continue
> +        for slave_pf in PLATFORMS[pf]['slave_platforms']:
> +            # yucky exceptions for 10.5

change comment to 
# to get around leopard-o having only debug key and leopard having only opt

or something more useful :) 

r+ with that nit addressed
Attachment #548471 - Flags: review?(lsblakk) → review+
Attachment #548475 - Flags: review?(lsblakk) → review+
As committed http://hg.mozilla.org/build/buildbot-configs/rev/66ceac0085da
Attachment #548471 - Attachment is obsolete: true
Attachment #548753 - Flags: review+
Attachment #548753 - Flags: checked-in+
Comment on attachment 548475 [details] [diff] [review]
adjusting for previous attachment

http://hg.mozilla.org/build/buildbotcustom/rev/58ae380d5231
Attachment #548475 - Flags: checked-in+
This is in production now.
Comment on attachment 556557 [details] [diff] [review]
It makes the try branch to always match other branches plus some more refactoring


> BRANCHES['mozilla-central']['build_branch'] = "1.9.2"
>-BRANCHES['mozilla-central']['platforms']['linux']['enable_mobile_unittests'] = True
> BRANCHES['mozilla-central']['platforms']['linux']['fedora']['opt_unittest_suites'] += [('reftest-no-accel', ['opengl-no-accel'])]
> BRANCHES['mozilla-central']['platforms']['linux-android']['enable_debug_unittests'] = True

Did you mean to turn off the mobile unittests here? I see you have them in your release_branch loop for the others, but it looks like you're just turning them off for m-c.
m-c is also a release branch even though we really don't release anything off it.
>     'mozilla-central':     { 'release_branch': True },
FTR this is a no-op change.
Comment on attachment 556557 [details] [diff] [review]
It makes the try branch to always match other branches plus some more refactoring

in that case you will end up with the wrong repo path for mozilla-central:


+        BRANCHES[branch]['repo_path'] = "releases/%s" % branch

mozilla-central is a top-level repo.
I deal with it in the exceptions section:

> # The following are exceptions to the defaults
>
> ######## mozilla-central
> BRANCHES['mozilla-central']['branch_name'] = "Firefox"
> BRANCHES['mozilla-central']['repo_path'] = "mozilla-central"
Comment on attachment 556557 [details] [diff] [review]
It makes the try branch to always match other branches plus some more refactoring

very well then! :)
Attachment #556557 - Flags: review?(lsblakk) → review+
Comment on attachment 556557 [details] [diff] [review]
It makes the try branch to always match other branches plus some more refactoring

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

What does a diff of dump master output say?

r+ with comments addressed, and a clean diff of dump master.

::: mozilla-tests/config.py
@@ +375,5 @@
>      BRANCHES[branch]['support_url_base'] = branchConfig.get('support_url_base', 'http://build.mozilla.org/talos')
>      BRANCHES[branch]['enable_unittests'] = branchConfig.get('enable_unittests', True)
>  
>  def loadCustomTalosSuites(BRANCHES, SUITES, branch, branchConfig):
> +    coallesceJobs = BRANCHES.get('coallesce_jobs', True)

I think you mean branchConfig.get('coallesce_jobs', True)?

@@ +393,5 @@
>  
>      for suite in SUITES.keys():
>          if not SUITES[suite]['enable_by_default']:
>              # Suites that are turned off by default
> +            BRANCHES[branch][suite + '_tests'] = (talosConfig.get(suite, 0), ) + (coallesceJobs, ) + SUITES[suite]['options']

This could be better written as 

BRANCHES[branch][suite + '_tests'] = (talosConfig.get(suite, 0), coallesceJobs) + SUITES[suite]['options']

@@ +403,5 @@
>      '''
>      This is very similar to loadCustomTalosSuites and is to deal with branches that are not in project_branches.py
>      but in config.py. Both functions could be unified later on when we do further refactoring.
>      '''
> +    coallesceJobs = BRANCHES.get('coallesce_jobs', True)

Probably should be BRANCHES[branch].get('callesce_jobs', True)
Attachment #556557 - Flags: review?(catlee) → review+
Comment on attachment 556557 [details] [diff] [review]
It makes the try branch to always match other branches plus some more refactoring

I addresses the comments. dump_masters.sh's diff returned no differences.

http://hg.mozilla.org/build/buildbot-configs/rev/cd4361138ffb

./test-masters.sh && ~/repos/releng/braindump/buildbot-related/dump_masters.sh > new && hg up -C &&  ~/repos/releng/braindump/buildbot-related/dump_masters.sh > old && diff -pU 8 old new > armen

armenzg-laptop $ ls -l old new
-rw-r--r--  1 armenzg  staff  614477680 31 Aug 15:01 new
-rw-r--r--  1 armenzg  staff  614477680 31 Aug 15:12 old
armenzg-laptop $ md5sum new
5ce56a3fc1cdcf6e64a383e7b7d04d6a  new
armenzg-laptop $ md5sum old
5ce56a3fc1cdcf6e64a383e7b7d04d6a  old
Attachment #556557 - Flags: checked-in+
Assignee: armenzg → nobody
Assignee: nobody → armenzg
Priority: P4 → P3
Depends on: 720782
I have not been able to work on this.
I don't see us gaining value since we will eventually be switching to Large-Wooden-Rabbit.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → WONTFIX
Product: mozilla.org → Release Engineering
You need to log in before you can comment on or make changes to this bug.