perf-o-matic should store test results' units

https://bugs.webkit.org/show_bug.cgi?id=82852

Reviewed by Kentaro Hara.

.:

* Websites/webkit-perf.appspot.com/models.py:
(Test):
(Test.update_or_insert): Added "unit" to the argument list.
(Test.update_or_insert.execute): Store the unit.
(ReportLog.results_are_well_formed): Moved from ReportHandler.
(ReportLog.results_are_well_formed._is_float_convertible): Ditto.
* Websites/webkit-perf.appspot.com/models_unittest.py:
(TestModelTests.test_update_or_insert): Added a test case for "unit" argument.
(TestModelTests.test_update_or_insert_to_update): Ditto.
(ReportLogTests.test_results_are_well_formed): Added.
(ReportLogTests.test_results_are_well_formed.assert_results_are_well_formed): Added.
* Websites/webkit-perf.appspot.com/report_handler.py:
(ReportHandler.post): Calls ReportLog.results_are_well_formed.
* Websites/webkit-perf.appspot.com/report_process_handler.py:
(ReportProcessHandler.post): Passes results['unit'] to Test.update_or_insert.

Tools:

Include units in the results JSON.

* Scripts/webkitpy/performance_tests/perftestsrunner.py:
(PerfTestsRunner._process_chromium_style_test_result):
(PerfTestsRunner._process_parser_test_result):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@112829 268f45cc-cd09-0410-ab3c-d52691b4dbfc

diff --git a/ChangeLog b/ChangeLog
index 549049f..dc2a05f 100644 (file)
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,28 @@
 2012-04-01  Ryosuke Niwa  <rniwa@webkit.org>
 
 2012-04-01  Ryosuke Niwa  <rniwa@webkit.org>
 

+        perf-o-matic should store test results' units
+        https://bugs.webkit.org/show_bug.cgi?id=82852
+
+        Reviewed by Kentaro Hara.
+
+        * Websites/webkit-perf.appspot.com/models.py:
+        (Test):
+        (Test.update_or_insert): Added "unit" to the argument list.
+        (Test.update_or_insert.execute): Store the unit.
+        (ReportLog.results_are_well_formed): Moved from ReportHandler.
+        (ReportLog.results_are_well_formed._is_float_convertible): Ditto.
+        * Websites/webkit-perf.appspot.com/models_unittest.py:
+        (TestModelTests.test_update_or_insert): Added a test case for "unit" argument.
+        (TestModelTests.test_update_or_insert_to_update): Ditto.
+        (ReportLogTests.test_results_are_well_formed): Added.
+        (ReportLogTests.test_results_are_well_formed.assert_results_are_well_formed): Added.
+        * Websites/webkit-perf.appspot.com/report_handler.py:
+        (ReportHandler.post): Calls ReportLog.results_are_well_formed.
+        * Websites/webkit-perf.appspot.com/report_process_handler.py:
+        (ReportProcessHandler.post): Passes results['unit'] to Test.update_or_insert.
+
+2012-04-01  Ryosuke Niwa  <rniwa@webkit.org>
+

         Admin page should lexicologically sort tests
         https://bugs.webkit.org/show_bug.cgi?id=82849
 
         Admin page should lexicologically sort tests
         https://bugs.webkit.org/show_bug.cgi?id=82849
 

diff --git a/Tools/ChangeLog b/Tools/ChangeLog
index 4ab910c..208b43e 100644 (file)
--- a/Tools/ChangeLog
+++ b/Tools/ChangeLog
@@ -1,3 +1,16 @@
+2012-04-01  Ryosuke Niwa  <rniwa@webkit.org>
+
+        perf-o-matic should store test results' units
+        https://bugs.webkit.org/show_bug.cgi?id=82852
+
+        Reviewed by Kentaro Hara.
+
+        Include units in the results JSON.
+
+        * Scripts/webkitpy/performance_tests/perftestsrunner.py:
+        (PerfTestsRunner._process_chromium_style_test_result):
+        (PerfTestsRunner._process_parser_test_result):
+

 2012-04-01  Tony Tseung  <tseung@apple.com>
 
         Composite Font References is a new established standard (ISO/IEC 14496-28:2012) for specifying
 2012-04-01  Tony Tseung  <tseung@apple.com>
 
         Composite Font References is a new established standard (ISO/IEC 14496-28:2012) for specifying

