address comments

oppia · brianrodri · Sep 8, 2019 · Aug 17, 2019 · Aug 18, 2019 · Aug 18, 2019
commit 5ac03b0a2f414b57925daf2e08d735ded32e7fa8
@@ -81,8 +81,10 @@ def create_test_suites(test_target=None):
             top_level_dir=CURR_DIR)])
 
 
-def main():
+def main(args=None):
     """Runs the tests."""
+    parsed_args = _PARSER.parse_args(args=args)
+
     for directory in DIRS_TO_ADD_TO_SYS_PATH:
         if not os.path.exists(os.path.dirname(directory)):
             raise Exception('Directory %s does not exist.' % directory)
@@ -91,7 +93,6 @@ def main():
     import dev_appserver
     dev_appserver.fix_sys_path()
 
-    parsed_args = _PARSER.parse_args()
     suites = create_test_suites(test_target=parsed_args.test_target)
 
     results = [unittest.TextTestRunner(verbosity=2).run(suite)

@@ -41,7 +41,7 @@ def test_cannot_add_directory_with_invalid_path(self):
         assert_raises_regexp_context_manager = self.assertRaisesRegexp(
             Exception, 'Directory invalid_path does not exist.')
         with assert_raises_regexp_context_manager, dir_to_add_swap:
-            gae_suite.main()
+            gae_suite.main(args=[])
 
     def test_failing_tests(self):
 
@@ -57,7 +57,7 @@ def _mock_create_test_suites(**unused_test_target):
             'Test suite failed: 1 tests run, 0 errors, 1 failures.')
 
         with create_test_suites_swap, assert_raises_regexp_context_manager:
-            gae_suite.main()
+            gae_suite.main(args=[])
 
     def test_no_tests_run_with_invalid_filename(self):
 
@@ -72,4 +72,4 @@ def _mock_create_test_suites(**unused_test_target):
             Exception, 'No module named invalid_test')
 
         with create_test_suites_swap, assert_raises_regexp_context_manager:
-            gae_suite.main()
+            gae_suite.main(args=[])
@@ -143,7 +143,12 @@
 APP_DEV_YAML_FILEPATH = 'app_dev.yaml'
 APP_YAML_FILEPATH = 'app.yaml'
 
-_PARSER = argparse.ArgumentParser()
+_PARSER = argparse.ArgumentParser(description="""
+Creates a third-party directory where all the JS and CSS dependencies are
+built and stored. Depending on the options passed to the script, might also
+minify third-party libraries and/or generate a build directory.
+""")
+
 _PARSER.add_argument(
     '--prod_env', action='store_true', default=False, dest='prod_mode')
 _PARSER.add_argument(
@@ -1359,12 +1364,7 @@ def compile_typescript_files_continuously(project_dir):
 
 
 def main(args=None):
-    """The main method of this script.
-
-    Creates a third-party directory where all the JS and CSS dependencies are
-    built and stored. Depending on the options passed to the script, might also
-    minify third-party libraries and/or generate a build directory.
-    """
+    """The main method of this script."""
     options = _PARSER.parse_args(args=args)
     # Regenerate /third_party/generated from scratch.
     safe_delete_directory_tree(THIRD_PARTY_GENERATED_DEV_DIR)

@@ -920,7 +920,7 @@ def mock_compare_file_count(unused_first_dir, unused_second_dir):
 
         with ensure_files_exist_swap, build_using_webpack_swap, (
             compile_typescript_files_swap), compare_file_count_swap, args_swap:
-            build.main()
+            build.main(args=[])
 
         self.assertEqual(check_function_calls, expected_check_function_calls)
 
@@ -950,7 +950,7 @@ def mock_compile_typescript_files_continuously(unused_project_dir):
 
         with ensure_files_exist_swap, (
             compile_typescript_files_continuously_swap), args_swap:
-            build.main()
+            build.main(args=[])
 
         self.assertEqual(check_function_calls, expected_check_function_calls)
 
@@ -982,7 +982,7 @@ def mock_compile_typescript_files(unused_project_dir):
 
         with ensure_files_exist_swap, compile_typescript_files_swap, (
             assert_raises_regexp_context_manager), args_swap:
-            build.main()
+            build.main(args=[])
 
         self.assertEqual(check_function_calls, expected_check_function_calls)
 

@@ -16,6 +16,7 @@
 from __future__ import absolute_import  # pylint: disable=import-only-modules
 from __future__ import unicode_literals  # pylint: disable=import-only-modules
 
+import argparse
 import os
 import shutil
 
@@ -24,6 +25,10 @@
 CURR_DIR = os.path.abspath(os.getcwd())
 OPPIA_TOOLS_DIR = os.path.join(CURR_DIR, '..', 'oppia_tools')
 
+_PARSER = argparse.ArgumentParser(description="""
+Deletes temporary and installed files.
+""")
+
 
 def delete_directory_tree(directory_path):
     """Recursively delete an existing directory tree. Does not do anything if
@@ -48,8 +53,10 @@ def delete_file(filepath):
     os.remove(filepath)
 
 
-def main():
+def main(args=None):
     """Runs the script to clean temporary and installed files."""
+    unused_parsed_args = _PARSER.parse_args(args=args)
+
     delete_directory_tree(OPPIA_TOOLS_DIR)
     delete_directory_tree('node_modules/')
     delete_directory_tree('third_party/')

@@ -28,10 +28,10 @@
 from . import setup
 
 _PARSER = argparse.ArgumentParser(description="""
-    Run this script from the oppia root folder:
-        python -m scripts.create_expression_parser
-
-    The root folder MUST be named 'oppia'.""")
+Run this script from the oppia root folder:
+    python -m scripts.create_expression_parser
+The root folder MUST be named 'oppia'.
+""")
 
 
 def main(args=None):

