Simplify and add comments to copy-existing-baselines-internal test.

third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_unittest.py

 # 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 optparse
 import unittest
 
 from webkitpy.common.net.buildbot import Build
 from webkitpy.common.net.layouttestresults import LayoutTestResults
 from webkitpy.common.system.executive_mock import MockExecutive, MockExecutive2
@@ skipping 91 matching lines @@
                 }
             }))
 
 
 class TestCopyExistingBaselinesInternal(BaseTestCase):
     command_constructor = CopyExistingBaselinesInternal
 
     def setUp(self):
         super(TestCopyExistingBaselinesInternal, self).setUp()
 
-    def test_copying_overwritten_baseline(self):
-        self.tool.executive = MockExecutive2()
+    def options(self, **kwargs):
+        options_dict = {
+            'results_directory': None,
+            'suffixes': 'txt',
+            'verbose': False,
+        }
+        options_dict.update(kwargs)
+        return optparse.Values(options_dict)
 
-        port = self.tool.port_factory.get('test-mac-mac10.10')
+    def test_copy_baseline_mac(self):
+        port = self.tool.port_factory.get('test-mac-mac10.11')
         self._write(
             port._filesystem.join(
                 port.layout_tests_dir(),
-                'platform/test-mac-mac10.10/failures/expected/image-expected.txt'),
+                'platform/test-mac-mac10.11/failures/expected/image-expected.txt'),
             'original mac10.11 result')
+        self.assertFalse(self.tool.filesystem.exists(
+            self.tool.filesystem.join(
+                port.layout_tests_dir(),
+                'platform/test-mac-mac10.10/failures/expected/image-expected.txt')))
 
         oc = OutputCapture()
-        try:
-            options = optparse.Values({
-                'builder': "MOCK Mac10.11",
-                'suffixes': 'txt',
-                'verbose': True,
-                'test': "failures/expected/image.html",
-                'results_directory': None
-            })
-            oc.capture_output()
-            self.command.execute(options, [], self.tool)
-        finally:
-            out, _, _ = oc.restore_output()
+        oc.capture_output()
+        self.command.execute(self.options(builder='MOCK Mac10.11', test='failures/expected/image.html'), [], self.tool)
+        out, _, _ = oc.restore_output()
 
