Unexpected reftest passes are only reported when pixel testing is enabled in results...
authorzandobersek@gmail.com <zandobersek@gmail.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 25 Sep 2012 06:17:58 +0000 (06:17 +0000)
committerzandobersek@gmail.com <zandobersek@gmail.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 25 Sep 2012 06:17:58 +0000 (06:17 +0000)
https://bugs.webkit.org/show_bug.cgi?id=97426

Reviewed by Ojan Vafai.

Tools:

The 'is_reftest' member of the TestResult class has been replaced by the reftest_type
member which is a set containing either '!=', '==', both or none if the test represented
by the object is not a reftest.

When summarizing results, the test dictionary is updated to contain the 'reftest_type' key
if the test is a reftest. The value of this key is a list of all the reftest types of this test.

The test failure interpretation method has been refactored, it now reports only
missing results of various formats and image diff percent in case of image or
reftest mismatch.

Unit tests have been modified accordingly.

* Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py:
(LayoutTestRunner._update_summary_with_result):
* Scripts/webkitpy/layout_tests/controllers/layout_test_runner_unittest.py:
(LayoutTestRunnerTests.test_update_summary_with_result):
* Scripts/webkitpy/layout_tests/controllers/manager.py:
(interpret_test_failures):
(summarize_results):
* Scripts/webkitpy/layout_tests/controllers/manager_unittest.py:
(ResultSummaryTest.test_interpret_test_failures):
* Scripts/webkitpy/layout_tests/controllers/single_test_runner.py:
(SingleTestRunner.run):
(SingleTestRunner._run_reftest):
* Scripts/webkitpy/layout_tests/models/test_results.py:
(TestResult.__init__):
* Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py:
(EndToEndTest.test_reftest_with_two_notrefs):

LayoutTests:

Report unexpectedly passing reftests as well. Test cases included.

* fast/harness/resources/results-test.js:
* fast/harness/results-expected.txt:
* fast/harness/results.html:

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

12 files changed:
LayoutTests/ChangeLog
LayoutTests/fast/harness/resources/results-test.js
LayoutTests/fast/harness/results-expected.txt
LayoutTests/fast/harness/results.html
Tools/ChangeLog
Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py
Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner_unittest.py
Tools/Scripts/webkitpy/layout_tests/controllers/manager.py
Tools/Scripts/webkitpy/layout_tests/controllers/manager_unittest.py
Tools/Scripts/webkitpy/layout_tests/controllers/single_test_runner.py
Tools/Scripts/webkitpy/layout_tests/models/test_results.py
Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py

index fdb66bd..286ef7e 100644 (file)
@@ -1,3 +1,16 @@
+2012-09-24  Zan Dobersek  <zandobersek@gmail.com>
+
+        Unexpected reftest passes are only reported when pixel testing is enabled in results.html as well
+        https://bugs.webkit.org/show_bug.cgi?id=97426
+
+        Reviewed by Ojan Vafai.
+
+        Report unexpectedly passing reftests as well. Test cases included.
+
+        * fast/harness/resources/results-test.js:
+        * fast/harness/results-expected.txt:
+        * fast/harness/results.html:
+
 2012-09-24  Filip Pizlo  <fpizlo@apple.com>
 
         SerializedScriptValue isn't aware of indexed storage, but should be
index 18228ec..907fecc 100644 (file)
@@ -266,14 +266,29 @@ function runTests()
 
     results = mockResults();
     results.tests['bar-reftest.html'] = mockExpectation('PASS', 'IMAGE', 1);
-    results.tests['bar-reftest.html'].is_reftest = true;
+    results.tests['bar-reftest.html'].reftest_type = ['=='];
     runSingleRowTest(results, false, '', 'ref html images diff (1%) ');
 
     results = mockResults();
     results.tests['bar-reftest-mismatch.html'] = mockExpectation('PASS', 'IMAGE');
