Add test for KOLIBRI_LISTEN_PORT and some start/stop functionality le…

…arningequality#1629

kolibri/utils/cli.py

@@ -229,19 +229,17 @@ def start(port=None, daemon=True):
     update()
 
     if port is None:
-        port = os.environ['KOLIBRI_LISTEN_PORT']
+        try:
+            port = int(os.environ['KOLIBRI_LISTEN_PORT'])
+        except ValueError:
+            logger.error("Invalid KOLIBRI_LISTEN_PORT, must be an integer")
+            raise
 
     if not daemon:
         logger.info("Running 'kolibri start' in foreground...")
     else:
         logger.info("Running 'kolibri start' as daemon (system service)")
 
-    # TODO: moved from server.start() but not sure where it should ideally be
-    # located. Question is if it should be run every time the server is started
-    # or if it depends on some kind of state change.
-    from kolibri.content.utils.annotation import update_channel_metadata_cache
-    update_channel_metadata_cache()
-
     # Daemonize at this point, no more user output is needed
     if daemon:
 
@@ -258,7 +256,7 @@ def start(port=None, daemon=True):
     server.start(port=port)
 
 
-def stop():
+def stop(sys_exit=True):
     """
     Stops the server unless it isn't running
     """
@@ -293,7 +291,8 @@ def stop():
 
     if stopped:
         logger.info("Server stopped")
-        sys.exit(0)
+        if sys_exit:
+            sys.exit(0)
 
 
 def status():
@@ -552,7 +551,8 @@ def main(args=None):
         return
 
     if arguments['start']:
-        port = int(arguments['--port']) or None
+        port = arguments['--port']
+        port = int(port) if port else None
         start(port, daemon=not arguments['--foreground'])
         return
 

kolibri/utils/server.py

@@ -61,6 +61,12 @@ def start(port=8080):
     with open(PID_FILE, 'w') as f:
         f.write("%d\n%d" % (os.getpid(), port))
 
+    # TODO: moved from server.start() but not sure where it should ideally be
+    # located. Question is if it should be run every time the server is started
+    # or if it depends on some kind of state change.
+    from kolibri.content.utils.annotation import update_channel_metadata_cache
+    update_channel_metadata_cache()
+
     def rm_pid_file():
         os.unlink(PID_FILE)
 
@@ -149,15 +155,17 @@ def _read_pid_file(filename):
                           if it exists. If the port number doesn't exist, then
                           port is None.
     """
-    try:
-        pid, port = open(filename, "r").readlines()
+    pid_file_lines = open(filename, "r").readlines()
+
+    if len(pid_file_lines) == 2:
+        pid, port = pid_file_lines
         pid, port = int(pid), int(port)
-    except ValueError:
+    elif len(pid_file_lines) == 1:
         # The file only had one line
-        try:
-            pid, port = int(open(filename, "r").read()), None
-        except ValueError:
-            pid, port = None, None
+        pid, port = int(pid_file_lines[0]), None
+    else:
+        raise ValueError("PID file must have 1 or two lines")
+
     return pid, port
 
 
@@ -198,8 +206,8 @@ def get_status():  # noqa: max-complexity=16
                 # It's dead so assuming the startup went badly
                 else:
                     raise NotRunning(STATUS_FAILED_TO_START)
-            # Couldn't parse to int
-            except TypeError:
+            # Couldn't parse to int or empty PID file
+            except (TypeError, ValueError):
                 raise NotRunning(STATUS_STOPPED)
         raise NotRunning(STATUS_STOPPED)  # Stopped
 
@@ -210,7 +218,7 @@ def get_status():  # noqa: max-complexity=16
         raise NotRunning(STATUS_PID_FILE_INVALID)  # Invalid PID file
 
     # PID file exists, but process is dead
-    if not pid_exists(pid):
+    if pid is None or not pid_exists(pid):
         if os.path.isfile(STARTUP_LOCK):
             raise NotRunning(STATUS_FAILED_TO_START)  # Failed to start
         raise NotRunning(STATUS_UNCLEAN_SHUTDOWN)  # Unclean shutdown

kolibri/utils/tests/test_cli.py

@@ -6,6 +6,7 @@
 import copy
 import logging
 import pytest
+import os
 
 from kolibri.utils import cli
 
@@ -109,3 +110,26 @@ def test_parsing(self):
                 assert docopt[k] == v
 
             assert django == django_expected
+
+    def test_kolibri_listen_port_env(self):
+        """
+        Starts and stops the server, mocking the actual server.start()
+        Checks that the correct fallback port is used from the environment.
+        """
+        test_port = 1234
+        # ENV VARS are always a string
+        os.environ['KOLIBRI_LISTEN_PORT'] = str(test_port)
+
+        def start_mock(port, *args, **kwargs):
+            assert port == test_port
+
+        from kolibri.utils import server
+
+        orig_start = server.start
+
+        try:
+            server.start = start_mock
+            cli.start(daemon=False)
+            cli.stop(sys_exit=False)
+        finally:
+            server.start = orig_start