@@ -25,8 +25,9 @@
 import python_utils
 
 _PARSER = argparse.ArgumentParser(description="""
-    This script should only be ran by Travis to install and provide a constant
-    version of Chrome.""")
+This script should only be ran by Travis to install and provide a constant
-This script should only be ran by Travis to install and provide a constant
+This script should only be run by Travis to install and provide a constant
-This script should only be ran by Travis to install and provide a constant
+This script should only be run by Travis to install and provide a constant
+version of Chrome.
+""")
 
 
 def main(args=None):

@@ -71,7 +71,8 @@
 }
 
 _PARSER = argparse.ArgumentParser(description="""
-    Installation script for Oppia third-party libraries.""")
+Installation script for Oppia third-party libraries.
+""")
 
 
 def download_files(source_url_root, target_dir, source_filenames):

@@ -57,7 +57,8 @@
 # pylint: enable=wrong-import-position
 
 _PARSER = argparse.ArgumentParser(description="""
-    Installation script for Oppia third-party libraries.""")
+Installation script for Oppia third-party libraries.
+""")
 
 _PARSER.add_argument(
     '--nojsrepl',
@@ -108,7 +109,7 @@ def pip_install(package, version, install_path):
         install_path])
 
 
-def install_skulpt(args):
+def install_skulpt(parsed_args):
     """Download and install Skulpt. Skulpt is built using a Python script
     included within the Skulpt repository (skulpt.py). This script normally
     requires GitPython, however the patches to it below
@@ -118,7 +119,6 @@ def install_skulpt(args):
     warning saying its dist command will not work properly without GitPython,
     but it does actually work due to the patches.
     """
-    parsed_args = _PARSER.parse_args(args=args)
     no_skulpt = parsed_args.nojsrepl or parsed_args.noskulpt
 
     python_utils.PRINT('Checking whether Skulpt is installed in third_party')
@@ -228,6 +228,8 @@ def ensure_pip_library_is_installed(package, version, path):
 
 def main(args=None):
     """Install third-party libraries for Oppia."""
+    parsed_args = _PARSER.parse_args(args=args)
+
     setup.main(args=[])
     setup_gae.main(args=[])
     pip_dependencies = [
@@ -257,7 +259,7 @@ def main(args=None):
     # 374076889.
     subprocess.call([common.NPM_PATH, 'dedupe'])
 
-    install_skulpt(args)
+    install_skulpt(parsed_args)
 
     # Install pre-commit script.
     python_utils.PRINT('Installing pre-commit hook for git')

diff --git a/scripts/release_info.py b/scripts/release_info.py
@@ -55,7 +55,11 @@
 
 Log = collections.namedtuple('Log', ['sha1', 'author', 'email', 'message'])
 
-_PARSER = argparse.ArgumentParser()
+_PARSER = argparse.ArgumentParser(description="""
+Script that simplifies releases by collecting various information.
+Should be run from the oppia root dir.
+""")
+
 _PARSER.add_argument(
     '--personal_access_token',
     help=(
@@ -293,14 +297,15 @@ def _check_storage_models(current_release):
     return [item for item in diff_list if item.startswith('core/storage')]
 
 
-def main():
+def main(args=None):
     """Collects necessary info and dumps it to disk."""
+    parsed_args = _PARSER.parse_args(args=args)
+
     branch_name = _get_current_branch()
     if not re.match(r'release-\d+\.\d+\.\d+$', branch_name):
         raise Exception(
             'This script should only be run from the latest release branch.')
 
-    parsed_args = _PARSER.parse_args()
     if parsed_args.personal_access_token is None:
         python_utils.PRINT(
             'No personal access token provided, please set up a personal '

@@ -81,11 +81,10 @@
 
 
 _PARSER = argparse.ArgumentParser(description="""
