562822 - Collect necessary Mozmill result data for all kind of Mozmill tests

Status: VERIFIED FIXED

(Testing Graveyard :: Mozmill, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

To be able to create the wanted dashboard with Mozmill test results, we have to make sure that Mozmill itself collects all the necessary information for the following type of test-runs:

* Normal tests
* Restart tests
* Software Update tests
* L10n tests
* Accessibility tests
* Add-on tests

I will come up with a list mid or end of next week and would be happy if others could check if everything is listed for their own test-runs. Especially this will be l10n and accessibility tests.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Sorry for the delay. Today I was working on a list of interesting data we will have to send to brasstacks. Please check the following spreadsheet if you are fine with the list of items or if you would need more information. We are planning to update Mozmill ASAP to use the new structure in the dev builds.

https://spreadsheets.google.com/ccc?key=trPutwwkRQzWCrBtKMxNpUg&hl=en#

Areas I would like to get feedback:

accessibility - Marco (we should have a call since you can't access it)

addons  - I will ask around

l10 - Axel and Adrian

updates - Al, Anthony

Thanks

Comment 2

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Looks pretty good to me.  We discussed some of this yesterday in Skype, but echoing here for commentary:

Tests should have some unique way to identify themselves, for run-over-run correlation and other reporting purposes.  

Path, as mentioned in the spreadsheet, could work, though it should be relative to the project directory (i.e. either firefox/restartTests/foo or restartTests/foo, depending how we want to track).

I think there's a good argument for defining an independent test "namespace" and making that example something like Firefox::RestartTests::Foo though, with a policy that pathing follows namespacing.  My thoughts on that are A) we might eventually move these tests into a cloud storage where volume-based paths don't make as much sense and B) even relative paths have a system-specific separator (/ vs. \) that will be inconsistent between platforms.

Implementation-wise, that would basically look like the test function calling an "init" function first thing that takes the test ID, and then ultimately uses
it for the rest of the reporting.

My other thought was that we should be prepared for the idea of a test with multiple screenshots, comments, or result lines.  Ultimately, I find functional test frameworks tend to differ from straight unit testing frameworks in that one test has multiple verifications/assertions, and it's useful to have a mechanism where some are non-fatal if they're independent from the others. That results in a master/detail structure where test function is master, and each assertion/comment/screenshot/whathaveyou is the detail.

I'm not proposing that we set that up now--the results should match our current flow, which is one result per test function.  But I'd like to keep us open for refactoring to that in the future if the benefit becomes obvious.

Comment 3

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

One other thing, re: path, if we use path as the unique ID I think we should leave off the .js part.  I realize it's extremely unlikely the tests will be reimplemented not using JavaScript, but anything that goes into the ID effectively needs to be true forever, or we'll have a discontinuity in reporting.  Even the ever-present outside chance makes me think we shouldn't couple test IDs to the JavaScript filetype.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #2)
> Path, as mentioned in the spreadsheet, could work, though it should be relative
> to the project directory (i.e. either firefox/restartTests/foo or
> restartTests/foo, depending how we want to track).

Yes, we will have to strip out the values above the test folder by using the test path. The only concern I have here is, how can we detect if an user is running a sub-folder like firefox/testGeneral. In that case our path will not match the proposed id and results will be handled differently. That gives me at least the impression that we should only allow test-runs against the root folders for each type of test-run, i.e. firefox, firefox/restartTests, and others. 

> I think there's a good argument for defining an independent test "namespace"
> and making that example something like Firefox::RestartTests::Foo though, with

We don't need the application name in the namespace. It's already given in another field and that makes it easier for us to run map functions. Same for the test type. Using values like "Firefox::restartTests" would force us to run a regex against it.

> make as much sense and B) even relative paths have a system-specific separator
> (/ vs. \) that will be inconsistent between platforms.

Good call! That's definitely a problem, which I forgot in my proposal. We definitely have to replace any '\' with a '/' to make sure we can correlate results for different platforms. That has to be implemented in Mozmill itself.

> Implementation-wise, that would basically look like the test function calling
> an "init" function first thing that takes the test ID, and then ultimately uses
> it for the rest of the reporting.

IMO it will be easier to just maintain the test path and test function name. Adding another id to setupModule doesn't let us identify different test functions in the same module.

"firefox/testPreferences/testPaneRetention", "testOptionsDialogRetention"