diff --git a/Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py b/Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py
index db15eb3..79b0c29 100644 (file)
--- a/Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py
+++ b/Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py
@@ -250,6 +250,7 @@ class PerfTestsRunner(object):
         for line in re.split('\n', output.text):
             resultLine = self._inspector_result_regex.match(line)
             if resultLine:
         for line in re.split('\n', output.text):
             resultLine = self._inspector_result_regex.match(line)
             if resultLine:

+                # FIXME: Store the unit

                 self._results[resultLine.group('name').replace(' ', '')] = float(resultLine.group('value'))
                 self._buildbot_output.write("%s\n" % line)
                 got_a_result = True
                 self._results[resultLine.group('name').replace(' ', '')] = float(resultLine.group('value'))
                 self._buildbot_output.write("%s\n" % line)
                 got_a_result = True


@@ -286,6 +287,7 @@ class PerfTestsRunner(object):
         keys = ['avg', 'median', 'stdev', 'min', 'max']
         score_regex = re.compile(r'^(?P<key>' + r'|'.join(keys) + r')\s+(?P<value>[0-9\.]+)\s*(?P<unit>.*)')
         unit = "ms"
         keys = ['avg', 'median', 'stdev', 'min', 'max']
         score_regex = re.compile(r'^(?P<key>' + r'|'.join(keys) + r')\s+(?P<value>[0-9\.]+)\s*(?P<unit>.*)')
         unit = "ms"

+

         for line in re.split('\n', output.text):
             score = score_regex.match(line)
             if score:
         for line in re.split('\n', output.text):
             score = score_regex.match(line)
             if score:


@@ -300,6 +302,9 @@ class PerfTestsRunner(object):
 
         if test_failed or set(keys) != set(results.keys()):
             return True
 
         if test_failed or set(keys) != set(results.keys()):
             return True

+
+        results['unit'] = unit
+

         self._results[filesystem.join(category, test_name).replace('\\', '/')] = results
         self._buildbot_output.write('RESULT %s: %s= %s %s\n' % (category, test_name, results['avg'], unit))
         self._buildbot_output.write(', '.join(['%s= %s %s' % (key, results[key], unit) for key in keys[1:]]) + '\n')
         self._results[filesystem.join(category, test_name).replace('\\', '/')] = results
         self._buildbot_output.write('RESULT %s: %s= %s %s\n' % (category, test_name, results['avg'], unit))
         self._buildbot_output.write(', '.join(['%s= %s %s' % (key, results[key], unit) for key in keys[1:]]) + '\n')

diff --git a/Websites/webkit-perf.appspot.com/models.py b/Websites/webkit-perf.appspot.com/models.py
index 9ad5d1e..9c1ffeb 100644 (file)
--- a/Websites/webkit-perf.appspot.com/models.py
+++ b/Websites/webkit-perf.appspot.com/models.py
@@ -148,10 +148,11 @@ class Test(db.Model):
     # one platform but only on some branch and vice versa.
     branches = db.ListProperty(db.Key)
     platforms = db.ListProperty(db.Key)
     # one platform but only on some branch and vice versa.
     branches = db.ListProperty(db.Key)
     platforms = db.ListProperty(db.Key)

+    unit = db.StringProperty()

     hidden = db.BooleanProperty()
 
     @staticmethod
     hidden = db.BooleanProperty()
 
     @staticmethod

-    def update_or_insert(test_name, branch, platform):
+    def update_or_insert(test_name, branch, platform, unit=None):

         existing_test = [None]
 
         def execute(id):
         existing_test = [None]
 
         def execute(id):


@@ -161,6 +162,7 @@ class Test(db.Model):
                     test.branches.append(branch.key())
                 if platform.key() not in test.platforms:
                     test.platforms.append(platform.key())
                     test.branches.append(branch.key())
                 if platform.key() not in test.platforms:
                     test.platforms.append(platform.key())

