Files app: Add presubmit to block @ts-ignore in TS files.

Fix the `MockFile` to output the same string value of the
implementation.

Bug: b:289003444

Change-Id: I36529ef112af6e81a9e84f6dbb0777ce97e3d543
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4975179
Reviewed-by: Bruce Dawson <brucedawson@chromium.org>
Reviewed-by: Ben Reich <benreich@chromium.org>
Commit-Queue: Luciano Pacheco <lucmult@chromium.org>
Reviewed-by: Wenbo Jie <wenbojie@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1215153}

PRESUBMIT_test_mocks.py

@@ -209,6 +209,9 @@ class MockFile(object):
         self._scm_diff += "+%s\n" % l
     self._old_contents = old_contents
 
+  def __str__(self):
+    return self._local_path
+
   def Action(self):
     return self._action
 

ui/file_manager/PRESUBMIT.py

@@ -2,11 +2,32 @@
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
+import sys
+
 PRESUBMIT_VERSION = '2.0.0'
 
 TEST_PATTERNS = [r'.+_test.py$']
 
 
+class SaveSysPath():
+    """Context manager to save and restore the sys.path.
+  >>> with SaveSysPath(['/path/to/append/to/sys/path']):
+  ...    # do stuff
+
+  >>> # sys.path is back to original value
+  """
+    def __init__(self, additional_paths=None):
+        self._original_path = sys.path[:]
+        self._additional_paths = additional_paths
+
+    def __enter__(self):
+        if self._additional_paths:
+            sys.path.extend(self._additional_paths)
+
+    def __exit__(self, *args):
+        sys.path = self._original_path
+
+
 def ChecksPatchFormatted(input_api, output_api):
     return input_api.canned_checks.CheckPatchFormatted(input_api,
                                                        output_api,
@@ -21,25 +42,32 @@ def ChecksUnitTests(input_api, output_api):
 
 def ChecksCommon(input_api, output_api):
     results = []
-    try:
+    cwd = input_api.PresubmitLocalPath()
-        import sys
+    more_paths = [
-        old_sys_path = sys.path[:]
+        input_api.os_path.join(cwd, '..', '..', 'tools'),
-        cwd = input_api.PresubmitLocalPath()
+        input_api.os_path.join(cwd, '..', 'chromeos'),
+        input_api.os_path.join(cwd),
+    ]
 
-        sys.path += [input_api.os_path.join(cwd, '..', '..', 'tools')]
+    with SaveSysPath(more_paths):
-        import web_dev_style.presubmit_support
+        from web_dev_style.presubmit_support import CheckStyle
-        results += web_dev_style.presubmit_support.CheckStyle(
+        from styles.presubmit_support import _CheckSemanticColors
-            input_api, output_api)
+        from base.presubmit_support import _CheckNoDirectLitImport
+        results += CheckStyle(input_api, output_api)
+        results += _CheckSemanticColors(input_api, output_api)
+        results += _CheckNoDirectLitImport(input_api, output_api)
+
+    return results
+
+
+def CheckBannedTsTags(input_api, output_api):
+    results = []
+    cwd = input_api.PresubmitLocalPath()
+    more_paths = [
+        input_api.os_path.join(cwd),
+    ]
+    with SaveSysPath(more_paths):
+        from base.presubmit_support import _CheckBannedTsTags
+        results += _CheckBannedTsTags(input_api, output_api)
 
-        sys.path += [input_api.os_path.join(cwd, '..', 'chromeos')]
-        import styles.presubmit_support
-        results += styles.presubmit_support._CheckSemanticColors(
-            input_api, output_api)
-
-        sys.path += [input_api.os_path.join(cwd)]
-        import base.presubmit_support
-        results += base.presubmit_support._CheckNoDirectLitImport(
-            input_api, output_api)
-    finally:
-        sys.path = old_sys_path
     return results

ui/file_manager/base/presubmit_support.py

@@ -47,3 +47,27 @@ def _CheckNoDirectLitImport(input_api, output_api):
                 break
 
     return results
+
+
+def _IsComment(line):
+    l = line.lstrip()
+    return l.startswith(('//', '/*', '* '))
+
+
+def _CheckBannedTsTags(input_api, output_api):
+    ts_only = lambda f: f.LocalPath().endswith('.ts')
+    results = []
+    offending_files = []
+    for f in input_api.AffectedFiles(file_filter=ts_only):
+        for line_num, line in f.ChangedContents():
+            if not _IsComment(line):
+                continue
+            if '@ts-ignore' in line:
+                offending_files.append(f'{f}: {line_num}: {line[:100]}')
+
+    if offending_files:
+        results.append(
+            output_api.PresubmitError('@ts-ignore is banned in TS files.',
+                                      offending_files))
+
+    return results

ui/file_manager/base/presubmit_support_test.py

@@ -7,7 +7,7 @@ import os.path
 import unittest
 import sys
 
-from presubmit_support import _CheckNoDirectLitImport
+from presubmit_support import _CheckNoDirectLitImport, _CheckBannedTsTags
 
 sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', '..'))
 from PRESUBMIT_test_mocks import MockInputApi, MockOutputApi, MockFile
@@ -21,26 +21,29 @@ class NoDirectLitImportPresubmit(unittest.TestCase):
 
         self.mock_output_api = MockOutputApi()
 
+    def emulateJsAndTsFiles(self, lines):
+        ts_path = os.path.join('some', 'path', 'foo.ts')
+        js_path = os.path.join('some', 'path', 'foo.js')
+        foo_ts = MockFile(ts_path, lines)
+        foo_js = MockFile(js_path, lines)
+        self.mock_input_api.files.extend((foo_ts, foo_js))
+        return ts_path, js_path
+
     def testWarningWithDirectLitImport(self):
         """
         If a TS file foo.ts or a JS file foo.js is changed, and there's direct
         Lit import in the file, show warnings.
         """
         lines = [
-            "import {aaa} from 'a.js';"
+            "import {aaa} from 'a.js';",
-            "import {css} from 'chrome://resources/mwc/lit/index.js';"
+            "import {css} from 'chrome://resources/mwc/lit/index.js';",
         ]
-        ts_path = os.path.join('some', 'path', 'foo.ts')
+        ts_path, js_path = self.emulateJsAndTsFiles(lines)
-        js_path = os.path.join('some', 'path', 'foo.js')
-        foo_ts = MockFile(ts_path, lines)
-        foo_js = MockFile(js_path, lines)
-        self.mock_input_api.files.append(foo_ts)
-        self.mock_input_api.files.append(foo_js)
         errors = _CheckNoDirectLitImport(self.mock_input_api,
                                          self.mock_output_api)
         self.assertEqual(2, len(errors))
-        self.assertTrue(ts_path + ':1' in errors[0].message)
+        self.assertTrue(ts_path + ':2' in errors[0].message)
-        self.assertTrue(js_path + ':1' in errors[1].message)
+        self.assertTrue(js_path + ':2' in errors[1].message)
 
     def testNoWarningWithDirectLitImportInXfBase(self):
         """
@@ -48,8 +51,8 @@ class NoDirectLitImportPresubmit(unittest.TestCase):
         there's direct lit import in the file, no warnings.
         """
         lines = [
-            "import {aaa} from 'a.js';"
+            "import {aaa} from 'a.js';",
-            "import {css} from 'chrome://resources/mwc/lit/index.js';"
+            "import {css} from 'chrome://resources/mwc/lit/index.js';",
         ]
         xf_base_ts = MockFile(
             os.path.join('ui', 'file_manager', 'file_manager', 'widgets',
@@ -64,14 +67,56 @@ class NoDirectLitImportPresubmit(unittest.TestCase):
         If a TS file foo.ts is changed, and there is no direct Lit import
         in the file, no warnings.
         """
-        foo_ts = MockFile(os.path.join('some', 'path', 'foo.ts'), '')
+        self.emulateJsAndTsFiles(lines=[])
-        foo_js = MockFile(os.path.join('some', 'path', 'foo.js'), '')
-        self.mock_input_api.files.append(foo_ts)
-        self.mock_input_api.files.append(foo_js)
         errors = _CheckNoDirectLitImport(self.mock_input_api,
                                          self.mock_output_api)
         self.assertEqual([], errors)
 
 
+class BannedTsTagsTest(unittest.TestCase):
+    def setUp(self):
+        self.mock_input_api = MockInputApi()
+        self.mock_input_api.change.RepositoryRoot = lambda: os.path.join(
+            os.path.dirname(__file__), '..', '..', '..')
+
+        self.mock_output_api = MockOutputApi()
+
+    def emulateJsAndTsFiles(self, lines):
+        ts_path = os.path.join('some', 'path', 'foo.ts')
+        js_path = os.path.join('some', 'path', 'foo.js')
+        foo_ts = MockFile(ts_path, lines)
+        foo_js = MockFile(js_path, lines)
+        self.mock_input_api.files.extend((foo_ts, foo_js))
+        return ts_path, js_path
+
+    def testNoTags(self):
+        lines = [
+            "import {aaa} from 'a.js';",
+            "import {css} from 'chrome://resources/mwc/lit/index.js';",
+        ]
+        self.emulateJsAndTsFiles(lines)
+        errors = _CheckBannedTsTags(self.mock_input_api, self.mock_output_api)
+        self.assertEqual(0, len(errors))
+
+    def testTagOutsideComment(self):
+        lines = [
+            "import {aaa} from 'a.js';",
+            "@ts-ignore randomly in the code",
+        ]
+        self.emulateJsAndTsFiles(lines)
+        errors = _CheckBannedTsTags(self.mock_input_api, self.mock_output_api)
+        self.assertEqual(0, len(errors))
+
+    def testDetectInComments(self):
+        lines = [
+            "// @ts-ignore: It should block for TS",
+            "import {aaa} from 'a.js';",
+        ]
+        ts_path, js_path = self.emulateJsAndTsFiles(lines)
+        errors = _CheckBannedTsTags(self.mock_input_api, self.mock_output_api)
+        self.assertEqual(1, len(errors))
+        self.assertIn(ts_path, errors[0].items[0])
+
+
 if __name__ == '__main__':
     unittest.main()