[rebaseline-cl] Remove extra baselines when rebaselining

This is a follow-up of crrev.com/c/1286894 which let a test fail if
the test has some extra baselines. This CL let rebaseline-cl work for
the failures by removing the extra baselines.

Bug: 703899
Change-Id: Ib811cb1179af399406214ed78cb22520967915cb
Reviewed-on: https://chromium-review.googlesource.com/c/1298124
Commit-Queue: Xianzhu Wang <wangxianzhu@chromium.org>
Reviewed-by: Robert Ma <robertma@chromium.org>
Cr-Commit-Position: refs/heads/master@{#604601}

third_party/blink/tools/blinkpy/common/checkout/baseline_optimizer_unittest.py

@@ -91,10 +91,10 @@ class BaselineOptimizerTest(unittest.TestCase):
         self.assertEqual(sorted(self.host.port_factory.all_port_names()),
                          ['linux-trusty', 'mac-mac10.10', 'mac-mac10.11', 'mac-mac10.12', 'mac-mac10.13', 'win-win10'])
 
-    def _assert_optimization(self, results_by_directory, directory_to_new_results, baseline_dirname=''):
+    def _assert_optimization(self, results_by_directory, directory_to_new_results, baseline_dirname='', suffix='txt'):
         layout_tests_dir = PathFinder(self.fs).layout_tests_dir()
         test_name = 'mock-test.html'
-        baseline_name = 'mock-test-expected.txt'
+        baseline_name = 'mock-test-expected.' + suffix
         self.fs.write_text_file(
             self.fs.join(layout_tests_dir, 'VirtualTestSuites'),
             '[{"prefix": "gpu", "base": "fast/canvas", "args": ["--foo"]}]')
@@ -104,7 +104,7 @@ class BaselineOptimizerTest(unittest.TestCase):
 
         baseline_optimizer = BaselineOptimizer(self.host, self.host.port_factory.get(), self.host.port_factory.all_port_names())
         self.assertTrue(baseline_optimizer.optimize(
-            self.fs.join(baseline_dirname, test_name), 'txt'))
+            self.fs.join(baseline_dirname, test_name), suffix))
 
         for dirname, contents in directory_to_new_results.items():
             path = self.fs.join(layout_tests_dir, dirname, baseline_name)
@@ -119,6 +119,11 @@ class BaselineOptimizerTest(unittest.TestCase):
             if dirname not in directory_to_new_results or directory_to_new_results[dirname] is None:
                 self.assertFalse(self.fs.exists(path), '%s should not exist after optimization' % path)
 
+    def _assert_reftest_optimization(self, results_by_directory, directory_to_new_results, test_path='', baseline_dirname=''):
+        layout_tests_dir = PathFinder(self.fs).layout_tests_dir()
+        self.fs.write_text_file(self.fs.join(layout_tests_dir, test_path, 'mock-test-expected.html'), 'ref')
+        self._assert_optimization(results_by_directory, directory_to_new_results, baseline_dirname, suffix='png')
+
     def test_linux_redundant_with_win(self):
         self._assert_optimization(
             {
@@ -379,6 +384,126 @@ class BaselineOptimizerTest(unittest.TestCase):
             },
             baseline_dirname='virtual/gpu/fast/canvas')
 