-    results.tests['bar-reftest-mismatch.html'].is_mismatch_reftest = true;
+    results.tests['bar-reftest-mismatch.html'].reftest_type = ['!='];
     runSingleRowTest(results, false, '', 'ref mismatch html actual ');
 
+    results = mockResults();
+    results.tests['bar-reftest.html'] = mockExpectation('IMAGE', 'PASS');
+    results.tests['bar-reftest.html'].reftest_type = ['=='];
+    results.pixel_tests_enabled = false;
+    runTest(results, function() {
+        assertTrue(document.querySelector('tbody td:nth-child(1)').textContent == 'bar-reftest.html \u2691');
+    });
+
+    results = mockResults();
+    results.tests['bar-reftest-mismatch.html'] = mockExpectation('IMAGE', 'PASS');
+    results.tests['bar-reftest-mismatch.html'].reftest_type = ['!='];
+    results.pixel_tests_enabled = false;
+    runTest(results, function() {
+        assertTrue(document.querySelector('tbody td:nth-child(1)').textContent == 'bar-reftest-mismatch.html \u2691');
+    });
 
     results = mockResults();
     var subtree = results.tests['foo'] = {}
index ff0fb96..b93c332 100644 (file)
@@ -94,18 +94,13 @@ TEST-15: PASS
 TEST-15: PASS
 TEST-15: PASS
 TEST-16: PASS
-TEST-16: PASS
-TEST-16: PASS
-TEST-17: PASS
 TEST-17: PASS
-TEST-17: PASS
-TEST-18: PASS
 TEST-18: PASS
 TEST-18: PASS
 TEST-18: PASS
 TEST-19: PASS
 TEST-19: PASS
-TEST-20: PASS
+TEST-19: PASS
 TEST-20: PASS
 TEST-20: PASS
 TEST-20: PASS
@@ -114,47 +109,42 @@ TEST-21: PASS
 TEST-21: PASS
 TEST-22: PASS
 TEST-22: PASS
+TEST-22: PASS
+TEST-22: PASS
+TEST-22: PASS
+TEST-23: PASS
 TEST-23: PASS
 TEST-24: PASS
 TEST-24: PASS
 TEST-25: PASS
 TEST-26: PASS
+TEST-26: PASS
 TEST-27: PASS
 TEST-28: PASS
-TEST-28: PASS
-TEST-28: PASS
 TEST-29: PASS
-TEST-29: PASS
-TEST-30: PASS
-TEST-30: PASS
-TEST-30: PASS
 TEST-30: PASS
 TEST-30: PASS
 TEST-30: PASS
-TEST-30: PASS
-TEST-30: PASS
-TEST-30: PASS
-TEST-30: PASS
-TEST-30: PASS
-TEST-30: PASS
-TEST-30: PASS
-TEST-30: PASS
-TEST-30: PASS
-TEST-30: PASS
-TEST-31: PASS
-TEST-31: PASS
-TEST-31: PASS
-TEST-31: PASS
-TEST-31: PASS
-TEST-31: PASS
-TEST-31: PASS
-TEST-31: PASS
 TEST-31: PASS
 TEST-31: PASS
 TEST-32: PASS
 TEST-32: PASS
 TEST-32: PASS
 TEST-32: PASS
+TEST-32: PASS
+TEST-32: PASS
+TEST-32: PASS
+TEST-32: PASS
+TEST-32: PASS
+TEST-32: PASS
+TEST-32: PASS
+TEST-32: PASS
+TEST-32: PASS
+TEST-32: PASS
+TEST-32: PASS
+TEST-32: PASS
+TEST-33: PASS
+TEST-33: PASS
 TEST-33: PASS
 TEST-33: PASS
 TEST-33: PASS
@@ -166,55 +156,18 @@ TEST-33: PASS
 TEST-34: PASS
 TEST-34: PASS
 TEST-34: PASS
+TEST-34: PASS
+TEST-35: PASS
+TEST-35: PASS
+TEST-35: PASS
+TEST-35: PASS
+TEST-35: PASS
 TEST-35: PASS
 TEST-35: PASS
 TEST-35: PASS
 TEST-36: PASS
 TEST-36: PASS
 TEST-36: PASS