-    Run this script from the oppia root folder:
-
-        python -m scripts.run_backend_tests
-
-    IMPORTANT: Only one of --test_path and --test_target should be specified""")
+Run this script from the oppia root folder:
+    python -m scripts.run_backend_tests
+IMPORTANT: Only one of --test_path and --test_target should be specified.
+""")
 
 _EXCLUSIVE_GROUP = _PARSER.add_mutually_exclusive_group()
 _EXCLUSIVE_GROUP.add_argument(
@@ -310,6 +309,8 @@ def _get_test_target_classes(path):
 
 def main(args=None):
     """Run the tests."""
+    parsed_args = _PARSER.parse_args(args=args)
+
     setup.main(args=[])
     setup_gae.main(args=[])
 
@@ -321,7 +322,6 @@ def main(args=None):
     import dev_appserver
     dev_appserver.fix_sys_path()
 
-    parsed_args = _PARSER.parse_args(args=args)
     if parsed_args.generate_coverage_report:
         python_utils.PRINT(
             'Checking whether coverage is installed in %s'

diff --git a/scripts/run_e2e_tests.py b/scripts/run_e2e_tests.py
@@ -35,14 +35,12 @@
 from . import setup_gae
 
 _PARSER = argparse.ArgumentParser(description="""
-    Run this script from the oppia root folder:
-
-        python -m scripts.run_e2e_tests
-
-    The root folder MUST be named 'oppia'.
-
-    Note: You can replace 'it' with 'fit' or 'describe' with 'fdescribe' to run
-    a single test or test suite.""")
+Run this script from the oppia root folder:
+    python -m scripts.run_e2e_tests
+The root folder MUST be named 'oppia'.
+Note: You can replace 'it' with 'fit' or 'describe' with 'fdescribe' to run
+a single test or test suite.
+""")
 
 _PARSER.add_argument(
     '--skip_install',
@@ -103,12 +101,13 @@ def cleanup():
 
 def main(args=None):
     """Runs the end to end tests."""
+    parsed_args = _PARSER.parse_args(args=args)
+
     setup.main(args=[])
     setup_gae.main(args=[])
     if os.environ.get('TRAVIS'):
         install_chrome_on_travis.main(args=[])
 
-    parsed_args = _PARSER.parse_args(args=args)
     install_third_party_libs.maybe_install_dependencies(
         parsed_args.skip_install, parsed_args.run_minified_tests)
 
@@ -174,21 +173,18 @@ def main(args=None):
     # info logs to stderr so we discard them.
     # TODO(jacob): Find a webdriver or selenium argument that controls log
     # level.
-    background_processes = []
-    background_processes.append(subprocess.Popen([
-        os.path.join(common.NODE_MODULES_PATH, '.bin', 'webdriver-manager'),
-        'start', '2>/dev/null']))
+    subprocess.Popen(
+        '(%s start 2>/dev/null)&'
+        % os.path.join(common.NODE_MODULES_PATH, '.bin', 'webdriver-manager'),
+        shell=True)
     # Start a demo server.
-    background_processes.append(subprocess.Popen(
-        'python %s/dev_appserver.py --host=0.0.0.0 --port=%s '
+    subprocess.Popen(
+        '(python %s/dev_appserver.py --host=0.0.0.0 --port=%s '
         '--clear_datastore=yes --dev_appserver_log_level=critical '
-        '--log_level=critical --skip_sdk_update_check=true %s' % (
+        '--log_level=critical --skip_sdk_update_check=true %s)&' % (
             common.GOOGLE_APP_ENGINE_HOME,
             python_utils.UNICODE(PORT_NUMBER_FOR_GAE_SERVER),
-            app_yaml_filepath), shell=True))
-
-    for process in background_processes:
-        process.wait()
+            app_yaml_filepath), shell=True)
 
     # Wait for the servers to come up.
     while not common.is_port_open(

@@ -29,14 +29,12 @@
 from . import setup_gae
 
 _PARSER = argparse.ArgumentParser(description="""
-    Run this script from the oppia root folder:
-
-        python -m scripts.run_frontend_tests
-
-    The root folder MUST be named 'oppia'.
-
-    Note: You can replace 'it' with 'fit' or 'describe' with 'fdescribe' to run
-    a single test or test suite.""")
+Run this script from the oppia root folder:
+    python -m scripts.run_frontend_tests
+The root folder MUST be named 'oppia'.
+Note: You can replace 'it' with 'fit' or 'describe' with 'fdescribe' to run
+a single test or test suite.
+""")
 
 _PARSER.add_argument(
     '--skip_install',
@@ -51,10 +49,11 @@
 
 def main(args=None):
     """Runs the frontend tests."""
+    parsed_args = _PARSER.parse_args(args=args)
+
     setup.main(args=[])
     setup_gae.main(args=[])
 
-    parsed_args = _PARSER.parse_args(args=args)
     install_third_party_libs.maybe_install_dependencies(
         parsed_args.skip_install, parsed_args.run_minified_tests)
     common.print_each_string_after_two_new_lines([