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

Status: RESOLVED WONTFIX

(Release Engineering :: General, defect, P3)

Description

[Lukas Blakk [:lsblakk] use ?needinfo]

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?

Comment 1

[Armen [:armenzg]]

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?

Comment 2

[Armen [:armenzg]]

Attachment: refactor mozilla-tests/config.py a little

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.

Comment 3

[Lukas Blakk [:lsblakk] use ?needinfo]

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.

Comment 5

[Armen [:armenzg]]

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?

Comment 6

[Lukas Blakk [:lsblakk] use ?needinfo]

> 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 :)

Comment 7

[Armen [:armenzg]]

Attachment: refactor mozilla-tests/config.py

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.

Comment 8

[Armen [:armenzg]]

Attachment: adjusting for previous attachment

Comment 9

[Lukas Blakk [:lsblakk] use ?needinfo]

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

Comment 10

[Armen [:armenzg]]

Attachment: refactor mozilla-tests/config.py

As committed http://hg.mozilla.org/build/buildbot-configs/rev/66ceac0085da

Comment 11

[Armen [:armenzg]]

Comment on attachment 548475 [details] [diff] [review]
adjusting for previous attachment

http://hg.mozilla.org/build/buildbotcustom/rev/58ae380d5231

Comment 12

[[e-mail] (:bhearsum)]

This is in production now.

Comment 13

[Armen [:armenzg]]

Attachment: It makes the try branch to always match other branches plus some more refactoring

Comment 14

[Lukas Blakk [:lsblakk] use ?needinfo]

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.

Comment 15

[Armen [:armenzg]]

m-c is also a release branch even though we really don't release anything off it.
>     'mozilla-central':     { 'release_branch': True },

Comment 16

[Armen [:armenzg]]

FTR this is a no-op change.

Comment 17

[Lukas Blakk [:lsblakk] use ?needinfo]

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.

Comment 18

[Armen [:armenzg]]

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 19

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 556557 [details] [diff] [review]
It makes the try branch to always match other branches plus some more refactoring

very well then! :)

Comment 20

[Chris AtLee [:catlee]]

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)

Comment 21

[Armen [:armenzg]]

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

Comment 22

[Armen [:armenzg]]

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.