+                test.unit = unit

                 test.put()
                 existing_test[0] = test
                 return None
                 test.put()
                 existing_test[0] = test
                 return None


@@ -252,6 +254,33 @@ class ReportLog(db.Model):
     def results(self):
         return self.get_value('results')
 
     def results(self):
         return self.get_value('results')
 

+    def results_are_well_formed(self):
+
+        def _is_float_convertible(value):
+            try:
+                float(value)
+                return True
+            except TypeError:
+                return False
+            except ValueError:
+                return False
+
+        if not isinstance(self.results(), dict):
+            return False
+
+        for testResult in self.results().values():
+            if isinstance(testResult, dict):
+                for key, value in testResult.iteritems():
+                    if key != "unit" and not _is_float_convertible(value):
+                        return False
+                if 'avg' not in testResult:
+                    return False
+                continue
+            if not _is_float_convertible(testResult):
+                return False
+
+        return True
+

     def builder(self):
         return self._model_by_key_name_in_payload(Builder, 'builder-name')
 
     def builder(self):
         return self._model_by_key_name_in_payload(Builder, 'builder-name')
 

diff --git a/Websites/webkit-perf.appspot.com/models_unittest.py b/Websites/webkit-perf.appspot.com/models_unittest.py
index fa4856b..cc298e5 100644 (file)
--- a/Websites/webkit-perf.appspot.com/models_unittest.py
+++ b/Websites/webkit-perf.appspot.com/models_unittest.py
@@ -265,6 +265,7 @@ class TestModelTests(DataStoreTestsBase):
         self.assertTrue(test)
         self.assertEqual(test.branches, [branch.key()])
         self.assertEqual(test.platforms, [platform.key()])
         self.assertTrue(test)
         self.assertEqual(test.branches, [branch.key()])
         self.assertEqual(test.platforms, [platform.key()])

+        self.assertEqual(test.unit, None)

         self.assertOnlyInstance(test)
 
     def test_update_or_insert_to_update(self):
         self.assertOnlyInstance(test)
 
     def test_update_or_insert_to_update(self):


@@ -275,14 +276,11 @@ class TestModelTests(DataStoreTestsBase):
 
         other_branch = Branch.create_if_possible('other-branch', 'Other Branch')
         other_platform = Platform.create_if_possible('other-platform', 'Other Platform')
 
         other_branch = Branch.create_if_possible('other-branch', 'Other Branch')
         other_platform = Platform.create_if_possible('other-platform', 'Other Platform')

-        test = Test.update_or_insert('some-test', other_branch, other_platform)
+        test = Test.update_or_insert('some-test', other_branch, other_platform, 'ms')

         self.assertOnlyInstance(test)
         self.assertEqualUnorderedList(test.branches, [branch.key(), other_branch.key()])
         self.assertEqualUnorderedList(test.platforms, [platform.key(), other_platform.key()])
         self.assertOnlyInstance(test)
         self.assertEqualUnorderedList(test.branches, [branch.key(), other_branch.key()])
         self.assertEqualUnorderedList(test.platforms, [platform.key(), other_platform.key()])

-
-        test = Test.get(test.key())
-        self.assertEqualUnorderedList(test.branches, [branch.key(), other_branch.key()])
-        self.assertEqualUnorderedList(test.platforms, [platform.key(), other_platform.key()])
+        self.assertEqualUnorderedList(test.unit, 'ms')

 
     def test_merge(self):
         branch, platform, builder = _create_some_builder()
 
     def test_merge(self):
         branch, platform, builder = _create_some_builder()


@@ -433,6 +431,20 @@ class ReportLogTests(DataStoreTestsBase):
         log = self._create_log_with_payload('{"key": "value"}')
         self.assertEqual(log.results(), None)
 
         log = self._create_log_with_payload('{"key": "value"}')
         self.assertEqual(log.results(), None)
 