-        self.assertMultiLineEqual(
+        # The Mac 10.11 baseline is copied over to the Mac 10.10 directory,
+        # because Mac10.10 is the "immediate predecessor" in the fallback tree.
+        # That means that normally for Mac10.10 if there's no Mac10.10-specific
+        # baseline, then we fall back to the Mac10.11 baseline.
+        # The idea is, if in the next step wae download new baselines for Mac10.11

[wkorman, 2016/10/06 21:22:29]

wae -> we

[qyearsley, 2016/10/06 22:00:29]

Done

+        # but not Mac10.10, then mac10.10 will still have the correct baseline.
+        self.assertEqual(
+            self._read(self.tool.filesystem.join(
+                port.layout_tests_dir(),
+                'platform/test-mac-mac10.11/failures/expected/image-expected.txt')),
+            'original mac10.11 result')
+        self.assertEqual(
             self._read(self.tool.filesystem.join(
                 port.layout_tests_dir(),
                 'platform/test-mac-mac10.10/failures/expected/image-expected.txt')),
             'original mac10.11 result')
-        self.assertMultiLineEqual(out, '{"add": [], "remove-lines": [], "delete": []}\n')
+        self.assertEqual(out, '{"add": [], "remove-lines": [], "delete": []}\n')
 
-    def test_copying_overwritten_baseline_to_multiple_locations(self):
-        self.tool.executive = MockExecutive2()
-
+    def test_copy_baseline_win7_to_linux_trusty(self):
         port = self.tool.port_factory.get('test-win-win7')
         self._write(
-            port._filesystem.join(port.layout_tests_dir(), 'platform/test-win-win7/failures/expected/image-expected.txt'),
+            self.tool.filesystem.join(
+                port.layout_tests_dir(),
+                'platform/test-win-win7/failures/expected/image-expected.txt'),
             'original win7 result')
+        self.assertFalse(self.tool.filesystem.exists(
+            self.tool.filesystem.join(
+                port.layout_tests_dir(),
+                'platform/test-linux-trusty/failures/expected/image-expected.txt')))
 
         oc = OutputCapture()
-        try:
-            options = optparse.Values({
-                'builder': "MOCK Win7",
-                'suffixes': "txt",
-                'verbose': True,
-                'test': "failures/expected/image.html",
-                'results_directory': None
-            })
-            oc.capture_output()
-            self.command.execute(options, [], self.tool)
-        finally:
-            out, _, _ = oc.restore_output()
+        oc.capture_output()
+        self.command.execute(self.options(builder='MOCK Win7', test='failures/expected/image.html'), [], self.tool)
+        out, _, _ = oc.restore_output()
 
-        self.assertMultiLineEqual(
+        # The Mac Win7 baseline is copied over to the Linux Trusty directory,
+        # because Linux Trusty is the baseline fallback "immediate predecessor" of Win7.
+        self.assertEqual(
+            self._read(self.tool.filesystem.join(
+                port.layout_tests_dir(),
+                'platform/test-win-win7/failures/expected/image-expected.txt')),
+            'original win7 result')
+        self.assertEqual(
             self._read(self.tool.filesystem.join(
                 port.layout_tests_dir(),
                 'platform/test-linux-trusty/failures/expected/image-expected.txt')),
             'original win7 result')
-        self.assertFalse(self.tool.filesystem.exists(self.tool.filesystem.join(
-            port.layout_tests_dir(), 'platform/test-linux-precise/userscripts/another-test-expected.txt')))
-        self.assertFalse(self.tool.filesystem.exists(self.tool.filesystem.join(
-            port.layout_tests_dir(), 'platform/test-mac-mac10.10/userscripts/another-test-expected.txt')))
-        self.assertMultiLineEqual(out, '{"add": [], "remove-lines": [], "delete": []}\n')
+
+        self.assertEqual(out, '{"add": [], "remove-lines": [], "delete": []}\n')
 
     def test_no_copy_existing_baseline(self):
-        self.tool.executive = MockExecutive2()
-
         port = self.tool.port_factory.get('test-win-win7')
-        self._write(port._filesystem.join(port.layout_tests_dir(),
-                                          'platform/test-win-win7/failures/expected/image-expected.txt'), 'original win7 result')
+        self._write(
+            self.tool.filesystem.join(
+                port.layout_tests_dir(),
+                'platform/test-win-win7/failures/expected/image-expected.txt'),
+            'original win7 result')
+        self._write(
+            self.tool.filesystem.join(
+                port.layout_tests_dir(),
+                'platform/test-linux-trusty/failures/expected/image-expected.txt'),
+            'original linux trusty result')
 
         oc = OutputCapture()
-        try:
-            options = optparse.Values({
-                'builder': "MOCK Win7",
-                'suffixes': "txt",
-                'verbose': True,
-                'test': "failures/expected/image.html",
-                'results_directory': None
-            })
-            oc.capture_output()
-            self.command.execute(options, [], self.tool)
-        finally:

[wkorman, 2016/10/06 21:22:29]

Just to make sure I understand the change, are we removing try/finally because
they shouldn't fail in general and if they do we'd prefer the test fail and
inform us re: what happened?

[qyearsley, 2016/10/06 22:00:29]

Right -- as far as I understand, if any sort of exception is raised here we want
to fail the test, which is what I think should happen.

I think that the try/finally blocks might be here just because that was used
elsewhere in unit tests that use OutputCapture to assert things about the output
(or hide/ignore the output), but here it's unnecessary.

Actually, in this case, I think that asserting things about the output may be
beside the point of the test (and after http://crrev.com/2397573002, this
shouldn't be outputting SCM changes anyway), and hiding the output is
unnecessary because that's done by the test runner.

So, in the latest patchset, I've removed use of OutputCapture from this test
case; does that seem OK?

[wkorman, 2016/10/06 22:29:28]

Yes, thanks.

-            out, _, _ = oc.restore_output()
+        oc.capture_output()
+        self.command.execute(self.options(builder='MOCK Win7', test='failures/expected/image.html'), [], self.tool)
+        out, _, _ = oc.restore_output()
 
-        self.assertMultiLineEqual(
-            self._read(self.tool.filesystem.join(
-                port.layout_tests_dir(),
-                'platform/test-linux-trusty/failures/expected/image-expected.txt')),
-            'original win7 result')
-        self.assertMultiLineEqual(
+        # Since a baseline existed already for Linux Trusty, the Win7 baseline is not copied over.
+        self.assertEqual(
             self._read(self.tool.filesystem.join(
                 port.layout_tests_dir(),
                 'platform/test-win-win7/failures/expected/image-expected.txt')),
             'original win7 result')
-        self.assertFalse(self.tool.filesystem.exists(self.tool.filesystem.join(
-            port.layout_tests_dir(), 'platform/test-mac-mac10.10/userscripts/another-test-expected.txt')))
-        self.assertMultiLineEqual(out, '{"add": [], "remove-lines": [], "delete": []}\n')
+        self.assertEqual(
+            self._read(self.tool.filesystem.join(
+                port.layout_tests_dir(),
+                'platform/test-linux-trusty/failures/expected/image-expected.txt')),
+            'original linux trusty result')
+
+        self.assertEqual(out, '{"add": [], "remove-lines": [], "delete": []}\n')
 
     def test_no_copy_skipped_test(self):
-        self.tool.executive = MockExecutive2()
         port = self.tool.port_factory.get('test-win-win7')
-        fs = self.tool.filesystem
         self._write(
-            fs.join(
+            self.tool.filesystem.join(
                 port.layout_tests_dir(),
                 'platform/test-win-win7/failures/expected/image-expected.txt'),
             'original win7 result')
-        expectations_path = fs.join(port.path_to_generic_test_expectations_file())
         self._write(
-            expectations_path,
+            port.path_to_generic_test_expectations_file(),
             ("[ Win ] failures/expected/image.html [ Failure ]\n"
              "[ Linux ] failures/expected/image.html [ Skip ]\n"))
+
         oc = OutputCapture()
-        try:
-            options = optparse.Values({
-                'builder': "MOCK Win7",
-                'suffixes': "txt", 'verbose': True,
-                'test': "failures/expected/image.html",
-                'results_directory': None
-            })
-            oc.capture_output()
-            self.command.execute(options, [], self.tool)
-        finally:
-            oc.restore_output()
+        oc.capture_output()
+        self.command.execute(self.options(builder='MOCK Win7', test='failures/expected/image.html'), [], self.tool)
+        oc.restore_output()
 
+        # The Win7 baseline is not copied over to the Linux Trusty directory
+        # because the test is skipped on linux.
         self.assertFalse(
-            fs.exists(fs.join(
-                port.layout_tests_dir(),
-                'platform/test-mac-mac10.10/failures/expected/image-expected.txt')))
-        self.assertFalse(
-            fs.exists(fs.join(
+            self.tool.filesystem.exists(self.tool.filesystem.join(
                 port.layout_tests_dir(),
                 'platform/test-linux-trusty/failures/expected/image-expected.txt')))
-        self.assertFalse(
-            fs.exists(fs.join(
-                port.layout_tests_dir(),
-                'platform/test-linux-precise/failures/expected/image-expected.txt')))
-        self.assertEqual(
-            self._read(fs.join(port.layout_tests_dir(), 'platform/test-win-win7/failures/expected/image-expected.txt')),
-            'original win7 result')
 
 
 class TestRebaselineTest(BaseTestCase):
     command_constructor = RebaselineTest  # AKA webkit-patch rebaseline-test-internal
 
     def setUp(self):
         super(TestRebaselineTest, self).setUp()
 
     @staticmethod
     def options(**kwargs):
@@ skipping 664 matching lines @@
         for cmd_line, cwd in commands:
             out = self.run_command(cmd_line, cwd=cwd)
             if 'rebaseline-test-internal' in cmd_line:
                 out = '{"add": [], "remove-lines": [{"test": "%s", "builder": "%s"}], "delete": []}\n' % (cmd_line[8], cmd_line[6])
             command_outputs.append([0, out, ''])
 
         new_calls = self.calls[num_previous_calls:]
         self.calls = self.calls[:num_previous_calls]
         self.calls.append(new_calls)
         return command_outputs