Closed
Bug 589847
Opened 14 years ago
Closed 13 years ago
Either auto-generate valid_builders.py or expand try_parser to pull from config.py
Categories
(Release Engineering :: General, defect, P3)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: lsblakk, Assigned: lsblakk)
References
Details
(Whiteboard: [tryserver])
Attachments
(1 file, 3 obsolete files)
85.97 KB,
patch
|
catlee
:
review+
lsblakk
:
checked-in+
|
Details | Diff | Splinter Review |
In order to do the try specific build/test/talos selection, there is currently a flat file called valid_builders.py that give the lists of DESKTOP_PLATFORMS, MOBILE_PLATFORMS, TALOS_SUITES, TEST_SUITES, and PRETTY_NAMES in order to help try_parser.py validate and generate the custom builder set to be scheduled. To keep this from getting out of sync with try configs (and ultimately m-c configs) we need it to be auto-generated from config.py or to have try_parser.py import config.py and use that to validate/generate.
Comment 1•14 years ago
|
||
I think everything you need is in config.py. PLATFORMS, SUITES, and UNITTEST_SUITES have all the values.
Assignee | ||
Comment 2•14 years ago
|
||
Yes, and then also we need the PRETTY_NAME mapping from base_name for each platform
Assignee | ||
Updated•14 years ago
|
Assignee | ||
Comment 3•13 years ago
|
||
So this is working in my local staging setup for both the builders as well as the talos/test sendchanges. It will need more extensive testing in staging and a re-working of the try_parser unit test to makes sure that various use cases are tested before putting this in production but I wanted to show how I was doing it for feedback before going too far into that side of things.
Attachment #485939 -
Flags: feedback?(catlee)
Comment 4•13 years ago
|
||
Comment on attachment 485939 [details] [diff] [review] [WIP] removing valid_builders.py there are a bunch of places where we're generating the same pretty name. e.g. "%s build" % base_name appears a bunch of places. can you factor that out into a new variable? There's also a TODO left in there, are you still working on that piece?
Attachment #485939 -
Flags: feedback?(catlee) → feedback+
Assignee | ||
Comment 5•13 years ago
|
||
So I've run a lot of tests on this and right now there is just one little bug that I couldn't figure out - when I run it on the test master, sending in unittest values, a platform with more than one slave_platform (in my test case macosx64 which has two) I ended up with adding request for all the 10.5 unittests requested and then double the 10.6 builders. So that's what is happening on lines 47/48 because it was the only way I could find to get around this and get just one set of unittest builders for each slave_platform. I can't figure out where the second set is being generated.
Attachment #485939 -
Attachment is obsolete: true
Attachment #490814 -
Flags: review?(catlee)
Comment 6•13 years ago
|
||
Comment on attachment 490814 [details] [diff] [review] [very close] removes valid_builders.py from both try_parser and test_try_parser >diff --git a/misc.py b/misc.py >--- a/misc.py >+++ b/misc.py >@@ -484,40 +484,48 @@ def generateBranchObjects(config, name): > } > builders = [] > unittestBuilders = [] > triggeredUnittestBuilders = [] > nightlyBuilders = [] > xulrunnerNightlyBuilders = [] > debugBuilders = [] > weeklyBuilders = [] >+ # prettyNames is a mapping to pass to the try_parser for validation >+ PRETTY_NAME = '%s build' >+ prettyNames = {} >+ unittestPrettyNames = {} >+ unittestSuites = [] > # These dicts provides mapping between en-US dep and nightly scheduler names > # to l10n dep and l10n nightly scheduler names. It's filled out just below here. > l10nBuilders = {} > l10nNightlyBuilders = {} > pollInterval = config.get('pollInterval', 60) > l10nPollInterval = config.get('l10nPollInterval', 5*60) > # generate a list of builders, nightly builders (names must be different) > # for easy access > for platform in config['platforms'].keys(): > pf = config['platforms'][platform] > base_name = pf['base_name'] Can you put "pretty_name = PRETTY_NAME % base_name" here? It'll get rid of the same expansion 4 times below. > if platform.endswith("-debug"): >- debugBuilders.append('%s build' % base_name) >+ debugBuilders.append(PRETTY_NAME % base_name) >+ prettyNames[platform] = PRETTY_NAME % base_name > # Debug unittests > if pf.get('enable_unittests'): > test_builders = [] > base_name = config['platforms'][platform.replace("-debug", "")]['base_name'] > for suites_name, suites in config['unittest_suites']: >+ unittestPrettyNames[platform] = '%s debug test' % base_name > test_builders.extend(generateTestBuilderNames('%s debug test' % base_name, suites_name, suites)) > triggeredUnittestBuilders.append(('%s-%s-unittest' % (name, platform), test_builders, config.get('enable_merging', True))) > # Skip l10n, unit tests and nightlies for debug builds > continue > else: >- builders.append('%s build' % base_name) >+ builders.append(PRETTY_NAME % base_name) >+ prettyNames[platform] = PRETTY_NAME % base_name > > def generateTalosBranchObjects(branch, branch_config, PLATFORMS, SUITES, > ACTIVE_UNITTEST_PLATFORMS, factory_class=TalosFactory): > branchObjects = {'schedulers': [], 'builders': [], 'status': [], 'change_source': []} > branch_builders = {} > all_test_builders = {} > all_builders = [] >+ # prettyNames is a mapping to pass to the try_parser for validation >+ prettyNames = {} > > buildBranch = branch_config['build_branch'] > talosCmd = branch_config['talos_command'] > > for platform, platform_config in PLATFORMS.items(): > if platform_config.get('is_mobile', False): > branchName = branch_config['mobile_branch_name'] > tinderboxTree = branch_config['mobile_tinderbox_tree'] >@@ -1967,16 +1984,22 @@ def generateTalosBranchObjects(branch, b > > if tinderboxTree not in branch_builders: > branch_builders[tinderboxTree] = [] > if tinderboxTree not in all_test_builders: > all_test_builders[tinderboxTree] = [] > > for slave_platform in platform_config['slave_platforms']: > platform_name = platform_config[slave_platform]['name'] >+ # this is to handle how a platform has more than one slave platform >+ if prettyNames.has_key(platform): >+ prettyNames[platform] = [prettyNames[platform]] >+ prettyNames[platform].append(platform_name) >+ else: >+ prettyNames[platform] = platform_name So prettyNames[platform] can contain either a list of platform names or a single platform name? Can you change it so that it always contains a list, even if there's only one platform? >diff --git a/test/test_try_parser.py b/test/test_try_parser.py >--- a/test/test_try_parser.py >+++ b/test/test_try_parser.py >@@ -1,167 +1,150 @@ >-from buildbotcustom.try_parser import TryParser >+from ..try_parser import TryParser Why change to relative imports here? >- >-def getTestBuilders(platforms, testType, tests, builderNames, buildTypes): >+def getTestBuilders(platforms, testType, tests, builderNames, buildTypes, prettyNames, unittestPrettyNames): > testBuilders = [] > # for all possible suites, add in the builderNames for that platform > if tests != 'none': > if testType == "test": > for buildType in buildTypes: > for platform in platforms: >- for test in tests: >- # we only do opt unittests on Rev3 WINNT 6.1, the debug tests still run on pm02 >- if platform == 'win32': >- if buildType == 'debug': >- custom_builder = "%s tryserver %s %s %s" % (PRETTY_NAMES['desktop_win32'], buildType, testType, test) >- else: >- custom_builder = "%s tryserver %s %s %s" % (PRETTY_NAMES[platform][1], buildType, testType, test) >+ if platform in prettyNames.keys(): >+ if type(prettyNames[platform]) is list: As above, let's make sure that this always is a list, and we can simplify this block a bit. >+ for test in tests: >+ for slave_platform in prettyNames[platform]: >+ custom_builder = "%s tryserver %s %s %s" % (slave_platform, buildType, testType, test) >+ # note: can anyone else see why without the 'not in' the second slave platform is doubled? >+ if custom_builder in (builderNames) and custom_builder not in testBuilders: >+ testBuilders.extend([custom_builder]) > else: >- custom_builder = "%s tryserver %s %s %s" % (PRETTY_NAMES[platform], buildType, testType, test)
Attachment #490814 -
Flags: review?(catlee) → review-
Assignee | ||
Comment 7•13 years ago
|
||
* I originally tried "pretty_name = PRETTY_NAME % base_name" and got an error on base_name not existing yet. * prettyNames[platform] can contain either a list of platform names or a single platform name because in the parser I want to be able to find out if there is more than one slave_platform for a single platform by checking if it is a list type. so I was doing that on purpose. if I make every platform a list then I suppose I can check for length in the parser instead - is that a better way to do this? * i can remove the relative import, that was for local testing
Assignee | ||
Comment 8•13 years ago
|
||
(In reply to comment #7) > * I originally tried "pretty_name = PRETTY_NAME % base_name" and got an error > on base_name not existing yet. and I see now that is not what you were suggesting. carrying on.
Assignee | ||
Comment 9•13 years ago
|
||
addressed all comments from last review and tested on local builder master and tests master to make sure I'm still getting the builders I want.
Attachment #490814 -
Attachment is obsolete: true
Attachment #491259 -
Flags: review?(catlee)
Comment 10•13 years ago
|
||
Comment on attachment 491259 [details] [diff] [review] [tested] removing valid_builders.py from try_parser As per our 1x1, let's not lose the various test aliases we already have (e.g. 'mochitest-1' or 'mochitests')
Attachment #491259 -
Flags: review?(catlee)
Assignee | ||
Comment 11•13 years ago
|
||
Completely re-wrote the unittests for this and actually have useful tests that cover 100% of try_parser Also ran against local builder_master and tests_master to double-check.
Attachment #491259 -
Attachment is obsolete: true
Attachment #491967 -
Flags: review?(catlee)
Comment 12•13 years ago
|
||
Comment on attachment 491967 [details] [diff] [review] [tested] removing valid_builders.py from try_parser new and improved Looks good!
Attachment #491967 -
Flags: review?(catlee) → review+
Updated•13 years ago
|
Flags: needs-reconfig+
Assignee | ||
Comment 13•13 years ago
|
||
Comment on attachment 491967 [details] [diff] [review] [tested] removing valid_builders.py from try_parser new and improved http://hg.mozilla.org/build/buildbotcustom/rev/ba4d2c17b0b6
Attachment #491967 -
Flags: checked-in+
Assignee | ||
Updated•13 years ago
|
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Updated•11 years ago
|
Product: mozilla.org → Release Engineering
You need to log in
before you can comment on or make changes to this bug.
Description
•