Create test expectations line automatically from failing test results

third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations_unittest.py

Index: third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations_unittest.py
diff --git a/third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations_unittest.py b/third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations_unittest.py
new file mode 100644
index 0000000000000000000000000000000000000000..feb9b5f777c3bfdb20071d3d2131ec039a305f2f
--- /dev/null
+++ b/third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations_unittest.py
@@ -0,0 +1,121 @@
+# Copyright 2016 The Chromium Authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+import json
+import logging
+import unittest
+
+from webkitpy.common.host_mock import MockHost
+from webkitpy.common.net.layouttestresults import LayoutTestResult
+from webkitpy.common.net.web_mock import MockWeb
+from webkitpy.w3c.update_w3c_test_expectations import W3CExpectationsLineAdder
+
+_log = logging.getLogger(__name__)
+
+
+class UpdateW3CTestExpectationsTest(unittest.TestCase, W3CExpectationsLineAdder):
+
+    def setUp(self):
+        self.web = MockWeb(urls={
+            'https://codereview.chromium.org/api/11112222': json.dumps({
+                'patchsets': [1],
+            }),
+            'https://codereview.chromium.org/api/11112222/1': json.dumps({
+                'try_job_results': [
+                    {
+                        'builder': 'foo-builder',
+                        'buildnumber': 20,
+                        'result': 1
+                    },
+                    {
+                        'builder': 'bar-builder',
+                        'buildnumber': 60,
+                        'result': 0
+                    },
+                ],
+            }),
+        })

[qyearsley, 2016/07/20 16:55:28]

Looks like this isn't used currently -- do you think we could add a test for the
main() function?

+        self.mock_dict_one = {
+            'fake/test/path.html': {
+                'one': {'expected': 'FAIL', 'actual': 'PASS', 'bug': 'crbug.com/626703'},
+                'two': {'expected': 'FAIL', 'actual': 'PASS', 'bug': 'crbug.com/626703'}
+            }
+        }
+        self.mock_dict_two = {
+            'fake/test/path.html': {
+                'one': {'expected': 'FAIL', 'actual': 'PASS', 'bug': 'crbug.com/626703'},
+                'two': {'expected': 'FAIL', 'actual': 'TIMEOUT', 'bug': 'crbug.com/626703'},
+                'three': {'expected': 'FAIL', 'actual': 'PASS', 'bug': 'crbug.com/626703'}
+            }
+        }
+        self.mock_dict_three = {
+            'fake/test/path.html': {
+                'four': {'expected': 'FAIL', 'actual': 'PASS', 'bug': 'crbug.com/626703'}}
+        }
+        self.mock_builder_dict = {
+            'foo-builder': {'port_name': 'port-a', 'specifiers': ['A', 'Release'], 'is_try_builder': True},
+            'bar-builder': {'port_name': 'port-b', 'specifiers': ['B', 'Release'], 'is_try_builder': True}
+        }

[qyearsley, 2016/07/20 16:55:28]

Looks like this isn't used anywhere currently

+
+    def test_merge_same_valued_keys(self):
+        self.assertEqual(self.merge_same_valued_keys(self.mock_dict_one['fake/test/path.html']), {
+            ('two', 'one'): {'expected': 'FAIL', 'actual': 'PASS', 'bug': 'crbug.com/626703'}
+        })
+        self.assertEqual(self.merge_same_valued_keys(self.mock_dict_two['fake/test/path.html']), {
+            ('three', 'one'): {'expected': 'FAIL', 'actual': 'PASS', 'bug': 'crbug.com/626703'},
+            'two': {'expected': 'FAIL', 'actual': 'TIMEOUT', 'bug': 'crbug.com/626703'}
+        })
+
+    def test_get_expectations(self):
+        self.assertEqual(self.get_expectations({'expected': 'FAIL', 'actual': 'PASS'}), ['Pass'])
+        self.assertEqual(self.get_expectations({'expected': 'FAIL', 'actual': 'TIMEOUT'}), ['Timeout'])
+        self.assertEqual(self.get_expectations({'expected': 'TIMEOUT', 'actual': 'PASS'}), ['Pass', 'Timeout'])
+
+    def test_create_line_list(self):
+        self.assertEqual(self.create_line_list(self.mock_dict_one),
+                         ['crbug.com/626703 [ two ] fake/test/path.html [ Pass ]',
+                          'crbug.com/626703 [ one ] fake/test/path.html [ Pass ]'])
+        self.assertEqual(self.create_line_list(self.mock_dict_two),
+                         ['crbug.com/626703 [ three ] fake/test/path.html [ Pass ]',
+                          'crbug.com/626703 [ two ] fake/test/path.html [ Timeout ]',
+                          'crbug.com/626703 [ one ] fake/test/path.html [ Pass ]'])
+
+    def test_merge_dicts(self):
+        self.assertRaises(Exception, self.merge_dicts, self.mock_dict_one, self.mock_dict_two)
+        self.assertTrue('four' in self.merge_dicts(self.mock_dict_one, self.mock_dict_three)['fake/test/path.html'])