+    def test_empty_at_root(self):
+        self._assert_optimization(
+            {'': ''},
+            {'': None})
+
+    def test_empty_at_linux(self):
+        self._assert_optimization(
+            {'platform/linux': ''},
+            {'platform/linux': None})
+
+    def test_empty_at_linux_and_win(self):
+        # https://crbug.com/805008
+        self._assert_optimization(
+            {
+                'platform/linux': '',
+                'platform/win': '',
+            },
+            {
+                'platform/linux': None,
+                'platform/win': None,
+            })
+
+    def test_empty_at_virtual_root(self):
+        self._assert_optimization(
+            {'virtual/gpu/fast/canvas': ''},
+            {'virtual/gpu/fast/canvas': None},
+            baseline_dirname='virtual/gpu/fast/canvas')
+
+    def test_empty_at_virtual_linux(self):
+        self._assert_optimization(
+            {'platform/linux/virtual/gpu/fast/canvas': ''},
+            {'platform/linux/virtual/gpu/fast/canvas': None},
+            baseline_dirname='virtual/gpu/fast/canvas')
+
+    def test_empty_falls_back_to_non_empty(self):
+        # The empty baseline needs to be preserved in this case.
+        self._assert_optimization(
+            {
+                'platform/linux': '',
+                '': '1',
+            },
+            {
+                'platform/linux': '',
+                '': '1',
+            })
+
+    def test_virtual_empty_falls_back_to_non_empty(self):
+        # The empty baseline needs to be preserved in this case.
+        self._assert_optimization(
+            {
+                'virtual/gpu/fast/canvas': '',
+                'platform/linux/fast/canvas': '1',
+            },
+            {
+                'virtual/gpu/fast/canvas': '',
+                'platform/linux/fast/canvas': '1',
+            },
+            baseline_dirname='virtual/gpu/fast/canvas')
+
+    def test_extra_png_for_reftest_at_root(self):
+        self._assert_reftest_optimization(
+            {'': 'extra'},
+            {'': None})
+
+    def test_extra_png_for_reftest_at_linux(self):
+        self._assert_reftest_optimization(
+            {'platform/linux': 'extra'},
+            {'platform/linux': None})
+
+    def test_extra_png_for_reftest_at_linux_and_win(self):
+        # https://crbug.com/805008
+        self._assert_reftest_optimization(
+            {
+                'platform/linux': 'extra1',
+                'platform/win': 'extra2',
+            },
+            {
+                'platform/linux': None,
+                'platform/win': None,
+            })
+
+    def test_extra_png_for_reftest_at_virtual_root(self):
+        self._assert_reftest_optimization(
+            {'virtual/gpu/fast/canvas': 'extra'},
+            {'virtual/gpu/fast/canvas': None},
+            test_path='fast/canvas', baseline_dirname='virtual/gpu/fast/canvas')
+
+    def test_extra_png_for_reftest_at_virtual_linux(self):
+        self._assert_reftest_optimization(
+            {'platform/linux/virtual/gpu/fast/canvas': 'extra'},
+            {'platform/linux/virtual/gpu/fast/canvas': None},
+            test_path='fast/canvas', baseline_dirname='virtual/gpu/fast/canvas')
+
+    def test_extra_png_for_reftest_falls_back_to_base(self):
+        # The extra png for reftest should be removed even if it's different
+        # from the fallback.
+        self._assert_reftest_optimization(
+            {
+                'platform/linux': 'extra1',
+                '': 'extra2',
+            },
+            {
+                'platform/linux': None,
+                '': None,
+            })
+
+    def test_virtual_extra_png_for_reftest_falls_back_to_base(self):
+        # The extra png for reftest should be removed even if it's different
+        # from the fallback.
+        self._assert_reftest_optimization(
+            {
+                'virtual/gpu/fast/canvas': 'extra',
+                'platform/linux/fast/canvas': 'extra2',
+            },
+            {
+                'virtual/gpu/fast/canvas': None,
+                'platform/linux/fast/canvas': None,
+            },
+            test_path='fast/canvas', baseline_dirname='virtual/gpu/fast/canvas')
+
     # Tests for protected methods - pylint: disable=protected-access
 
     def test_move_baselines(self):
@@ -440,26 +565,45 @@ class ResultDigestTest(unittest.TestCase):
         self.fs.write_text_file('/all-pass/foo-expected.txt', ALL_PASS_TESTHARNESS_RESULT)
         self.fs.write_text_file('/all-pass/bar-expected.txt', ALL_PASS_TESTHARNESS_RESULT2)
         self.fs.write_text_file('/failures/baz-expected.txt', 'failure')