> My other thought was that we should be prepared for the idea of a test with
> multiple screenshots, comments, or result lines.  Ultimately, I find functional
> test frameworks tend to differ from straight unit testing frameworks in that
> one test has multiple verifications/assertions, and it's useful to have a
> mechanism where some are non-fatal if they're independent from the others. That
> results in a master/detail structure where test function is master, and each
> assertion/comment/screenshot/whathaveyou is the detail.

We should move over any discussion on that topic to bug 533857.

(In reply to comment #3)
> One other thing, re: path, if we use path as the unique ID I think we should
> leave off the .js part.  I realize it's extremely unlikely the tests will be
> reimplemented not using JavaScript, but anything that goes into the ID
> effectively needs to be true forever, or we'll have a discontinuity in
> reporting.  Even the ever-present outside chance makes me think we shouldn't
> couple test IDs to the JavaScript filetype.

I don't think that is that much important. I would just leave ".js" in place. It's easier to read and people will directly see that it is a function name. Once we reimplement tests or move them around they will always get a new path and filename and will appear as new in the results. IMO that is the right way to go.

Clint, Heather, and Jeff, can you three please also check and comment back? Thanks.

Comment 5

[Axel Hecht [:Pike]]

FWIW, I don't see anything missing from what I have in my head so far.

Comment 6

[Jeff Hammel]

I like
path/to/testModule.js:testName
but don't really care how the fields are separated, nor have I studied the problem in depth or am that familiar with how the other harness do things.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #6)
> I like
> path/to/testModule.js:testName
> but don't really care how the fields are separated, nor have I studied the
> problem in depth or am that familiar with how the other harness do things.

Well, storing them separate gives us more flexibility. We can always create map/reduce functions which combine the test module and the test function.

Thanks Axel! Looks like everyone agrees to the proposal. I will work on getting this integrated later this month or beginning of July.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Something we also wanna have added/changed:

* The starttime and endtime shouldn't have milliseconds included but should have UTC time with the timezone included
* The id of the applications branch (shouldn't be determined again and again in the queries for couchdb)
* A link to the hg changeset of the build can be really useful for checking regression ranges

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Patch v1 (Mozmill)

Alright, after more than 1 hour in trying to get merge conflicts resolved, I have the first patch ready. It implements all the given data from the spreadsheet.

Comment 10

[cmtalbert]

Comment on attachment 460715 [details] [diff] [review]
Patch v1 (Mozmill)

>diff --git a/mozmill/mozmill/__init__.py b/mozmill/mozmill/__init__.py
>+class LocalTimezone(tzinfo):
>+    """ Class to automatically determine the timezone offset. """
>+
>+    def __init__(self):
>+        self.stdOffset = timedelta(seconds = -time.timezone)
>+        if time.daylight:
>+            self.dstOffset = timedelta(seconds = -time.altzone)
>+        else:
>+            self.dstOffset = self.stdOffset
>+        self.dstDiff = self.dstOffset - self.stdOffset
>+
>+    def utcoffset(self, dt):
>+        if self._isdst(dt):
>+            return self.dstOffset
>+        else:
>+            return self.stdOffset
>+
>+    def dst(self, dt):
>+        if self._isdst(dt):
>+            return self.dstDiff
>+        else:
>+            return timedelta(0)
>+
>+    def tzname(self, dt):
>+        return time.tzname[self._isdst(dt)]
>+
>+    def _isdst(self, dt):
>+        tt = (dt.year, dt.month, dt.day,
>+              dt.hour, dt.minute, dt.second,
>+              dt.weekday(), 0, -1)
>+        stamp = time.mktime(tt)
>+        tt = time.localtime(stamp)
>+        return tt.tm_isdst > 0
>+
I really don't want this change in Mozmill.  Putting timezone code into Mozmill just opens up an entire pandora's box of bugs that I'd rather not deal with.  Furthermore, I think (with only a quick glance at this code) you're going to have a problem with negative offset timezones.  But regardless, I really am opposed to this change.

There are a lot of problems uploading data in a local computer timezone.  First, that's never going to be reliable.  Second, what happens when a timezone doesn't match?  Microsoft doesn't define timezones the way linux does, what happens when timezones change?  Are you going to retool all the uploaded timezone results on the server?  Furthermore, it's never going to be what someone looking at the time information on the server is going to expect (they expect to see those times in THEIR timezone, which will never be the case).  It's much preferable to upload using UTC timezone and then convert to viewer's timezone when displaying on the server side.  That way the viewer always sees the timezone in their data.  Also that way, it is very easy to compare runs of data when looking at which one happened first, which one happened second etc.  And lastly, converting from utc to a timezone is straightforward. Converting from a local timezone to another local timezone is not at all straightforward.  I can go into a lot more detail if you want, but we should really really stay away from this.

Mozmill should upload time information using UTC, and any dashboards built from that data then have a much easier time displaying something understandable for the viewer.  

And also there's the age-old case of coming out of summer time. If a test starts at 1:59AM on a day when daylight savings is about to end, then it could actually end before it starts.  How are you going to deal with that?

Why do you feel uploading results in local time is necessary inside of Mozmill?

>@@ -164,6 +202,7 @@ class MozMill(object):
>         #self.zombieDetector = ZombieDetector(self.stop)
> 
>         # test time
>+        self.localTimezone = LocalTimezone()
No, I really don't like this.
>         self.starttime = self.endtime = None
> 
>         # setup event listeners
>@@ -244,7 +283,7 @@ class MozMill(object):
> 
>         # set the starttime for the tests
>         # XXX assumes run_tests will be called soon after (currently true)
>-        self.starttime = datetime.utcnow().isoformat()        
>+        self.starttime = datetime.now(self.localTimezone)
see above
> 
>     def run_tests(self, test, sleeptime=4):
>         """
>@@ -304,53 +343,102 @@ class MozMill(object):
>         print "INFO Skipped: %d" % len(self.skipped)
> 
>     def get_appinfo(self, bridge):
>+        """ Collect application specific information. """
>+
>         mozmill = jsbridge.JSObject(bridge, mozmillModuleJs)
>         appInfo = mozmill.appInfo
>-        results = {'app.name' : appInfo.name,
>-                   'app.id' : str(appInfo.ID),
>-                   'app.version' : str(appInfo.version),
>-                   'app.buildID' : str(appInfo.appBuildID),
>-                   'platform.version' : str(appInfo.platformVersion),
>-                   'platform.buildID' : str(appInfo.platformBuildID),
>-                   'uploaded' : datetime.now().isoformat(),
>-                   'locale' : mozmill.locale,
>+
>+        branch = re.search('(\d\.\d)\.*', appInfo.version)
>+
>+        results = {'application_id': str(appInfo.ID),
>+                   'application_name': appInfo.name,
>+                   'application_version': str(appInfo.version),
>+                   'application_branch': branch.group(1),
>+                   'application_locale': mozmill.locale,
>+                   'platform_buildid': str(appInfo.platformBuildID),
>+                   'platform_version': str(appInfo.platformVersion)
I'd be sure to wrap all these values in str() or else if a non-string value creeps in here, it will hose you later.
>                   }
>+
>         return results
>-    
>-    def get_sysinfo(self):
>+
>+    def get_platform_information(self):
>+        """ Retrieves platform information for test reports. Parts of that code
>+            come from the dirtyharry application:
>+            http://github.com/harthur/dirtyharry/blob/master/dirtyutils.py """
>+
Cool!
>         import platform
>+
>         (system, node, release, version, machine, processor) = platform.uname()
>-        sysinfo = {'os.name' : system, 'hostname' : node, 'os.version.number' : version,
>-                   'os.version.string' : release, 'arch' : machine}
>-        if system == 'Darwin':
>-            sysinfo['os.name'] = "Mac OS X"
>-            sysinfo['os.version.number'] = platform.mac_ver()[0]
>-            sysinfo['os.version.string'] = platform.mac_ver()[0]
>-        elif (system == 'linux2') or (system in ('sunos5', 'solaris')):
>-
>-            # XXX mispelling of key; can this be fixed?
>-            sysinfo['linux_distrobution'] = platform.linux_distribution()
>-            
>-            sysinfo['libc_ver'] = platform.libc_ver()        
>-        return sysinfo
>+        (bits, linkage) = platform.architecture()
>+        service_pack = ''
>+
>+        if system in ["Microsoft", "Windows"]:
>+            # There is a Python bug on Windows to determine platform values
>+            # http://bugs.python.org/issue7860
>+            if "PROCESSOR_ARCHITEW6432" in os.environ:
>+              processor = os.environ.get("PROCESSOR_ARCHITEW6432", processor)
>+            else:
>+              processor = os.environ.get('PROCESSOR_ARCHITECTURE', processor)
>+            system = os.environ.get("OS", system).replace('_', ' ')
>+            service_pack = os.sys.getwindowsversion()[4]
>+        elif system == "Linux":
>+            (distro, version, codename) = platform.linux_distribution()
>+            version = distro + " " + version
>+            if not processor:
>+                processor = machine
>+        elif system == "Darwin":
>+            system = "Mac"
>+            (release, versioninfo, machine) = platform.mac_ver()
>+            version = "OS X " + release
>+
>+        if processor in ["i386", "i686"]:
>+            if bits == "32bit":
>+                processor = "x86"
>+            elif bits == "64bit":
>+                processor = "x86_64"
>+        elif processor == "AMD64":
>+            bits = "64bit"
>+            processor = "x86_64"
>+        elif processor == "Power Macintosh":
>+            processor = "ppc"
>+
>+        bits = re.search('(\d+)bit', bits).group(1)
>+
>+        platform = {'hostname': node,
>+                    'system': system,
>+                    'version': version,
>+                    'service_pack': service_pack,
>+                    'processor': processor,
>+                    'bits': bits
>+                   }
>+
>+        return platform
Awesome stuff there.
> 
>     def get_report(self):
>-        """get the report results"""
>+        format = "%Y-%m-%dT%H:%M:%S %z"
> 
>         assert self.test, 'run_tests not called'
>         assert self.starttime, 'starttime not set; have you started the tests?'
>         if not self.endtime:
>-            self.endtime = datetime.utcnow().isoformat()
>-        
>-        results = {'type' : self.report_type,
>-                   'starttime' : self.starttime, 
>-                   'endtime' : self.endtime,
>-                   'testPath' : self.test,
>-                   'tests' : self.alltests
>-                  }
>-        results.update(self.appinfo)
>-        results['sysinfo'] = self.get_sysinfo()
>-        return results
>+            self.endtime = datetime.now(self.localTimezone)
>+
>+        report = {'report_type': self.report_type,
>+                  'report_version': self.report_version,
>+                  'time_start': self.starttime.strftime(format),
>+                  'time_end': self.endtime.strftime(format),
>+                  'time_upload': 'n/a',
>+                  'root_path': self.test,
>+                  'tests_passed': len(self.passes),
>+                  'tests_failed': len(self.fails),
>+                  'tests_skipped': len(self.skipped),
>+                  'results': self.alltests
>+                 }
>+
>+        report.update(self.appinfo)
>+        report.update(self.runner.get_repositoryInfo())
>+        report['system_info'] = self.get_platform_information()
>+
>+        return report
> 
>     def send_report(self, results, report_url):
>         """ Send a report of the results to a CouchdB instance or a file. """
>@@ -372,6 +460,10 @@ class MozMill(object):
> 
>         # report to CouchDB
>         try:
>+            # Set the upload time of the report
>+            now = datetime.now(self.localTimezone)
see above.
>+            results['time_upload'] = now.strftime("%Y-%m-%dT%H:%M:%S %z")
>+
We won't need the timezone info in the time format because it will be UTC (we should add the standard Z to the end)

>             # Parse URL fragments and send data
>             url_fragments = urlparse.urlparse(report_url)
>             connection = httplib.HTTPConnection(url_fragments.netloc)
>@@ -412,9 +504,9 @@ class MozMill(object):
>             pass
> 
>         if not close_bridge:
>-            starttime = datetime.now()
>+            starttime = datetime.now(self.localTimezone)
>             self.runner.wait(timeout=timeout)
>-            endtime = datetime.now()
>+            endtime = datetime.now(self.localTimezone)
This is a bug.  You do not ever want to use local timezone based times for doing timing measurements on systems.  Never ever ever.  Please revert this change.
>             if ( endtime - starttime ) > timedelta(seconds=timeout):
If you do the above then this ^^ measurement will sometimes be flaky and will cause us hang bugs later.
>                 try:
>                     self.runner.stop()
>@@ -458,8 +550,9 @@ class MozMill(object):
>                 pass # assume profile is already cleaned up
> 
> class MozMillRestart(MozMill):
>-	
>+
>     report_type = 'mozmill-restart-test'
>+    report_version = 1
We have way too many versions of things in Mozmill land.  Let's tie this to the Mozmill version.  Can you get that from the egg info or from the attributes Andrew is exposing to do the version command line argument?  I don't want yet one more version to keep track of.  This way Mozmill version 1.4.2 will have report version 1.4.2 and 2.0 will have report version 2.0 etc.
>@@ -486,7 +579,7 @@ class MozMillRestart(MozMill):
> 
>         # set the starttime for the tests
>         # XXX assumes run_tests will be called soon after (currently true)
>-        self.starttime = datetime.utcnow().isoformat()
>+        self.starttime = datetime.now(self.localTimezone)
see above.
>@@ -505,7 +598,7 @@ class MozMillRestart(MozMill):
>         frame = jsbridge.JSObject(self.bridge,
>                                   "Components.utils.import('resource://mozmill/modules/frame.js')")
>         return frame
>diff --git a/mozrunner/mozrunner/__init__.py b/mozrunner/mozrunner/__init__.py
>index 49b03a3..978f7d9 100644
>--- a/mozrunner/mozrunner/__init__.py
>+++ b/mozrunner/mozrunner/__init__.py
>@@ -408,6 +408,28 @@ class Runner(object):
>         """Returns the command list to run."""
>         return [self.binary, '-profile', self.profile.profile]
> 
>+    def get_repositoryInfo(self):
>+        """Read repository information from application.ini and platform.ini."""
>+        import ConfigParser
Very awesome.

I'm going to have to r-.  We cannot have the timezone stuff in Mozmill, I'll listen to your arguments why you want that, but I feel very strongly that is a server-side issue, and is not something we want to add to this test harness.

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #10)
> Comment on attachment 460715 [details] [diff] [review]
> Patch v1 (Mozmill)
> 
> >diff --git a/mozmill/mozmill/__init__.py b/mozmill/mozmill/__init__.py
> >+class LocalTimezone(tzinfo):
> >+    """ Class to automatically determine the timezone offset. """
> >+
[...]
> >+
> I really don't want this change in Mozmill.  Putting timezone code into Mozmill
> just opens up an entire pandora's box of bugs that I'd rather not deal with. 

I should really have asked earlier. :/ It takes me a couple of hours to have it in such a state. But you are right. I missed the points you are calling out here. UTC makes perfect sense and is even necessary for calculating the duration of test runs. I will remove it completely.

> Why do you feel uploading results in local time is necessary inside of Mozmill?

Well, it would have been nice to know from which part of the world the result has been sent. But as talked on IRC we can implement it as part of the extension, where the user can decide if he wants to send i.e. the country. That is definitely more accurate.

> We won't need the timezone info in the time format because it will be UTC (we
> should add the standard Z to the end)

Z will not be necessary. Without a tzinfo instance it will be an empty string. So lets leave it out completely.

> >-            endtime = datetime.now()
> >+            endtime = datetime.now(self.localTimezone)
> This is a bug.  You do not ever want to use local timezone based times for
> doing timing measurements on systems.  Never ever ever.  Please revert this
> change.

Well, to be 100% safe and consistent across the whole module we would also have to use .utcnow() here. 

> >     report_type = 'mozmill-restart-test'
> >+    report_version = 1
> We have way too many versions of things in Mozmill land.  Let's tie this to the
> Mozmill version.  Can you get that from the egg info or from the attributes
> Andrew is exposing to do the version command line argument?  I don't want yet
> one more version to keep track of.  This way Mozmill version 1.4.2 will have
> report version 1.4.2 and 2.0 will have report version 2.0 etc.

As talked on IRC we will remove the report_version from Mozmill. It will be set by the automation scripts instead.

New patch upcoming.

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Patch v2

Fixed review comments based on my previous comment.

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Patch v2.1

Now with the most up2date state (includes saving inside the editor)

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Patch v2.2

We shouldn't create the application_branch value in Mozmill itself, because we will not know how other applications define a branch. It should be part of the automation script.

Comment 15

[cmtalbert]

Comment on attachment 461533 [details] [diff] [review]
Patch v2.2

Thanks for the changes.  These look good.

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Landed as:
http://github.com/whimboo/mozmill/commit/c9f2c0f57f9254e32041f52b20b52f326aa40b46
http://github.com/whimboo/mozmill/commit/afdc6241ad879b43a44358774eecc6def63b0660

Thanks everyone for your input!

Comment 17

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Verified generated results.  Filed bug 584203 re: UTC timestamps but otherwise
looks fine to me.