-TEST-36: PASS
-TEST-36: PASS
-TEST-36: PASS
-TEST-36: PASS
-TEST-36: PASS
-TEST-36: PASS
-TEST-36: PASS
-TEST-36: PASS
-TEST-36: PASS
-TEST-36: PASS
-TEST-36: PASS
-TEST-36: PASS
-TEST-36: PASS
-TEST-36: PASS
-TEST-36: PASS
-TEST-36: PASS
-TEST-36: PASS
-TEST-36: PASS
-TEST-36: PASS
-TEST-36: PASS
-TEST-36: PASS
-TEST-36: PASS
-TEST-36: PASS
-TEST-36: PASS
-TEST-36: PASS
-TEST-36: PASS
-TEST-36: PASS
-TEST-36: PASS
-TEST-36: PASS
-TEST-36: PASS
-TEST-36: PASS
-TEST-36: PASS
-TEST-36: PASS
-TEST-36: PASS
-TEST-37: PASS
-TEST-37: PASS
-TEST-37: PASS
-TEST-37: PASS
-TEST-37: PASS
-TEST-37: PASS
-TEST-37: PASS
-TEST-37: PASS
-TEST-37: PASS
 TEST-37: PASS
 TEST-37: PASS
 TEST-37: PASS
@@ -222,6 +175,55 @@ TEST-38: PASS
 TEST-38: PASS
 TEST-38: PASS
 TEST-38: PASS
+TEST-38: PASS
+TEST-38: PASS
+TEST-38: PASS
+TEST-38: PASS
+TEST-38: PASS
+TEST-38: PASS
+TEST-38: PASS
+TEST-38: PASS
+TEST-38: PASS
+TEST-38: PASS
+TEST-38: PASS
+TEST-38: PASS
+TEST-38: PASS
+TEST-38: PASS
+TEST-38: PASS
+TEST-38: PASS
+TEST-38: PASS
+TEST-38: PASS
+TEST-38: PASS
+TEST-38: PASS
+TEST-38: PASS
+TEST-38: PASS
+TEST-38: PASS
+TEST-38: PASS
+TEST-38: PASS
+TEST-38: PASS
+TEST-38: PASS
+TEST-38: PASS
+TEST-38: PASS
+TEST-38: PASS
+TEST-38: PASS
+TEST-38: PASS
+TEST-38: PASS
+TEST-39: PASS
+TEST-39: PASS
+TEST-39: PASS
+TEST-39: PASS
+TEST-39: PASS
+TEST-39: PASS
+TEST-39: PASS
+TEST-39: PASS
+TEST-39: PASS
 TEST-39: PASS
 TEST-39: PASS
 TEST-39: PASS
