Closed Bug 631659 Opened 11 years ago Closed 10 years ago
Push Firebug Test Runner to m-c
100.37 KB, patch
|Details | Diff | Splinter Review|
100.82 KB, patch
|Details | Diff | Splinter Review|
295 bytes, patch
|Details | Diff | Splinter Review|
Push the Firebug-Test-Runner files to mozilla-central/testing
Assignee: nobody → ahalberstadt
Status: NEW → ASSIGNED
These files depend on mozrunner. Due to buildbot calling the python script directly it made more sense not to add the entire test runner package. https://github.com/ahal/Firebug-Test-Runner/tree/buildbotInt/runFBTests
Comment on attachment 510420 [details] [diff] [review] Firebug-Test-Runner files diff -r 165b354e3ec9 testing/firebug/fb_run.py --- /dev/null Thu Jan 01 00:00:00 1970 +0000 +++ b/testing/firebug/fb_run.py Mon Feb 07 15:24:00 2011 -0800 @@ -0,0 +1,206 @@ + +from mozrunner import FirefoxRunner +from mozprofile import FirefoxProfile These are possibly going to have to change when we move this to a virtualenv based system. Hopefully not, but there is a chance. We'll need to see how the makefile changes help us here. + +class FBRunner: + + def cleanup(self): + """ + Remove temporarily downloaded files + """ + try: + for tmpFile in ["firebug.xpi", "fbtest.xpi", "test-bot.config"]: + if os.path.exists(tmpFile): + os.remove(tmpFile) + except Exception as e: + print "[Warn] Could not clean up temporary files: " + str(e) Ideally, I'd like to have a mechanism to output these to a file as well as having them output to stdout + def get_extensions(self): + """ + Downloads the firebug and fbtest extensions + for the specified Firebug version + """ + FIREBUG_XPI = self.config.get("Firebug" + self.version, "FIREBUG_XPI") + FBTEST_XPI = self.config.get("Firebug" + self.version, "FBTEST_XPI") Doesn't this throw if you don't have a Firebug section in the INI? Let's ensure our ini file is properly formatted so that we can throw a useful error message in case that is not the case. + + def disable_compatibilityCheck(self): + """ + Disables compatibility check which could + potentially prompt the user for action + """ + try: + prefs = open(os.path.join(self.profile, "prefs.js"), "a") + prefs.write("user_pref(\"extensions.checkCompatibility.4.0b\", false);\n") + prefs.write("user_pref(\"extensions.checkCompatibility.4.0\", false);\n") + prefs.write("user_pref(\"extensions.checkCompatibility.3.6\", false);\n") This is going to break every time we increment versions. Can you check your binary's application.ini (firefox/application.ini) and get out the version you are running, then set your pref accordingly? + + def run(self): + """ + Code for running the tests + """ + + # Grab the extensions from server + try: + self.get_extensions() + except Exception as e: + self.cleanup() + print "[Error] Extensions could not be downloaded: " + str(e) + return I haven't followed it upstream yet, but this should definitely cause us to return a non zero error code from the entire python application. Please ensure that's the case. + + # Create environment variables + mozEnv = os.environ + mozEnv["XPC_DEBUG_WARN"] = "warn" # Suppresses certain alert warnings that may sometimes appear We probably also want to disable the crash reporter UI, see bug 616383 + + # Create profile for mozrunner and start the Firebug tests + print "[Info] Starting FBTests" Another thing about these print statements, when these are outputted to stdout when running under a buildbot framework, that stdout will end up in the log of the test. Therefore, these should be formatted the same way that the logs are formatted in our automation system |INFO|<msg> etc. + + # Find the log file + timeout, logfile = 0, 0 + # Wait up to 1 minute for the log file to be initialized + while not logfile and timeout < 60: + try: + for name in os.listdir(os.path.join(self.profile, "firebug/fbtest/logs")): + logfile = open(os.path.join(self.profile, "firebug/fbtest/logs/", name)) + except OSError: + timeout += 1 + sleep(1) + + # If log file was not found, create our own log file + if not logfile: + print "[Error] Could not find the log file in profile '" + self.profile + "'" + logfile = utils.create_log(self.profile, self.binary, self.testlist) + # If log file found, exit when fbtests finished (if no activity, wait up to 10 min) + else: + line, timeout = "", 0 + while timeout < 600: + line = logfile.readline() + if line == "": + sleep(1) + timeout += 1 + else: + print line.rstrip() + if line.find("Test Suite Finished") != -1: + break + timeout = 0 We can't have something in teh build automation waiting 10 minutes to figure out if it is hosed or not. This feels like you're working around something. What's the real issue here, and how can we fix that so that the log file is always generated, all the time. + +# Called from the command line +def cli(argv=sys.argv[1:]): + parser = OptionParser("usage: %prog [options]") + parser.add_option("-b", "--binary", dest="binary", + help="Firefox binary path") + + parser.add_option("-p", "--profile", dest="profile", + help="The profile to use when running Firefox") + + parser.add_option("-s", "--serverpath", dest="serverpath", + default="https://getfirebug.com/", + help="The http server containing the firebug tests") + + parser.add_option("-v", "--version", dest="version", + default="1.7", + help="The firebug version to run") + + parser.add_option("-t", "--testlist", dest="testlist", + help="Specify the name of the testlist to use, should usually use the default") + (opt, remainder) = parser.parse_args(argv) I'd prefer these standardize toward the names that our other test harnesses use. i.e. --appname for -b, --profile-path for -p. I think the others are specific to firebug and for those, I'm ok with. Just want to standardize where we can. diff -r 165b354e3ec9 testing/firebug/fb_update.py --- /dev/null Thu Jan 01 00:00:00 1970 +0000 +++ b/testing/firebug/fb_update.py Mon Feb 07 15:24:00 2011 -0800 @@ -0,0 +1,152 @@ +# This script will grab all of the latest firebug releases, as well as +# the fbtests and testlists and store them on the local webserver +from ConfigParser import ConfigParser +from time import sleep +import fb_utils as utils +import fileinput +import os, sys +import subprocess +import shutil +import optparse +import urllib2 + +def getRelativeURL(url): + if (url.find("http://") != -1): + index = url[7:].find("/") + 7 + elif (FIREBUG_XPI.find("https://") != -1): + index = url[8:].find("/") + 8 + else: + index = url.find("/") + return url[index+1:] Holy magic numbers, Batman. Um...we shoudln't depend on our URL string. Let's configure the webserver so that we don't have this issue. + + Um, let's deal with this code when we put the webserver together, let's not put this on m-c since it won't run on the slaves. diff -r 165b354e3ec9 testing/firebug/fb_utils.py --- /dev/null Thu Jan 01 00:00:00 1970 +0000 +++ b/testing/firebug/fb_utils.py Mon Feb 07 15:24:00 2011 -0800 @@ -0,0 +1,101 @@ +from ConfigParser import ConfigParser +import datetime +import os +import urllib2 +import platform + +# This is a quick hack that will rarely be used +# It is here so we don't have to depend on an entire RDF parsing module +def parse_rdf(lines, tagname): + """ + Parse a list of rdf formated text + and return the value of 'tagname' + """ + for line in lines: + if line.find("<em:" + tagname + ">") != -1: + return line[line.find(">") + 1:line.rfind("<")] + return -1 Sigh, I wish we didn't have to do this, but I don't know a better way. Firebug could break us here, but I think we can depend on them not to drastically change their install.rdf's Since we're not dealing with all addons in general I'm sorta ok with this. + Let's get these things done now, and we'll still have to change to accomodate for python 2.5 as well as other fun and exciting issues I'm sure we'll find as we test in staging. Thanks!
Thanks for the feedback. In regards to that last comment, the reason I felt sort of alright doing this is that it will hardly ever get called (in fact it's part of the same workaround where we wait for the logfile). The fbtest console generates the log file, but over the summer there was a version of fbtests that didn't start and thus didn't create a logfile. The test runner didn't know what was going on and there was absolutely no feedback about what happened. To get around this it waits for the logfile to be started (this can probably be changed to 5 seconds or less) and if none is found it synthesizes a logfile (getting some of its info from install.rdf) and then sends it to the database so we at least know what the changeset, versions, timestamp and platforms are. tl;dr - If we trust the firebug test console to be completely bug free such that it always generates a log file we can get rid of the rdf parsing and the wait for the log file altogether.
This patch uses python's logging module to report to stdout or a file (as specified by command line arg). Also reports crashes, gets rid of fb_utils and fb_update, and addresses the other concerns in Clint's review. It also no longer tries to synthesize a log file if none are generated.
See: https://github.com/ahal/Firebug-Test-Runner/commit/d156cff9caf007cefa9155b4a414b1b5b31987d4 for incremental diff over Patch 1
This patch is the first one that could be potentially checked in. It adds the mozrunner, mozprofile and mozprocess modules which firebug depends on. It also uses the virtualenv and simplejson modules that are in the mozmill testing component. The only change to the firebug code is to add an explicit mozRunner.stop() so we don't depend on garbage collection. The makefile is also slightly modified to fix an error that I only encountered in Windows.
(In reply to comment #6) > The only change to the firebug code is to add an explicit mozRunner.stop() so > we don't depend on garbage collection. The makefile is also slightly modified > to fix an error that I only encountered in Windows. Can you please provide more information about when the call should be made (I guess by FBTest harness) I'll fix implement it and provide new release of the FBTtest extension. Honza
> Can you please provide more information about when the call should be made (I > guess by FBTest harness) I'll fix implement it and provide new release of the > FBTtest extension. > > Honza Oops sorry, by firebug code I meant the test-runner code, or the code that isn't an external module like mozrunner (I'm getting lazy with my naming conventions). That was definitely my problem not yours and is already in that patch ;)
This patch uses a local config file to get the Firebug server and version args. Also refactor of exception handling. In case of the diff being too painful to look at, this patch is aligned with this changeset: https://github.com/ahal/Firebug-Test-Runner/tree/1db688c7ab9a0191aea6eb1a5b841ed3330334ff
Comment on attachment 513313 [details] [diff] [review] Patch 2.2 - Read firebug server and version from local config file >diff --git a/testing/firebug/Makefile.in b/testing/firebug/Makefile.in >new file mode 100644 >--- /dev/null >+++ b/testing/firebug/Makefile.in >@@ -0,0 +1,86 @@ >+# ***** BEGIN LICENSE BLOCK ***** >+# Version: MPL 1.1/GPL 2.0/LGPL 2.1 >+# >+# The contents of this file are subject to the Mozilla Public License Version >+# 1.1 (the "License"); you may not use this file except in compliance with >+# the License. You may obtain a copy of the License at >+# http://www.mozilla.org/MPL/ >+# >+# Software distributed under the License is distributed on an "AS IS" basis, >+# WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License >+# for the specific language governing rights and limitations under the >+# License. >+# >+# The Original Code is mozilla.org code. >+# >+# The Initial Developer of the Original Code is >+# Mozilla.org. >+# Portions created by the Initial Developer are Copyright (C) 2010 >+# the Initial Developer. All Rights Reserved. >+# >+# Contributor(s): >+# Andrew Halberstadt <firstname.lastname@example.org> (Original author) no tabs >+DEPTH = ../.. >+topsrcdir = @top_srcdir@ >+srcdir = @srcdir@ >+VPATH = @srcdir@ >+mozmilldir = @top_srcdir@/testing/mozmill These tab characters are unncessary >+ >+include $(DEPTH)/config/autoconf.mk >+ >+MODULE = testing_firebug Here too >diff --git a/testing/firebug/fb-test-runner.config b/testing/firebug/fb-test-runner.config >new file mode 100644 >--- /dev/null >+++ b/testing/firebug/fb-test-runner.config >@@ -0,0 +1,11 @@ >+# This section contains arguments to be read by the firebug test runner >+[runner_args] >+server = http://10.250.5.0 >+ >+# This section maps firefox versions to firebug versions. >+# The default value is used for any firefox version not specified here. >+[version_map] >+3.6 = 1.6 >+4.0 = 1.7 >+4.0b = 1.7 >+default = 1.7 Include an empty section for disabling tests. This way it will be obvious to people where to disable a specific firebug test. Perhaps something like: [disabled_tests] testfoo.js = 4.0 # Disable testfoo on the 4.0 test run, for instance. I don't really care about the format, but we need some indicator here so people will know how to use this feature. >diff --git a/testing/firebug/fb_run.py b/testing/firebug/fb_run.py >new file mode 100644 >--- /dev/null >+++ b/testing/firebug/fb_run.py >+# Contributor(s): >+# Andrew Halberstadt - email@example.com Andrew Halberstadt <firstname.lastname@example.org> >+ # If there was a timeout, then there was most likely a crash (however could also be failure in FBTest console or test itself) >+ if timeout >= self.TEST_TIMEOUT: >+ logfile.seek(1) >+ line = logfile.readlines()[-1] >+ if line.find("FIREBUG INFO") != -1: >+ line = line[line.find("|") + 1:].lstrip() # Extract the test name from log line >+ line = line[:line.find("|")].rstrip() >+ else: >+ line = "Unknown Test" >+ print "FIREBUG TEST-UNEXPECTED-FAIL | " + line + " | Possible Firefox crash detected" # Print out crash message with offending test >+ self.log.warn("Possible crash detected - test run aborted") We should file a follow on bug to integrate minidump stackwalker into here so we can output a stack from the crash. >+# Called from the command line >+def cli(argv=sys.argv[1:]): >+ try: >+ runner = FBRunner(binary=opt.binary, profile=opt.profile, serverpath=opt.serverpath, >+ version=opt.version, testlist=opt.testlist, log=opt.log, debug=opt.debug) >+ runner.run() >+ except Exception: >+ return -1 >+ >+if __name__ == '__main__': >+ sys.exit(cli()) Stray Tab ^ Also, since cli is effectively your "main" function, why not call it "main"? >diff --git a/testing/firebug/installfirebug.py b/testing/firebug/installfirebug.py >new file mode 100644 >--- /dev/null >+++ b/testing/firebug/installfirebug.py The code in installfirebug.py looks good, but if this is a new file for m-c we'd like to try to standardize on 4 space for all new python going into the tree. I hate to ask you but would you mind making this 4 space so we can (slowly) get rid of our old 2 space python in the m-c test harnesses? This looks really good, Andrew. Just a few nits. And also, file another follow on bug to remove all this mozmill stuff once we check in the mozmill 2.0 codebase. Once that codebase is in m-c, this code will all be duplicated.
Attachment #513313 - Flags: review?(ctalbert) → review+
Comment on attachment 515216 [details] [diff] [review] Patch 2.4 - Forgot to add config file to Makefile (inc) r+
Attachment #515216 - Flags: review+
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
And http://hg.mozilla.org/mozilla-central/rev/79d8465de149 to (more or less) trigger a clobber.
Comment on attachment 513344 [details] [diff] [review] Patch 2.3 - Nits addressed >+# The Initial Developer of the Original Code is >+# Mozilla.org. That's not true. This should be "the Mozilla Foundation". Please fix. http://weblogs.mozillazine.org/gerv/archives/2010/02/mpl_initial_developer_for_mozilla_employ.html
You'll also wanna do this: diff --git a/toolkit/toolkit-tiers.mk b/toolkit/toolkit-tiers.mk --- a/toolkit/toolkit-tiers.mk +++ b/toolkit/toolkit-tiers.mk @@ -293,11 +293,12 @@ endif ifdef MOZ_MAPINFO tier_platform_dirs += tools/codesighs endif ifdef ENABLE_TESTS tier_platform_dirs += testing/mochitest tier_platform_dirs += testing/xpcshell tier_platform_dirs += testing/mozmill +tier_platform_dirs += testing/firebug tier_platform_dirs += testing/tools/screenshot endif
You need to log in before you can comment on or make changes to this bug.