-
-    def test_test_all_pass_testharness_result(self):
-        self.assertTrue(ResultDigest.test_all_pass_testharness_result(
-            self.fs, '/all-pass/foo-expected.txt'))
-        self.assertTrue(ResultDigest.test_all_pass_testharness_result(
-            self.fs, '/all-pass/bar-expected.txt'))
-        self.assertFalse(ResultDigest.test_all_pass_testharness_result(
-            self.fs, '/failures/baz-expected.txt'))
-        self.assertFalse(ResultDigest.test_all_pass_testharness_result(
-            self.fs, '/others/something-expected.png'))
-
-    def test_implicit_all_pass(self):
-        # Implicit all-PASS should equal to any all-PASS but not failures.
+        self.fs.write_binary_file('/others/reftest-expected.png', 'extra')
+        self.fs.write_binary_file('/others/reftest2-expected.png', 'extra2')
+        self.fs.write_text_file('/others/empty-expected.txt', '')
+        self.fs.write_binary_file('/others/something-expected.png', 'Something')
+        self.fs.write_binary_file('/others/empty-expected.png', '')
+
+    def test_all_pass_testharness_result(self):
+        self.assertTrue(ResultDigest(self.fs, '/all-pass/foo-expected.txt').is_extra_result)
+        self.assertTrue(ResultDigest(self.fs, '/all-pass/bar-expected.txt').is_extra_result)
+        self.assertFalse(ResultDigest(self.fs, '/failures/baz-expected.txt').is_extra_result)
+
+    def test_empty_result(self):
+        self.assertFalse(ResultDigest(self.fs, '/others/something-expected.png').is_extra_result)
+        self.assertTrue(ResultDigest(self.fs, '/others/empty-expected.txt').is_extra_result)
+        self.assertTrue(ResultDigest(self.fs, '/others/empty-expected.png').is_extra_result)
+
+    def test_extra_png_for_reftest_result(self):
+        self.assertFalse(ResultDigest(self.fs, '/others/something-expected.png').is_extra_result)
+        self.assertTrue(ResultDigest(self.fs, '/others/reftest-expected.png', is_reftest=True).is_extra_result)
+
+    def test_non_extra_result(self):
+        self.assertFalse(ResultDigest(self.fs, '/others/something-expected.png').is_extra_result)
+
+    def test_implicit_extra_result(self):
+        # Implicit empty equal to any extra result but not failures.
         implicit = ResultDigest(None, None)
         self.assertTrue(implicit == ResultDigest(self.fs, '/all-pass/foo-expected.txt'))
         self.assertTrue(implicit == ResultDigest(self.fs, '/all-pass/bar-expected.txt'))
         self.assertFalse(implicit == ResultDigest(self.fs, '/failures/baz-expected.txt'))
+        self.assertTrue(implicit == ResultDigest(self.fs, '/others/reftest-expected.png', is_reftest=True))
 
     def test_different_all_pass_results(self):
         x = ResultDigest(self.fs, '/all-pass/foo-expected.txt')
         y = ResultDigest(self.fs, '/all-pass/bar-expected.txt')
         self.assertTrue(x != y)
         self.assertFalse(x == y)
+
+    def test_same_extra_png_for_reftest(self):
+        x = ResultDigest(self.fs, '/others/reftest-expected.png', is_reftest=True)
+        y = ResultDigest(self.fs, '/others/reftest2-expected.png', is_reftest=True)
+        self.assertTrue(x == y)
+        self.assertFalse(x != y)

third_party/blink/tools/blinkpy/common/net/web_mock.py

@@ -40,7 +40,9 @@ class MockWeb(object):
         self.urls_fetched.append(url)
         if url in self.urls:
             return self.urls[url]
-        return 'MOCK Web result, convert 404 to None=%s' % return_none_on_404
+        if return_none_on_404:
+            return None
+        return 'MOCK Web result, 404 Not found'
 
     def request(self, method, url, data, headers=None):  # pylint: disable=unused-argument
         return MockResponse(self.responses.pop(0))

third_party/blink/tools/blinkpy/tool/commands/rebaseline_test.py

@@ -33,15 +33,6 @@ class RebaselineTest(AbstractRebaseliningCommand):
     def _rebaseline_test_and_update_expectations(self, options):
         self._baseline_suffix_list = options.suffixes.split(',')
 
-        port_name = options.port_name or self._tool.builders.port_name_for_builder_name(options.builder)
-        port = self._tool.port_factory.get(port_name)
-
-        if port.reference_files(options.test):
-            if 'png' in self._baseline_suffix_list:
-                _log.warning('Cannot rebaseline image result for reftest: %s', options.test)
-                return
-            assert self._baseline_suffix_list == ['txt']
-
         if options.results_directory:
             results_url = 'file://' + options.results_directory
         else:
@@ -49,40 +40,58 @@ class RebaselineTest(AbstractRebaseliningCommand):
                 options.builder, build_number=options.build_number,
                 step_name=options.step_name)
 
