Closed
Bug 695888
Opened 13 years ago
Closed 13 years ago
test-masters should test that universal masters still work
Categories
(Release Engineering :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: jhford, Assigned: jhford)
References
Details
(Whiteboard: [buildbotcustom-testing])
Attachments
(4 files)
12.68 KB,
text/plain
|
Details | |
6.29 KB,
text/plain
|
Details | |
8.74 KB,
patch
|
catlee
:
review+
|
Details | Diff | Splinter Review |
2.31 KB,
patch
|
Details | Diff | Splinter Review |
I use universal masters to do my testing. There is also a check that is disabled for multi-master mode, which is that every builder name in a scheduler refers to a real builder. Turns out that this check is really useful. I am not sure how multi master works 100%, but my guess is that the scheduler is creating builds for this nonexistant builder which never end up running. Using the universal master.cfg with checkconfig does pick up this issue. We should make sure that all of our masters are able to checkconfig when using a universal master.cfg. Whether we do this by adding the master to production_masters.json, create a second json file or hardcode two universal masters is not important to me. Example is the following changesets. (testing)~/mozilla/testing $ hg id -R buildbotcustom/ 7cb892e55990 (testing)~/mozilla/testing $ hg id -R buildbot-configs/ e599bb8e99e8 tip With a multi-master setup, nothing errors out. With a universal master.cfg file, I get: AssertionError: <buildbotcustom.scheduler.SpecificNightly-props instance at 0x105175200> uses unknown builder Android Debug mozilla-central nightly make: *** [checkconfig-build] Error 1 When I go back to the multi master.cfg, I can verify that the builder isn't actually created (testing)~/mozilla/testing $ curl -i http://localhost:8501/builders/Android%20Debug%20mozilla-central%20nightly HTTP/1.1 404 Not Found Date: Wed, 19 Oct 2011 22:41:02 GMT Content-Length: 173 Content-Type: text/html Server: TwistedWeb/8.2.0 <html> <head><title>404 - No Such Resource</title></head> <body><h1>No Such Resource</h1> <p>No such child resource.</p> </body></html>
Assignee | ||
Updated•13 years ago
|
Severity: normal → major
Whiteboard: [buildbotcustom-testing]
Assignee | ||
Comment 1•13 years ago
|
||
to be clear, the priority for the reasons to fix this issue are, in order: 1) do a check that we can't do without testing universal masters 2) make development and testing easier
Assignee | ||
Comment 2•13 years ago
|
||
I have a patch coming up shortly
Assignee | ||
Comment 3•13 years ago
|
||
As you can see, there are two deprecation warnings as well as *no* failure, even though there is actually failure due to bug 690860.
Assignee: nobody → jhford
Assignee | ||
Comment 4•13 years ago
|
||
(should have mentioned, that test-masters.sh log I attached was run with my patch) Note: 1) no deprecation warnings, easier to read 2) test status (TEST-PASS, TEST-FAIL, TEST-INFO) lines printed at useful points during the test 3) There is a summary at the end of the test with the number of passing and failing tests 4) if a master fails, the test goes on. With the long time it takes to test the code, this is very useful to see narrow problems down, imo 5) last, but most important, it catches fallout from bug 690860 which is not caught by test-masters.sh Not present in the log is the ability to limit by role. This is only available for json testing. This is useful you are only working on one master type. It wouldn't be hard to add a -h/--host flag to test a specific master. (testing)~/mozilla/testing/buildbot-configs $ ./setup-master.py -l -R try bm04-try1 bm06-try1 bm09-try1 bm14-try1 pm02-try try-mtv
Assignee | ||
Comment 5•13 years ago
|
||
This patch adds the ability to run tests to setup-masters. I have attached a lot of the output from this patch. I have also run test-masters.sh against this patch so I think I haven't broken the commonly used interfaces. Extra verification would be great though! It might seem odd to do the universal master logic in load_master_json. This is the only safe place to do it because load_master_json is the expert on what files go in each master. Changing this would be a significant change to the program. This works with the -8 flag, and I would assume the -7 flag. I also modified the __name__=='__main__' flow a bit to allow for more than one 'command'. If I wasn't concerned with the interface used by other programs, I would have done things a little differently, like have explicit commands like: ./setup-masters.py list ./setup-masters.py test ./setup-masters.py create master-name master-dir Ideas for further improvement: -use python logging module -write unit tests for setup-master.py -rename setup-master.py to setup_master.py so other python modules can import it -save logs from creating the master -automatically checkconfig a master when its created
Attachment #568298 -
Flags: review?(catlee)
Assignee | ||
Comment 6•13 years ago
|
||
This patch converts test-masters.sh into a shortcut to launch ./setup-master.py with the familiar test-masters.sh syntax. It also adds the TEMP environment variable to the test routine in setup-master.py
Attachment #568301 -
Flags: feedback?
Comment 7•13 years ago
|
||
Comment on attachment 568298 [details] [diff] [review] buildbot-configs v1 Review of attachment 568298 [details] [diff] [review]: ----------------------------------------------------------------- ::: setup-master.py @@ +87,5 @@ > + log = open(test_log_filename) > + log_size = os.path.getsize(test_log_filename) > + # We expect that the reconfig done message is before the last K of output > + if log_size > 1024: > + log.seek(log_size - 1024) Is this really necessary? @@ +100,5 @@ > + print "TEST-FAIL checkconfig returned 0 for %s but didn't print 'Config file is good!'" % \ > + self.name > + else: > + print "TEST-FAIL %s failed to run checkconfig" % self.name > + #print "TEST-INFO log: %s, remnants of test: %s" % (test_log_filename, test_dir) left over debugging code? remove it or uncomment it.
Attachment #568298 -
Flags: review?(catlee) → review+
Assignee | ||
Comment 8•13 years ago
|
||
(In reply to Chris AtLee [:catlee] from comment #7) > Comment on attachment 568298 [details] [diff] [review] [diff] [details] [review] > buildbot-configs v1 > > Review of attachment 568298 [details] [diff] [review] [diff] [details] [review]: > ----------------------------------------------------------------- > > ::: setup-master.py > @@ +87,5 @@ > > + log = open(test_log_filename) > > + log_size = os.path.getsize(test_log_filename) > > + # We expect that the reconfig done message is before the last K of output > > + if log_size > 1024: > > + log.seek(log_size - 1024) > > Is this really necessary? No, but I think its a safe and good optimization. It allows us to avoid iterating over the entire log, which could be quite long, especially if there are local debugging print statements. > @@ +100,5 @@ > > + print "TEST-FAIL checkconfig returned 0 for %s but didn't print 'Config file is good!'" % \ > > + self.name > > + else: > > + print "TEST-FAIL %s failed to run checkconfig" % self.name > > + #print "TEST-INFO log: %s, remnants of test: %s" % (test_log_filename, test_dir) > > left over debugging code? remove it or uncomment it. Removed. This was from before I had the test summary at the end. http://hg.mozilla.org/build/buildbot-configs/rev/d087a4d9c2a8
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Updated•11 years ago
|
Product: mozilla.org → Release Engineering
Assignee | ||
Updated•10 years ago
|
Attachment #568301 -
Flags: feedback?
You need to log in
before you can comment on or make changes to this bug.
Description
•