[qyearsley, 2016/07/20 16:55:28]

This test method could make an assertion about what the whole output of
merge_dicts looks like. It could also assert that the output dict is the same as
the first argument, and that the first argument was changed, e.g.:

output = self.merge_dicts(self.mock_dict_one, self.mock_dict_three)
self.assertEqual(output, self.mock_dict_one)
self.assertEqual(output, ...)

Also, one alternative to `self.assertTrue(x in y)` is `self.assertIn(x, y)`,
which is a bit more concise.


Finally, I want to note that ideally, each test method should test one
"behavior", and you could consider "raising an exception when the inputs
conflict" and "successfully merging recursively when there is no conflict" as
two different behaviors. So you can split this up into test methods:

  test_merge_dicts_with_conflict_raises_exception
  test_merge_dicts_merges_second_dict_into_first

+
+    def test_generate_results_dict(self):
+        line_adder = W3CExpectationsLineAdder(MockHost())
+        layout_test_list = [LayoutTestResult(
+            'test/name.html', {
+                'expected': 'bar',
+                'actual': 'foo',
+                'is_unexpected': True,
+                'has_stderr': True
+            }
+        )]
+        platform = 'dummy_platform'
+        self.assertEqual(line_adder._generate_results_dict(platform, layout_test_list), {
+            'test/name.html': {
+                'dummy_platform': {
+                    'expected': 'bar',
+                    'actual': 'foo',
+                    'bug': 'crbug.com/626703'
+                }
+            }
+        })
+
+    def test_write_to_test_expectations(self):
+        line_adder = W3CExpectationsLineAdder(MockHost())
+        host = MockHost()

[qyearsley, 2016/07/20 16:55:28]

It's probably simpler to use the same host object for the
W3CExpectationsLineAdder and for the filesystem -- and this is what's done in
the real main() function.

  host = MockHost()
  line_adder = W3CExpectationsLineAdder(host)

[dcampb, 2016/07/20 21:11:59]

done, had to make a public filesystem attribute to the W3CExpectationsAdder
class. 

+        host.filesystem.files = {'TestExpectations': '# Tests added from W3C auto import bot\n'}
+        line_list = ['fake crbug [foo] fake/file/path.html [Pass]']
+        path = 'TestExpectations'
+        line_adder.write_to_test_expectations(host, path, line_list)
+        value = host.filesystem.read_text_file(path)
+        self.assertEqual(value, '# Tests added from W3C auto import bot\nfake crbug [foo] fake/file/path.html [Pass]\n')
+        host.filesystem.files['TestExpectations'] = 'not empty\n'
+        line_adder.write_to_test_expectations(host, path, line_list)
+        value = host.filesystem.read_text_file(path)
+        self.assertEqual(value, 'not empty\n\n# Tests added from W3C auto import bot\nfake crbug [foo] fake/file/path.html [Pass]')

[qyearsley, 2016/07/20 16:55:28]

This test method looks like it could be split into two -- testing with the
comment marker already existing, and without.

[dcampb, 2016/07/20 21:11:59]

done