+        succeeded = True
+        port_name = options.port_name or self._tool.builders.port_name_for_builder_name(options.builder)
+        test_name = options.test
         for suffix in self._baseline_suffix_list:
-            self._rebaseline_test(port_name, options.test, suffix, results_url)
-        self.expectation_line_changes.remove_line(
-            test=options.test,
-            port_name=port_name)
+            if not self._rebaseline_test(port_name, test_name, suffix, results_url):
+                succeeded = False
 
-    def _save_baseline(self, data, target_baseline):
-        if not data:
-            _log.debug('No baseline data to save.')
-            return
-        filesystem = self._tool.filesystem
-        self._tool.filesystem.maybe_make_directory(filesystem.dirname(target_baseline))
-        self._tool.filesystem.write_binary_file(target_baseline, data)
+        if succeeded:
+            self.expectation_line_changes.remove_line(test=test_name, port_name=port_name)
 
     def _rebaseline_test(self, port_name, test_name, suffix, results_url):
         """Downloads a baseline file and saves it to the filesystem.
 
         Args:
-            port_name: The port that the baseline is for. This determines
+            port: The port that the baseline is for. This determines
                 the directory that the baseline is saved to.
             test_name: The name of the test being rebaselined.
             suffix: The baseline file extension (e.g. png); together with the
                 test name and results_url this determines what file to download.
             results_url: Base URL to download the actual result from.
+
+        Returns:
+            True if the rebaseline is successful.
         """
-        baseline_directory = self._tool.port_factory.get(port_name).baseline_version_dir()
+        port = self._tool.port_factory.get(port_name)
+
+        baseline_directory = port.baseline_version_dir()
 
         source_baseline = '%s/%s' % (results_url, self._file_name_for_actual_result(test_name, suffix))
         target_baseline = self._tool.filesystem.join(baseline_directory, self._file_name_for_expected_result(test_name, suffix))
 
-        _log.debug('Retrieving source %s for target %s.', source_baseline, target_baseline)
-        self._save_baseline(
-            self._tool.web.get_binary(source_baseline, return_none_on_404=True),
-            target_baseline)
+        succeeded = True
+        if suffix == 'png' and port.reference_files(test_name):
+            _log.warning('Cannot rebaseline image result for reftest: %s', test_name)
+            succeeded = False
+            data = ''
+            # Still continue in case we can remove extra -expected.png.
+        else:
+            _log.debug('Retrieving source %s for target %s.', source_baseline, target_baseline)
+            data = self._tool.web.get_binary(source_baseline, return_none_on_404=True)
+
+        if not data:
+            # We don't just remove the file because the test may create empty
+            # result on this platform but non-empty on other platforms.
+            # Create an empty file, and let optimization deal with it.
+            _log.debug('Writing empty result %s which may be removed during optimization.', target_baseline)
+            data = ''
+
+        filesystem = self._tool.filesystem
+        filesystem.maybe_make_directory(filesystem.dirname(target_baseline))
+        filesystem.write_binary_file(target_baseline, data)
+        return succeeded
 
     def _print_expectation_line_changes(self):
         print json.dumps(self.expectation_line_changes.to_dict())

third_party/blink/tools/blinkpy/tool/commands/rebaseline_test_unittest.py

@@ -29,11 +29,12 @@ class TestRebaselineTest(BaseTestCase):
         self.tool.executive = MockExecutive()
 
         port = self.tool.port_factory.get('test-win-win7')
-        self._write(
-            port.host.filesystem.join(
-                port.layout_tests_dir(),
-                'platform/test-win-win10/failures/expected/image-expected.txt'),
-            'original win10 result')
+        baseline_relative_path = 'platform/test-win-win10/failures/expected/image-expected.txt'
+        baseline_local_absolute_path = port.host.filesystem.join(port.layout_tests_dir(), baseline_relative_path)
+        self._write(baseline_local_absolute_path, 'original win10 result')
+        actual_result_url = ('https://test-results.appspot.com/data/layout_results/MOCK_Win10/' +
+                             'results/layout-test-results/failures/expected/image-actual.txt')
+        self.tool.web.urls[actual_result_url] = 'new win10 result'
 
         oc = OutputCapture()
         try:
@@ -53,11 +54,9 @@ class TestRebaselineTest(BaseTestCase):
         finally:
             out, _, _ = oc.restore_output()
 
