[Files app] Fixes in module conversion script and first unittests

Update the modules generation script with the changes below.

Fix reformatting of single-line dependency lists: empty dependency lists
(e.g. `deps = []`) were rewritten on multiple lines (deps = [\n])
incorrectly.

In the `find_dependencies` function, simplify calls to
`add_dependency` by the need for redundant `if is_unittest... else...`.

"//chrome/test/data/webui:chai_assert" can be imported in non-unittest
files, and "//ui/webui/resources/js" can be imported in unittest files.
Remove unittest distinction when importing variabble from these
locations.

Fix function exports: The detection of functions used a variable
(the name of the function the export) that was not defined.

Fix test function exports: when converting the same unittest multiple
times, the "export" keyword was added multiple times in front of each
test function to be exported.

Facilitate testing by adding a `add_js_file_exports` function that
takes `file_lines` as arguments (`convert_js_file` is harder to test
directly, taking a file path as argument).

Fix edge case where, when the only reported closure error is about
extending `EventTarget`, the script doesn't do what's needed to fix it:
Run `find_dependencies` whether or not there is any variable to import.

Add unittests and presubmit script.

Bug: 1133186
Test: ui/file_manager/base/tools/modules_test.py
Change-Id: Ia0ed675576d021b3b952215384e7a491ff7f4430
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2580970
Commit-Queue: Jeremie Boulic <jboulic@chromium.org>
Reviewed-by: Luciano Pacheco <lucmult@chromium.org>
Cr-Commit-Position: refs/heads/master@{#836525}

ui/file_manager/base/tools/PRESUBMIT.py

+# Copyright 2020 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.
+"""Presubmit script for files in ui/file_manager/base/tools/
+
+See http://dev.chromium.org/developers/how-tos/depottools/presubmit-scripts
+for more details about the presubmit API built into depot_tools.
+"""
+
+
+def RunTests(input_api, output_api):
+    presubmit_path = input_api.PresubmitLocalPath()
+    sources = ['modules_test.py']
+    tests = [input_api.os_path.join(presubmit_path, s) for s in sources]
+    return input_api.canned_checks.RunUnitTests(input_api, output_api, tests)
+
+
+def _CheckChangeOnUploadOrCommit(input_api, output_api):
+    results = []
+    affected = input_api.AffectedFiles()
+
+    def is_in_current_folder(f):
+      dirname = input_api.os_path.dirname(f.LocalPath())
+      return dirname == u'ui/file_manager/base/tools'
+
+    # Check if any of the modified files is in 'ui/file_manager/base/tools'.
+    if any([is_in_current_folder(f) for f in affected]):
+        results += RunTests(input_api, output_api)
+
+    return results
+
+
+def CheckChangeOnUpload(input_api, output_api):
+    return _CheckChangeOnUploadOrCommit(input_api, output_api)
+
+
+def CheckChangeOnCommit(input_api, output_api):
+    return _CheckChangeOnUploadOrCommit(input_api, output_api)

ui/file_manager/base/tools/modules.py

@@ -227,23 +227,23 @@ def add_hide_third_party(build_gn_path):
     save_file_lines(build_gn_path, build_file_lines)
 
 
-def add_dependency(file_lines, section_first_line, list_name, dependency_line):
+def add_dependency(file_lines, rule_first_line, list_name, dependency_line):
     '''
     Add dependency in BUILD.gn.
     Parameters:
         file_lines: lines of BUILD.gn file.
-        section_first_line: opening line of target to update.
+        rule_first_line: opening line of target to update.
         list_name: name of the dependency list, deps', 'input_files' etc...
         dependency_line: line to add to the dependency list to update.
     '''
     # Find a line that starts with deps.
-    if not section_first_line in file_lines:
-        print 'Unable to find ' + section_first_line
+    if not rule_first_line in file_lines:
+        print 'Unable to find ' + rule_first_line
         return False
 
     # Find index of `list_name`. Get index of 'sources = [' in case `list_name`
     # is not defined.
-    rule_index = file_lines.index(section_first_line)
+    rule_index = file_lines.index(rule_first_line)
     sources_index = -1
     insertion_index = -1
     single_line_dependency_list = False
@@ -281,17 +281,25 @@ def add_dependency(file_lines, section_first_line, list_name, dependency_line):
         insertion_index = index + 1
 
     if single_line_dependency_list:
-        # If one-line dependency is found, rewrite it over 2 lines, as above.
-        existing_dependency = file_lines[insertion_index].split(' ')[-2]
-        file_lines[insertion_index] = '  {} = ['.format(list_name)
-        file_lines[insertion_index + 1] = '    {},'.format(existing_dependency)
-        file_lines[insertion_index + 2] = '  ]'
+        # Use regex to find characters between [].
+        result = re.search('\[(.*)\]', file_lines[insertion_index])
+        existing_dependency = result.group(1).strip()
+        new_lines = '''\
+  {} = [
+    {},
+  ]'''.format(list_name, existing_dependency)
+
+        # Rewrite single-line dependency list.
+        file_lines[insertion_index:insertion_index + 1] = new_lines.split('\n')
+        insertion_index += 1
 
         # Check for already imported dependency after reformatting.
-        if file_lines[insertion_index + 1] == dependency_line:
+        if file_lines[insertion_index] == dependency_line:
             return False
 
-        insertion_index += 2
+        # If there was no existing dependency, remove appropriate line.
+        if existing_dependency == '':
+            del file_lines[insertion_index]
 
     # Insert dependency.
     file_lines.insert(insertion_index, dependency_line)
@@ -535,18 +543,18 @@ def find_dependencies(dir_path, build_gn_path, js_file_path,
 
     file_name = js_file_path.split('/').pop().replace('.js', '')
 
+    # Define the first line of the rule in which the new dependency has
+    # to be added.
+    rule_first_line = 'js_unittest("%s") {' % (
+        file_name) if is_unittest else 'js_library("%s.m") {' % (file_name)
+
     # Special case: classes that extend "cr.EventTarget".
     index = get_index_substr(js_file_lines, ' extends cr.EventTarget')
     if index >= 0:
         if is_unittest:
             js_file_lines[index] = js_file_lines[index].replace(
                 ' extends cr.EventTarget', ' extends EventTarget')
-            add_dependency(build_file_lines,
-                           'js_unittest("%s") {' % (file_name), 'deps',
-                           '    "//ui/webui/resources/js/cr:event_target.m",')
-        else:
-            add_dependency(build_file_lines,
-                           'js_library("%s.m") {' % (file_name), 'deps',
+        add_dependency(build_file_lines, rule_first_line, 'deps',
                        '    "//ui/webui/resources/js/cr:event_target.m",')
         add_import_line(js_file_lines, 'NativeEventTarget as EventTarget',
                         'chrome://resources/js/cr/event_target.m.js',
@@ -555,7 +563,7 @@ def find_dependencies(dir_path, build_gn_path, js_file_path,
     # General case.
     for variable in variables_to_import:
         # First, handle special cases.
-        if is_unittest:
+
         # "//chrome/test/data/webui:chai_assert".
         out, _ = subprocess.Popen([
             'egrep', '-R', 'export function {}\('.format(variable), '-l',
@@ -563,26 +571,25 @@ def find_dependencies(dir_path, build_gn_path, js_file_path,
         ],
                                   stdout=subprocess.PIPE).communicate()
         if out.rstrip() != '':
-                add_dependency(build_file_lines,
-                               'js_unittest("%s") {' % (file_name), 'deps',
+            add_dependency(build_file_lines, rule_first_line, 'deps',
                            '    "//chrome/test/data/webui:chai_assert",')
             add_import_line(js_file_lines, variable,
                             'chrome://test/chai_assert.js', is_unittest)
             continue
-        else:
+
         # "//ui/webui/resources/js".
         out, _ = subprocess.Popen([
-                'egrep', '-R', '\/\* #export \*\/ \w+ {}\W'.format(variable),
-                '-l', './ui/webui/resources/js'
+            'egrep', '-R', '\/\* #export \*\/ \w+ {}\W'.format(variable), '-l',
+            './ui/webui/resources/js'
         ],
                                   stdout=subprocess.PIPE).communicate()
         path = out.rstrip()
+        print path
         if path != '':
             path = path.replace('./', '//').replace('.js', '.m')
             dependency = ':'.join(path.rsplit(
                 '/', 1))  # //ui/webui/resources/js:assert.m.
-                add_dependency(build_file_lines,
-                               'js_library("%s.m") {' % (file_name), 'deps',
+            add_dependency(build_file_lines, rule_first_line, 'deps',
                            '    "{}",'.format(dependency))
             path = path.replace('//ui/webui/', 'chrome://').replace(
                 '.m', '.m.js')  # chrome://resources/js/assert.m.js.
@@ -619,13 +626,8 @@ def find_dependencies(dir_path, build_gn_path, js_file_path,
         if not relative_path.startswith('../'):
             relative_path = './' + relative_path
         add_import_line(js_file_lines, variable, relative_path, is_unittest)
-        if is_unittest:
         add_dependency(
-                build_file_lines, 'js_unittest("%s") {' % (file_name), 'deps',
-                '    "{}",'.format(get_relative_dependency(path, dir_path)))
-        else:
-            add_dependency(
-                build_file_lines, 'js_library("%s.m") {' % (file_name), 'deps',
+            build_file_lines, rule_first_line, 'deps',
             '    "{}",'.format(get_relative_dependency(path, dir_path)))
 
     # Save BUILD.gn and JS file contents.
@@ -633,17 +635,7 @@ def find_dependencies(dir_path, build_gn_path, js_file_path,
     save_file_lines(js_file_path, js_file_lines)
 
 
-def convert_js_file(js_file_path):
-    '''Add exports (/* #export */) and ignore 'use strict'.'''
-    file_lines = get_file_lines(js_file_path)
-
-    # Ignore 'use strict'.
-    index = get_index_substr(file_lines, "'use strict'")
-    if index >= 0:
-        file_lines[index] = file_lines[index].replace(
-            "'use strict'", "/* #ignore */ 'use strict'")
-
-    # Add exports.
+def add_js_file_exports(file_lines):
     for i, line in enumerate(file_lines):
         # Export class.
         if line.startswith('class '):
@@ -711,11 +703,32 @@ def convert_js_file(js_file_path):
                 file_lines.insert(index, new_line)
         # Export function.
         elif line.startswith('function '):
-            if not get_index_substr(file_lines, ' {}('.format(function_name)):
+            # Extract function name.
+            result = re.search('function (.*)\(', line)
+            function_name = result.group(1).strip()
+
+            # Check if the function is used within the current file.
+            filtered_file_lines = filter(lambda x: x != line, file_lines)
+            if get_index_substr(filtered_file_lines,
+                                '{}('.format(function_name)) < 0:
                 # The function has to be used outside the file, so has to be
                 # exported.
                 file_lines[i] = '/* #export */ ' + line
 
+
+def convert_js_file(js_file_path):
+    '''Add exports (/* #export */) and ignore 'use strict'.'''
+    file_lines = get_file_lines(js_file_path)
+
+    # Ignore 'use strict'.
+    index = get_index_substr(file_lines, "'use strict'")
+    if index >= 0:
+        file_lines[index] = file_lines[index].replace(
+            "'use strict'", "/* #ignore */ 'use strict'")
+
+    # Add exports.
+    add_js_file_exports(file_lines)
+
     # Save file contents.
     save_file_lines(js_file_path, file_lines)
 
@@ -731,7 +744,8 @@ def convert_unittest_file(js_file_path):
 
     # Add exports.
     for i, line in enumerate(file_lines):
-        if line.startswith('function setUp()') or 'function test' in line:
+        if line.startswith('function setUp()') or line.startswith(
+                'function test'):
             file_lines[i] = 'export ' + file_lines[i]
 
     # Save file contents.
@@ -765,12 +779,11 @@ def main():
         compiler_output = sys.argv[3]
         variables_to_import = parse_compiler_output(compiler_output,
                                                     build_gn_path, file_name)
-        if variables_to_import == []:
-            return
         find_dependencies(dir_path, build_gn_path, js_file_path,
                           variables_to_import, is_unittest)
 
 
-print "-----modules.py-----"
-main()
-print "--------------------"
+if __name__ == '__main__':
+    print "-----modules.py-----"
+    main()
+    print "--------------------"

ui/file_manager/base/tools/modules_test.py

+#!/usr/bin/env python
+# Copyright 2020 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 unittest
+import modules
+import os
+
+_HERE_DIR = os.path.dirname(__file__)
+
+
+class ModulesConversionTest(unittest.TestCase):
+    def _get_file_lines(self, file):
+        path = os.path.join(_HERE_DIR, 'tests', file)
+        return modules.get_file_lines(path)
+
+    def _debug_file_contents(self, file_lines, expected_file_lines):
+        print '\n------- FILE -------'
+        print '\n'.join(file_lines)
+        print '----- EXPECTED -----'
+        print '\n'.join(expected_file_lines)
+        print '--------------------\n'
+
+    def testExportFunction(self):
+        '''Tests exporting functions that are not used locally.'''
+        file_lines = self._get_file_lines('export_function.js')
+        expected_file_lines = self._get_file_lines(
+            'export_function_expected.js')
+
+        # Check: function is exported correctly.
+        modules.add_js_file_exports(file_lines)
+        self.assertEquals(file_lines, expected_file_lines)
+
+        # Check: running add_js_file_exports a second time produces the same
+        # expected output.
+        modules.add_js_file_exports(file_lines)
+        self.assertEquals(file_lines, expected_file_lines)
+
+    def testExportLocalFunction(self):
+        '''Tests that functions that are used locally are not be exported.'''
+        file_lines = self._get_file_lines('export_local_function.js')
+        expected_file_lines = self._get_file_lines(
+            'export_local_function_expected.js')
+
+        # Check: local function is not exported.
+        modules.add_js_file_exports(file_lines)
+        self.assertEquals(file_lines, expected_file_lines)
+
+    def testUpdateEmptyDependencyList(self):
+        '''Tests adding a dependency to an empty dependency list.'''
+        file_lines = self._get_file_lines('empty_dependency_list.gn')
+        expected_file_lines = self._get_file_lines(
+            'empty_dependency_list_expected.gn')
+        rule_first_line = 'js_unittest("importer_common_unittest.m") {'
+        list_name = 'deps'
+        dependency_line = '    "//ui/file_manager/base/js:mock_chrome",'
+
+        # Check: dependency list correctly updated with new dependency.
+        modules.add_dependency(file_lines, rule_first_line, list_name,
+                               dependency_line)
+        self.assertEquals(file_lines, expected_file_lines)
+
+        # Check: running add_dependency a second time produces the same
+        # expected output (no duplicate dependency).
+        modules.add_dependency(file_lines, rule_first_line, list_name,
+                               dependency_line)
+        self.assertEquals(file_lines, expected_file_lines)
+
+    def testUpdateSingleLineDependencyList(self):
+        '''Tests adding a dependency to an single-line dependency list.'''
+        file_lines = self._get_file_lines('single_line_dependency_list.gn')
+        expected_file_lines = self._get_file_lines(
+            'single_line_dependency_list_expected.gn')
+        rule_first_line = 'js_unittest("importer_common_unittest.m") {'
+        list_name = 'deps'
+        dependency_line = '    "//ui/file_manager/base/js:mock_chrome",'
+
+        # Check: dependency list correctly formatted and updated with new
+        # dependency.
+        modules.add_dependency(file_lines, rule_first_line, list_name,
+                               dependency_line)
+        self.assertEquals(file_lines, expected_file_lines)
+
+
+if __name__ == '__main__':
+    unittest.main()

ui/file_manager/base/tools/tests/empty_dependency_list.gn

+
+js_unittest("importer_common_unittest.m") {
+  deps = []
+}

ui/file_manager/base/tools/tests/empty_dependency_list_expected.gn

+
+js_unittest("importer_common_unittest.m") {
+  deps = [
+    "//ui/file_manager/base/js:mock_chrome",
+  ]
+}

ui/file_manager/base/tools/tests/export_function.js

+
+function Foo() {}
+Foo.types = [];

ui/file_manager/base/tools/tests/export_function_expected.js

+
+/* #export */ function Foo() {}
+Foo.types = [];

ui/file_manager/base/tools/tests/export_local_function.js

+
+function Foo() {}
+Foo();

ui/file_manager/base/tools/tests/export_local_function_expected.js

+
+function Foo() {}
+Foo();

ui/file_manager/base/tools/tests/single_line_dependency_list.gn

+
+js_unittest("importer_common_unittest.m") {
+  deps = [ ":mock_entry" ]
+}

ui/file_manager/base/tools/tests/single_line_dependency_list_expected.gn

+
+js_unittest("importer_common_unittest.m") {
+  deps = [
+    "//ui/file_manager/base/js:mock_chrome",
+    ":mock_entry",
+  ]
+}