+    def test_results_are_well_formed(self):
+
+        def assert_results_are_well_formed(json, expected):
+            self.assertEqual(self._create_log_with_payload(json).results_are_well_formed(), expected)
+
+        assert_results_are_well_formed('{"results": 123}', False)
+        assert_results_are_well_formed('{"results": {"test": 123}}', True)
+        assert_results_are_well_formed('{"results": {"test": 123, "other-test": 456}}', True)
+        assert_results_are_well_formed('{"results": {"test": 123, "other-test": 456, "bad-test": "hi"}}', False)
+        assert_results_are_well_formed('{"results": {"test": {"avg": 456}}}', True)
+        assert_results_are_well_formed('{"results": {"test": {"avg": 456, "median": "hello"}}}', False)
+        assert_results_are_well_formed('{"results": {"test": {"avg": 456, "median": 789}}}', True)
+        assert_results_are_well_formed('{"results": {"test": {"avg": 456, "unit": "bytes"}}}', True)
+

     def test_builder(self):
         log = self._create_log_with_payload('{"key": "value"}')
         self.assertEqual(log.builder(), None)
     def test_builder(self):
         log = self._create_log_with_payload('{"key": "value"}')
         self.assertEqual(log.builder(), None)

diff --git a/Websites/webkit-perf.appspot.com/report_handler.py b/Websites/webkit-perf.appspot.com/report_handler.py
index 9618875..0f1adb1 100644 (file)
--- a/Websites/webkit-perf.appspot.com/report_handler.py
+++ b/Websites/webkit-perf.appspot.com/report_handler.py
@@ -69,7 +69,7 @@ class ReportHandler(webapp2.RequestHandler):
         if builder and not (self.bypass_authentication() or builder.authenticate(password)):
             self._output('Authentication failed')
 
         if builder and not (self.bypass_authentication() or builder.authenticate(password)):
             self._output('Authentication failed')
 

-        if not self._results_are_valid(log):
+        if not log.results_are_well_formed():

             self._output("The payload doesn't contain results or results are malformed")
 
         if self._encountered_error:
             self._output("The payload doesn't contain results or results are malformed")
 
         if self._encountered_error:


@@ -87,33 +87,6 @@ class ReportHandler(webapp2.RequestHandler):
     def bypass_authentication(self):
         return False
 
     def bypass_authentication(self):
         return False
 

-    def _results_are_valid(self, log):
-
-        def _is_float_convertible(value):
-            try:
-                float(value)
-                return True
-            except TypeError:
-                return False
-            except ValueError:
-                return False
-
-        if not isinstance(log.results(), dict):
-            return False
-
-        for testResult in log.results().values():
-            if isinstance(testResult, dict):
-                for value in testResult.values():
-                    if not _is_float_convertible(value):
-                        return False
-                if 'avg' not in testResult:
-                    return False
-                continue
-            if not _is_float_convertible(testResult):
-                return False
-
-        return True
-

 
 class AdminReportHandler(ReportHandler):
     def bypass_authentication(self):
 
 class AdminReportHandler(ReportHandler):
     def bypass_authentication(self):

diff --git a/Websites/webkit-perf.appspot.com/report_process_handler.py b/Websites/webkit-perf.appspot.com/report_process_handler.py
index 7103785..f57b557 100644 (file)
--- a/Websites/webkit-perf.appspot.com/report_process_handler.py
+++ b/Websites/webkit-perf.appspot.com/report_process_handler.py
@@ -58,7 +58,7 @@ class ReportProcessHandler(webapp2.RequestHandler):
         build = Build.get_or_insert_from_log(log)
 
         for test_name, result_value in log.results().iteritems():
         build = Build.get_or_insert_from_log(log)
 
         for test_name, result_value in log.results().iteritems():

-            test = Test.update_or_insert(test_name, branch, platform)
+            test = Test.update_or_insert(test_name, branch, platform, result_value.get('unit'))

             result = TestResult.get_or_insert_from_parsed_json(test_name, build, result_value)
             runs = Runs.get_by_objects(branch, platform, test)
             regenerate_runs = True
             result = TestResult.get_or_insert_from_parsed_json(test_name, build, result_value)
             runs = Runs.get_by_objects(branch, platform, test)
             regenerate_runs = True