631659 - Push Firebug Test Runner to m-c

Status: RESOLVED FIXED

(Testing :: General, defect)

Description

[Andrew Halberstadt [:ahal]]

Push the Firebug-Test-Runner files to mozilla-central/testing

Comment 1

[Andrew Halberstadt [:ahal]]

Attachment: Firebug-Test-Runner files

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 2

[cmtalbert]

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!

Comment 3

[Andrew Halberstadt [:ahal]]

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.

Comment 4

[Andrew Halberstadt [:ahal]]

Attachment: Patch 2.0 - Use python logging, address stability concerns, report crashes +more

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.

Comment 5

[Andrew Halberstadt [:ahal]]

See: https://github.com/ahal/Firebug-Test-Runner/commit/d156cff9caf007cefa9155b4a414b1b5b31987d4

for incremental diff over Patch 1

Comment 6

[Andrew Halberstadt [:ahal]]

Attachment: Patch 2.1 - Add mozrunner/mozprocess/mozprofile dependencies

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.

Comment 7

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(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

Comment 8

[Andrew Halberstadt [:ahal]]

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

Comment 9

[Andrew Halberstadt [:ahal]]

Attachment: Patch 2.2 - Read firebug server and version from local config file

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 10

[cmtalbert]

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 <[e-mail]>		(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 - [e-mail]
Andrew Halberstadt <[e-mail]>

>+        # 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.

Comment 11

[Andrew Halberstadt [:ahal]]

Attachment: Patch 2.3 - Nits addressed

Diff: https://github.com/ahal/Firebug-Test-Runner/commit/078231299be966c2c2f354493448b6337fb88281

Comment 12

[Andrew Halberstadt [:ahal]]

Attachment: Patch 2.4 - Forgot to add config file to Makefile (inc)

Comment 13

[cmtalbert]

Comment on attachment 515216 [details] [diff] [review]
Patch 2.4 - Forgot to add config file to Makefile (inc)

r+

Comment 14

[Joel Maher ( :jmaher ) (UTC -8)]

http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=1df2e6e48609
http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=aca8013c8cff

Comment 15

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

And http://hg.mozilla.org/mozilla-central/rev/79d8465de149 to (more or less) trigger a clobber.

Comment 16

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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

Comment 17

[jag (Peter Annema)]

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