+TEST-40: PASS
+TEST-40: PASS
+TEST-40: PASS
+TEST-40: PASS
+TEST-41: PASS
+TEST-41: PASS
+TEST-41: PASS
index fcaf72f..c7d7f6a 100644 (file)
@@ -512,7 +512,7 @@ function processGlobalStateFor(testObject)
     }
 
     if (actual == 'PASS' && expected != 'PASS') {
-        if (expected != 'IMAGE' || globalState().results.pixel_tests_enabled) {
+        if (expected != 'IMAGE' || (globalState().results.pixel_tests_enabled || testObject.reftest_type)) {
             globalState().unexpectedPassTests.push(testObject);
         }
         return;
@@ -568,7 +568,7 @@ function tableRow(testObject)
     var row = '<tbody'
     if (globalState().results.uses_expectations_file)
         row += ' class="' + (testObject.isExpected ? 'expected' : '') + '"';
-    if (testObject.is_mismatch_reftest)
+    if (testObject.reftest_type && testObject.reftest_type.indexOf('!=') != -1)
         row += ' mismatchreftest=true';
     row += '><tr>';
 
@@ -602,11 +602,11 @@ function tableRow(testObject)
     if (actual.indexOf('IMAGE') != -1) {
         globalState().hasImageFailures = true;
 
-        if (testObject.is_mismatch_reftest) {
+        if (testObject.reftest_type && testObject.reftest_type.indexOf('!=') != -1) {
             row += resultLink(test_prefix, '-expected-mismatch.html', 'ref mismatch html');
             row += resultLink(test_prefix, '-actual.png', 'actual');
         } else {
-            if (testObject.is_reftest) {
+            if (testObject.reftest_type && testObject.reftest_type.indexOf('==') != -1) {
                 row += resultLink(test_prefix, '-expected.html', 'ref html');
             }
             if (globalState().shouldToggleImages) {
index c744224..bb79b13 100644 (file)
@@ -1,3 +1,40 @@
+2012-09-24  Zan Dobersek  <zandobersek@gmail.com>
+
+        Unexpected reftest passes are only reported when pixel testing is enabled in results.html as well
+        https://bugs.webkit.org/show_bug.cgi?id=97426
+
+        Reviewed by Ojan Vafai.
+
+        The 'is_reftest' member of the TestResult class has been replaced by the reftest_type
+        member which is a set containing either '!=', '==', both or none if the test represented
+        by the object is not a reftest.
+
+        When summarizing results, the test dictionary is updated to contain the 'reftest_type' key
+        if the test is a reftest. The value of this key is a list of all the reftest types of this test.
+
+        The test failure interpretation method has been refactored, it now reports only
+        missing results of various formats and image diff percent in case of image or
+        reftest mismatch.
+
+        Unit tests have been modified accordingly.
+
+        * Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py:
+        (LayoutTestRunner._update_summary_with_result):
+        * Scripts/webkitpy/layout_tests/controllers/layout_test_runner_unittest.py:
+        (LayoutTestRunnerTests.test_update_summary_with_result):
+        * Scripts/webkitpy/layout_tests/controllers/manager.py:
+        (interpret_test_failures):
+        (summarize_results):
+        * Scripts/webkitpy/layout_tests/controllers/manager_unittest.py:
+        (ResultSummaryTest.test_interpret_test_failures):
+        * Scripts/webkitpy/layout_tests/controllers/single_test_runner.py:
+        (SingleTestRunner.run):
+        (SingleTestRunner._run_reftest):
+        * Scripts/webkitpy/layout_tests/models/test_results.py:
+        (TestResult.__init__):
+        * Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py:
+        (EndToEndTest.test_reftest_with_two_notrefs):
+
 2012-09-24  Sam Weinig  <sam@webkit.org>
 
         Use NSUserDefaults rather than an environment variable to control whether to use an XPC Service for the WebProcess
index 69d2cc0..4201286 100644 (file)
@@ -198,7 +198,7 @@ class LayoutTestRunner(object):
             exp_str = got_str = 'SKIP'
             expected = True
         else:
-            expected = self._expectations.matches_an_expected_result(result.test_name, result.type, self._options.pixel_tests or result.is_reftest)
+            expected = self._expectations.matches_an_expected_result(result.test_name, result.type, self._options.pixel_tests or result.reftest_type)
             exp_str = self._expectations.get_expectations_string(result.test_name)
             got_str = self._expectations.expectation_to_string(result.type)
 
index 5062d52..4efa4e0 100644 (file)
@@ -152,13 +152,13 @@ class LayoutTestRunnerTests(unittest.TestCase):
         runner._expectations = expectations
 
         result_summary = ResultSummary(expectations, [test], 1, set())
-        result = TestResult(test_name=test, failures=[test_failures.FailureReftestMismatchDidNotOccur()], is_reftest=True)
+        result = TestResult(test_name=test, failures=[test_failures.FailureReftestMismatchDidNotOccur()], reftest_type=['!='])
         runner._update_summary_with_result(result_summary, result)
         self.assertEquals(1, result_summary.expected)
         self.assertEquals(0, result_summary.unexpected)
 
         result_summary = ResultSummary(expectations, [test], 1, set())
-        result = TestResult(test_name=test, failures=[], is_reftest=True)
+        result = TestResult(test_name=test, failures=[], reftest_type=['=='])
         runner._update_summary_with_result(result_summary, result)
         self.assertEquals(0, result_summary.expected)
         self.assertEquals(1, result_summary.unexpected)
index e6924a3..d06ed71 100644 (file)
@@ -75,7 +75,7 @@ def interpret_test_failures(port, test_name, failures):
         test_name: test name relative to layout_tests directory
         failures: list of test failures
     Returns:
-        A dictionary like {'is_reftest': True, ...}
+        A dictionary like {'is_missing_text': True, ...}
     """
     test_dict = {}
     failure_types = [type(failure) for failure in failures]
@@ -84,20 +84,16 @@ def interpret_test_failures(port, test_name, failures):
     if test_failures.FailureMissingAudio in failure_types:
         test_dict['is_missing_audio'] = True
 
-    for failure in failures:
-        if isinstance(failure, test_failures.FailureImageHashMismatch):
-            test_dict['image_diff_percent'] = failure.diff_percent
-        elif isinstance(failure, test_failures.FailureReftestMismatch):
-            test_dict['is_reftest'] = True
-            test_dict['image_diff_percent'] = failure.diff_percent
-        elif isinstance(failure, test_failures.FailureReftestMismatchDidNotOccur):
-            test_dict['is_mismatch_reftest'] = True
-
     if test_failures.FailureMissingResult in failure_types:
         test_dict['is_missing_text'] = True
 
     if test_failures.FailureMissingImage in failure_types or test_failures.FailureMissingImageHash in failure_types:
         test_dict['is_missing_image'] = True
+
+    for failure in failures:
+        if isinstance(failure, test_failures.FailureImageHashMismatch) or isinstance(failure, test_failures.FailureReftestMismatch):
+            test_dict['image_diff_percent'] = failure.diff_percent
+
     return test_dict
 
 
@@ -172,6 +168,9 @@ def summarize_results(port_obj, expectations, result_summary, retry_summary, tes
         if result.has_stderr:
             test_dict['has_stderr'] = True
 
+        if result.reftest_type:
+            test_dict.update(reftest_type=list(result.reftest_type))
+
         if expectations.has_modifier(test_name, test_expectations.WONTFIX):
             test_dict['wontfix'] = True
 
index dcd24a4..0984099 100644 (file)
@@ -104,24 +104,32 @@ class ResultSummaryTest(unittest.TestCase):
 
     def test_interpret_test_failures(self):
         test_dict = interpret_test_failures(self.port, 'foo/reftest.html',
-            [test_failures.FailureReftestMismatch(self.port.abspath_for_test('foo/reftest-expected.html'))])
-        self.assertTrue('is_reftest' in test_dict)
-        self.assertFalse('is_mismatch_reftest' in test_dict)
+            [test_failures.FailureImageHashMismatch(diff_percent=0.42)])
+        self.assertEqual(test_dict['image_diff_percent'], 0.42)
 
         test_dict = interpret_test_failures(self.port, 'foo/reftest.html',
-            [test_failures.FailureReftestMismatch(self.port.abspath_for_test('foo/common.html'))])
-        self.assertTrue('is_reftest' in test_dict)
-        self.assertFalse('is_mismatch_reftest' in test_dict)
+            [test_failures.FailureReftestMismatch(self.port.abspath_for_test('foo/reftest-expected.html'))])
+        self.assertTrue('image_diff_percent' in test_dict)
 
         test_dict = interpret_test_failures(self.port, 'foo/reftest.html',
             [test_failures.FailureReftestMismatchDidNotOccur(self.port.abspath_for_test('foo/reftest-expected-mismatch.html'))])
-        self.assertFalse('is_reftest' in test_dict)
-        self.assertTrue(test_dict['is_mismatch_reftest'])
+        self.assertDictEqual(test_dict, {})
 
-        test_dict = interpret_test_failures(self.port, 'foo/reftest.html',
-            [test_failures.FailureReftestMismatchDidNotOccur(self.port.abspath_for_test('foo/common.html'))])
-        self.assertFalse('is_reftest' in test_dict)
-        self.assertTrue(test_dict['is_mismatch_reftest'])
+        test_dict = interpret_test_failures(self.port, 'foo/audio-test.html',
+            [test_failures.FailureMissingAudio()])
+        self.assertTrue('is_missing_audio' in test_dict)
+
+        test_dict = interpret_test_failures(self.port, 'foo/text-test.html',
+            [test_failures.FailureMissingResult()])
+        self.assertTrue('is_missing_text' in test_dict)
+
+        test_dict = interpret_test_failures(self.port, 'foo/pixel-test.html',
+            [test_failures.FailureMissingImage()])
+        self.assertTrue('is_missing_image' in test_dict)
+
+        test_dict = interpret_test_failures(self.port, 'foo/pixel-test.html',
+            [test_failures.FailureMissingImageHash()])
+        self.assertTrue('is_missing_image' in test_dict)
 
     def get_result(self, test_name, result_type=test_expectations.PASS, run_time=0):
         failures = []
index 09b4e1d..0cb7aef 100644 (file)
@@ -94,7 +94,8 @@ class SingleTestRunner(object):
     def run(self):
         if self._reference_files:
             if self._port.get_option('no_ref_tests') or self._options.reset_results:
-                result = TestResult(self._test_name, is_reftest=True)
+                reftest_type = set([reference_file[0] for reference_file in self._reference_files])
+                result = TestResult(self._test_name, reftest_type=reftest_type)
                 result.type = test_expectations.SKIP
                 return result
             return self._run_reftest()
@@ -303,7 +304,8 @@ class SingleTestRunner(object):
 
         assert(reference_output)
         test_result_writer.write_test_result(self._filesystem, self._port, self._test_name, test_output, reference_output, test_result.failures)
-        return TestResult(self._test_name, test_result.failures, total_test_time + test_result.test_run_time, test_result.has_stderr, is_reftest=True)
+        reftest_type = set([reference_file[0] for reference_file in self._reference_files])
+        return TestResult(self._test_name, test_result.failures, total_test_time + test_result.test_run_time, test_result.has_stderr, reftest_type=reftest_type)
 
     def _compare_output_with_reference(self, reference_driver_output, actual_driver_output, reference_filename, mismatch):
         total_test_time = reference_driver_output.test_time + actual_driver_output.test_time
index d6c8301..6b9db55 100644 (file)
@@ -38,12 +38,12 @@ class TestResult(object):
     def loads(string):
         return cPickle.loads(string)
 
-    def __init__(self, test_name, failures=None, test_run_time=None, has_stderr=False, is_reftest=False):
+    def __init__(self, test_name, failures=None, test_run_time=None, has_stderr=False, reftest_type=[]):
         self.test_name = test_name
         self.failures = failures or []
         self.test_run_time = test_run_time or 0
         self.has_stderr = has_stderr
-        self.is_reftest = is_reftest
+        self.reftest_type = reftest_type
         # FIXME: Setting this in the constructor makes this class hard to mutate.
         self.type = test_failures.determine_result_type(failures)
 
index 5e6e422..d10360e 100755 (executable)
@@ -968,7 +968,7 @@ class EndToEndTest(unittest.TestCase):
         self.assertEqual(json["tests"]["reftests"]["foo"]["multiple-mismatch-failure.html"],
             {"expected": "PASS", "actual": "IMAGE", "is_mismatch_reftest": True})
         self.assertEqual(json["tests"]["reftests"]["foo"]["multiple-both-failure.html"],
-            {"expected": "PASS", "actual": "IMAGE", "is_mismatch_reftest": True})
+            {"expected": "PASS", "actual": "IMAGE", "is_mismatch_reftest": True, "is_reftest": True})
 
 
 class RebaselineTest(unittest.TestCase, StreamTestingMixin):