Make the backend test for scripts/start.py 100 times faster. (oppia#23968)

* Update start scripts

* More fixes to main scripts

* Rewrite tests.

* Clean up

* Drop conditional startup based on EMULATOR_MODE

* Final fixes

* Simplify

* Import order

* fix mypy

* Address comments

* Remove skip-install from frontend tests; we always skip now.

* Add coverage

* fixes

* Fix arg

Author: seanlip

core/feconf.py

@@ -490,6 +490,9 @@ def get_empty_ratings() -> Dict[str, int]:
 # Valid Mailchimp tags.
 VALID_MAILCHIMP_TAGS = ['Account', 'Android', 'Web']
 
+GAE_DEVELOPMENT_SERVER_PORT = 8181
+GAE_ADMIN_SERVER_PORT = 8000
+
 ES_HOST = os.environ.get('ES_HOST', 'localhost')
 ES_LOCALHOST_PORT = 9200
 # NOTE TO RELEASE COORDINATORS: Replace this with the correct ElasticSearch

core/tests/test_utils.py

@@ -2127,7 +2127,7 @@ def setUp(self) -> None:
         storage_services.CLIENT.namespace = self.id()
         # Set up apps for testing.
         self.testapp = webtest.TestApp(main.app_without_context)
-        # Mock set_constans_to_default method to throw an exception.
+        # Mock set_constants_to_default method to throw an exception.
         # Don't directly change constants file in the test.
         # Mock this method again in your test.
         self.contextManager = self.swap(
@@ -2219,11 +2219,16 @@ def get_pending_tasks(
         )
 
     def mock_set_constants_to_default(self) -> None:
-        """Change constants file in the test could lead to other
-        tests fail. Mock set_constants_to_default method in your test
-        will suppress this exception.
+        """Mock implementation of set_constants_to_default for tests.
+
+        Directly changing the shared constants file in your test can cause
+        problems. Mocking common.set_constants_to_default() in your test will
+        suppress this exception.
         """
-        raise Exception('Please mock this method in the test.')
+        raise Exception(
+            'Tests should mock common.set_constants_to_default() to avoid '
+            'modifying the constants file during tests.'
+        )
 
     @contextlib.contextmanager
     def mock_datetime_utcnow(

core/tests/test_utils_test.py

@@ -864,6 +864,15 @@ def test_set_translation_coordinators(self) -> None:
         self.assertEqual('en', coordinator_rights_model[0].language_id)
         self.assertEqual('hi', coordinator_rights_model[1].language_id)
 
+    def test_mock_set_constants_to_default_raises_exception(self) -> None:
+        """Test that mock_set_constants_to_default raises an exception."""
+        with self.assertRaisesRegex(
+            Exception,
+            r'Tests should mock common\.set_constants_to_default\(\) to avoid '
+            r'modifying the constants file during tests\.',
+        ):
+            self.mock_set_constants_to_default()
+
 
 class CheckImagePngOrWebpTests(test_utils.GenericTestBase):
 

scripts/common.py

@@ -490,6 +490,22 @@ def is_port_in_use(port: int) -> bool:
         return bool(not s.connect_ex(('localhost', port)))
 
 
+def get_ports_in_use(ports: list[int]) -> list[int]:
+    """Checks which ports from a list are currently in use.
+
+    Args:
+        ports: list[int]. List of port numbers.
+
+    Returns:
+        list[int]. List of port numbers that are currently in use.
+    """
+    in_use = []
+    for port in ports:
+        if is_port_in_use(port):
+            in_use.append(port)
+    return in_use
+
+
 def recursive_chown(path: str, uid: int, gid: int) -> None:
     """Changes the owner and group id of all files in a path to the numeric
     uid and gid.

scripts/common_test.py

@@ -642,6 +642,24 @@ def test_is_port_in_use(self) -> None:
             self.assertTrue(common.is_port_in_use(port))
         self.assertFalse(common.is_port_in_use(port))
 
+    def test_get_ports_in_use(self) -> None:
+        with self.open_tcp_server_port() as port_in_use:
+            # Test with one port in use and one not in use.
+            ports = [port_in_use, port_in_use + 1]
+            in_use = common.get_ports_in_use(ports)
+            self.assertEqual(in_use, [port_in_use])
+
+            # Test with no ports in use.
+            ports_not_in_use = [port_in_use + 1, port_in_use + 2]
+            in_use_empty = common.get_ports_in_use(ports_not_in_use)
+            self.assertEqual(in_use_empty, [])
+
+            # Test with all ports in use.
+            with self.open_tcp_server_port() as port_in_use2:
+                ports_all_in_use = [port_in_use, port_in_use2]
+                in_use_all = common.get_ports_in_use(ports_all_in_use)
+                self.assertEqual(set(in_use_all), set(ports_all_in_use))
+
     def test_wait_for_port_to_not_be_in_use_port_never_closes(self) -> None:
         def mock_sleep(unused_seconds: int) -> None:
             return

scripts/extend_index_yaml.py

@@ -13,8 +13,6 @@
 # limitations under the License.
 
 """Script for extending index.yaml.
-In Python 2, the index.yaml autoamtically extended with # AUTOGENERATED,
-but this is no longer true in Python 3.
 
 This script extracts new kind from
 ../cloud_datastore_emulator_cache/WEB-INF/index.yaml
@@ -87,21 +85,25 @@ def reformat_xml_dict_into_yaml_dict(
 
 def main() -> None:
     """Extends index.yaml file."""
-    with open(INDEX_YAML_PATH, 'r', encoding='utf-8') as f:
-        index_yaml_dict = yaml.safe_load(f)
+    if not os.path.exists(WEB_INF_INDEX_XML_PATH):
+        print('No new index definitions were created during this server run.')
+        return
 
     with open(WEB_INF_INDEX_XML_PATH, 'r', encoding='utf-8') as f:
         web_inf_index_xml_dict = xmltodict.parse(
             f.read(), force_list={'datastore-index', 'property'}
         )
-
     web_inf_index_yaml_dict = reformat_xml_dict_into_yaml_dict(
         web_inf_index_xml_dict
     )
-
     if web_inf_index_yaml_dict is None:
         return
 
+    print('\033[94mExtending index.yaml...\033[0m')
+
+    with open(INDEX_YAML_PATH, 'r', encoding='utf-8') as f:
+        index_yaml_dict = yaml.safe_load(f)
+
     new_kinds = [
         kind
         for kind in web_inf_index_yaml_dict['indexes']
@@ -121,3 +123,5 @@ def main() -> None:
     index_yaml = new_index_yaml_dict.replace('- kind', '\n- kind')
     with open(INDEX_YAML_PATH, 'w', encoding='utf-8') as f:
         f.write(index_yaml)
+
+    print('\033[92mDone!\033[0m')

scripts/extend_index_yaml_test.py

@@ -18,6 +18,7 @@
 
 from __future__ import annotations
 
+import os
 import tempfile
 
 from core.tests import test_utils
@@ -470,3 +471,37 @@ def test_extend_index_yaml_with_same_kind_different_order(self) -> None:
         self._run_test_for_extend_index_yaml(
             index_yaml, web_inf_index_xml, expected_index_yaml
         )
+
+    def test_main_exits_early_when_web_inf_index_xml_does_not_exist(
+        self,
+    ) -> None:
+        # Create a swap that points to a non-existent file.
+        non_existent_file = '/tmp/non_existent_web_inf_index_xml.xml'
+        web_inf_index_xml_swap = self.swap(
+            extend_index_yaml, 'WEB_INF_INDEX_XML_PATH', non_existent_file
+        )
+
+        with self.index_yaml_swap, web_inf_index_xml_swap:
+            self.assertFalse(
+                os.path.exists(extend_index_yaml.WEB_INF_INDEX_XML_PATH)
+            )
+
+            # Record the initial state of the index yaml file.
+            initial_content = ''
+            with open(
+                extend_index_yaml.INDEX_YAML_PATH, 'r', encoding='utf-8'
+            ) as f:
+                initial_content = f.read()
+
+            extend_index_yaml.main()
+
+            # Assert that no file writes were done to index.yaml.
+            with open(
+                extend_index_yaml.INDEX_YAML_PATH, 'r', encoding='utf-8'
+            ) as f:
+                final_content = f.read()
+            self.assertEqual(
+                final_content,
+                initial_content,
+                'INDEX_YAML_PATH content changed unexpectedly',
+            )

scripts/install_third_party_libs.py

@@ -36,28 +36,17 @@
 import sys
 import tarfile
 
-from scripts import (  # pylint: disable=wrong-import-position, wrong-import-order
-    install_python_dev_dependencies,
+from scripts import (
+    install_python_dev_dependencies,  # pylint: disable=wrong-import-position, wrong-import-order
 )
-
-from typing import Final
-
-install_python_dev_dependencies.main(['--assert_compiled'])
-
-from core import (  # pylint: disable=wrong-import-position, wrong-import-order
-    utils,
-)
-from scripts import (  # pylint: disable=wrong-import-position, wrong-import-order
+from scripts import (
     install_dependencies_json_packages,
     install_python_prod_dependencies,
 )
 
-from . import (  # pylint: disable=wrong-import-position, wrong-import-order
-    clean,
-    common,
-    pre_commit_hook,
-    pre_push_hook,
-)
+from typing import Final
+
+from . import clean, common
 
 # Place to download zip files for temporary storage.
 TMP_UNZIP_PATH: Final = os.path.join('.', 'tmp_unzip.zip')
@@ -84,7 +73,9 @@ def make_google_module_importable_by_python(google_module_path: str) -> None:
     for path_list in os.walk(google_module_path):
         root_path = path_list[0]
         if not root_path.endswith('__pycache__'):
-            with utils.open_file(os.path.join(root_path, '__init__.py'), 'a'):
+            with open(
+                os.path.join(root_path, '__init__.py'), 'a', encoding='utf-8'
+            ):
                 # If the file doesn't exist, it is created. If it does exist,
                 # this open does nothing.
                 pass
@@ -440,6 +431,14 @@ def main() -> None:
     """Set up GAE and install third-party libraries for Oppia."""
     print('Running install_third_party_libs script...')
 
+    # This ensures dev dependencies are present and compiled before we
+    # proceed to other setup tasks that require them.
+    install_python_dev_dependencies.main(['--assert_compiled'])
+    # Import the hook scripts here (after dev deps are installed) so that
+    # they are only loaded when running the installer.
+    from . import pre_commit_hook  # pylint: disable=wrong-import-position
+    from . import pre_push_hook  # pylint: disable=wrong-import-position
+
     if common.is_windows_os():
         raise Exception(
             'Installation of Oppia is not supported on Windows OS. Please use '

scripts/install_third_party_libs_test.py

@@ -25,10 +25,11 @@
 import subprocess
 import sys
 import tarfile
+import types
 
 from core.tests import test_utils
 
-from typing import Final, List, Tuple
+from typing import Final, List, Optional, Tuple, Type
 
 from . import (
     clean,
@@ -74,11 +75,38 @@ def __init__(
     ) -> None:
         self.returncode = returncode
         self.communicate_val = communicate_val
+        self.stdout = None
+        self.args: List[str] = []
 
-    def communicate(self) -> Tuple[bytes, bytes]:
+    def communicate(
+        self,
+        _input: Optional[bytes] = None,  # pylint: disable=unused-argument
+        timeout: Optional[float] = None,  # pylint: disable=unused-argument
+    ) -> Tuple[bytes, bytes]:
         """Return required method."""
         return self.communicate_val
 
+    def kill(self) -> None:
+        """Mock kill method."""
+        pass
+
+    def poll(self) -> int:
+        """Mock poll method."""
+        return self.returncode
+
+    def __enter__(self) -> 'Ret':
+        """Context manager enter."""
+        return self
+
+    def __exit__(
+        self,
+        _exc_type: Optional[Type[BaseException]],
+        _exc_val: Optional[BaseException],
+        _exc_tb: Optional[types.TracebackType],
+    ) -> None:
+        """Context manager exit."""
+        pass
+
 
 class InstallThirdPartyLibsTests(test_utils.GenericTestBase):
     """Test the methods for installing third party libs."""
@@ -113,7 +141,11 @@ def mock_print(msg: str) -> None:
         self.check_call_swap = self.swap(
             subprocess, 'check_call', mock_check_call
         )
-        self.Popen_swap = self.swap(subprocess, 'Popen', mock_check_call)
+
+        def mock_popen(*_args: str, **_kwargs: str) -> Ret:
+            return Ret()
+
+        self.Popen_swap = self.swap(subprocess, 'Popen', mock_popen)
         self.check_call_error_swap = self.swap(
             subprocess, 'check_call', mock_check_call_error
         )

scripts/servers.py

@@ -162,7 +162,7 @@ def managed_dev_appserver(
     host: str = '0.0.0.0',
     port: int = 8080,
     admin_host: str = '0.0.0.0',
-    admin_port: int = 8000,
+    admin_port: int = feconf.GAE_ADMIN_SERVER_PORT,
     enable_host_checking: bool = True,
     automatic_restart: bool = False,
     skip_sdk_update_check: bool = False,