-        self.assertMultiLineEqual(
-            self._read(self.tool.filesystem.join(
-                port.layout_tests_dir(),
-                'platform/test-win-win10/failures/expected/image-expected.txt')),
-            'MOCK Web result, convert 404 to None=True')
+        self.assertItemsEqual(self.tool.web.urls_fetched, [actual_result_url])
+        self.assertMultiLineEqual(self._read(baseline_local_absolute_path),
+                                  'new win10 result')
         self.assertFalse(self.tool.filesystem.exists(self.tool.filesystem.join(
             port.layout_tests_dir(), 'platform/test-win-win7/failures/expected/image-expected.txt')))
         self.assertMultiLineEqual(
@@ -102,9 +101,35 @@ class TestRebaselineTest(BaseTestCase):
 
     def test_rebaseline_test(self):
         # pylint: disable=protected-access
+        actual_result_url = self.WEB_PREFIX + '/userscripts/another-test-actual.txt'
+        self.tool.web.urls[actual_result_url] = 'new result'
+        self.command._rebaseline_test('test-linux-trusty', 'userscripts/another-test.html', 'txt', self.WEB_PREFIX)
+        self.assertItemsEqual(self.tool.web.urls_fetched, [actual_result_url])
+        port = self.tool.port_factory.get('test-linux-trusty')
+        self.assertMultiLineEqual(
+            self._read(port.host.filesystem.join(port.baseline_version_dir(), 'userscripts/another-test-expected.txt')),
+            'new result')
+
+    def test_rebaseline_test_empty_result(self):
+        # pylint: disable=protected-access
+        actual_result_url = self.WEB_PREFIX + '/userscripts/another-test-actual.txt'
+        self.tool.web.urls[actual_result_url] = ''
         self.command._rebaseline_test('test-linux-trusty', 'userscripts/another-test.html', 'txt', self.WEB_PREFIX)
-        self.assertItemsEqual(
-            self.tool.web.urls_fetched, [self.WEB_PREFIX + '/userscripts/another-test-actual.txt'])
+        self.assertItemsEqual(self.tool.web.urls_fetched, [actual_result_url])
+        port = self.tool.port_factory.get('test-linux-trusty')
+        self.assertMultiLineEqual(
+            self._read(port.host.filesystem.join(port.baseline_version_dir(), 'userscripts/another-test-expected.txt')),
+            '')
+
+    def test_rebaseline_test_non_existence_result(self):
+        # pylint: disable=protected-access
+        actual_result_url = self.WEB_PREFIX + '/userscripts/another-test-actual.txt'
+        self.command._rebaseline_test('test-linux-trusty', 'userscripts/another-test.html', 'txt', self.WEB_PREFIX)
+        self.assertItemsEqual(self.tool.web.urls_fetched, [actual_result_url])
+        port = self.tool.port_factory.get('test-linux-trusty')
+        self.assertMultiLineEqual(
+            self._read(port.host.filesystem.join(port.baseline_version_dir(), 'userscripts/another-test-expected.txt')),
+            '')
 
     def test_rebaseline_test_with_results_directory(self):
         # pylint: disable=protected-access
@@ -124,3 +149,15 @@ class TestRebaselineTest(BaseTestCase):
             self, self.command._rebaseline_test_and_update_expectations, args=[self.options(suffixes='png')],
             expected_logs='Cannot rebaseline image result for reftest: userscripts/another-test.html\n')
         self.assertDictEqual(self.command.expectation_line_changes.to_dict(), {'remove-lines': []})
+
+    def test_rebaseline_reftest_with_text(self):
+        # pylint: disable=protected-access
+        self._write('userscripts/another-test.html', 'test data')
+        self._write('userscripts/another-test-expected.html', 'generic result')
+        self._write('userscripts/another-test-expected.txt', 'text')
+        OutputCapture().assert_outputs(
+            self, self.command._rebaseline_test_and_update_expectations, args=[self.options(suffixes='png,txt')],
+            expected_logs='Cannot rebaseline image result for reftest: userscripts/another-test.html\n')
+        self.assertItemsEqual(self.tool.web.urls_fetched,
+                              [self.WEB_PREFIX + '/userscripts/another-test-actual.txt'])
+        self.assertDictEqual(self.command.expectation_line_changes.to_dict(), {'remove-lines': []})