Merge pull request #2446 from Open-MSS/merge_stable_to_develop

Merge stable to develop

mslib/mscolab/mscolab.py

@@ -45,15 +45,13 @@
 
 
 def handle_start(args):
-    from mslib.mscolab.server import APP, initialize_managers, start_server
+    from mslib.mscolab.server import APP, sockio, cm, fm, start_server
     setup_logging(args)
     logging.info("MSS Version: %s", __version__)
     logging.info("Python Version: %s", sys.version)
     logging.info("Platform: %s (%s)", platform.platform(), platform.architecture())
     logging.info("Launching MSColab Server")
-
-    app, sockio, cm, fm = initialize_managers(APP)
-    start_server(app, sockio, cm, fm)
+    start_server(APP, sockio, cm, fm)
 
 
 def confirm_action(confirmation_prompt):

mslib/mscolab/server.py

@@ -48,7 +48,7 @@
 
 from mslib.mscolab.conf import mscolab_settings, setup_saml2_backend
 from mslib.mscolab.models import Change, MessageType, User
-from mslib.mscolab.sockets_manager import setup_managers
+from mslib.mscolab.sockets_manager import _setup_managers
 from mslib.mscolab.utils import create_files, get_message_dict
 from mslib.utils import conditional_decorator
 from mslib.index import create_app
@@ -126,16 +126,16 @@ def confirm_token(token, expiration=3600):
     return email
 
 
-def initialize_managers(app):
-    sockio, cm, fm = setup_managers(app)
+def _initialize_managers(app):
+    sockio, cm, fm = _setup_managers(app)
     # initializing socketio and db
     app.wsgi_app = socketio.Middleware(socketio.server, app.wsgi_app)
     sockio.init_app(app)
     # db.init_app(app)
     return app, sockio, cm, fm
 
 
-_app, sockio, cm, fm = initialize_managers(APP)
+_app, sockio, cm, fm = _initialize_managers(APP)
 
 
 def check_login(emailid, password):

mslib/mscolab/sockets_manager.py

@@ -262,7 +262,7 @@ def emit_operation_delete(self, op_id):
         socketio.emit("operation-deleted", json.dumps({"op_id": op_id}))
 
 
-def setup_managers(app):
+def _setup_managers(app):
     """
     takes app as parameter to extract config data,
     initializes ChatManager, FileManager, SocketManager and return them

tests/_test_mscolab/test_server.py

@@ -30,7 +30,7 @@
 
 from mslib.mscolab.conf import mscolab_settings
 from mslib.mscolab.models import User, Operation
-from mslib.mscolab.server import initialize_managers, check_login, register_user
+from mslib.mscolab.server import check_login, register_user
 from mslib.mscolab.file_manager import FileManager
 from mslib.mscolab.seed import add_user, get_user
 
@@ -43,9 +43,9 @@ def setup(self, mscolab_app):
         with self.app.app_context():
             yield
 
-    def test_initialize_managers(self):
-        app, sockio, cm, fm = initialize_managers(self.app)
-        assert app.config['MSCOLAB_DATA_DIR'] == mscolab_settings.MSCOLAB_DATA_DIR
+    def test_initialized_managers(self, mscolab_managers):
+        sockio, cm, fm = mscolab_managers
+        assert self.app.config['MSCOLAB_DATA_DIR'] == mscolab_settings.MSCOLAB_DATA_DIR
         assert 'Create a Flask-SocketIO server.' in sockio.__doc__
         assert 'Class with handler functions for chat related functionalities' in cm.__doc__
         assert 'Class with handler functions for file related functionalities' in fm.__doc__

tests/_test_mscolab/test_sockets_manager.py

@@ -26,11 +26,7 @@
 """
 import os
 import pytest
-import socket
-import socketio
 import datetime
-import requests
-from urllib.parse import urljoin, urlparse
 
 from mslib.msui.icons import icons
 from mslib.mscolab.conf import mscolab_settings
@@ -41,10 +37,10 @@
 
 class Test_Socket_Manager:
     @pytest.fixture(autouse=True)
-    def setup(self, mscolab_app, mscolab_managers, mscolab_server):
+    def setup(self, mscolab_app, mscolab_managers):
         self.app = mscolab_app
-        _, self.cm, self.fm = mscolab_managers
-        self.url = mscolab_server
+        self.sockio, self.cm, self.fm = mscolab_managers
+        self.sm = self.sockio.sm
         self.sockets = []
         self.userdata = 'UV10@uv10', 'UV10', 'uv10'
         self.anotheruserdata = 'UV20@uv20', 'UV20', 'uv20'
@@ -57,28 +53,13 @@ def setup(self, mscolab_app, mscolab_managers, mscolab_server):
         self.anotheruser = get_user(self.anotheruserdata[0])
         self.token = self.user.generate_auth_token()
         self.operation = get_operation(self.operation_name)
-        self.sm = SocketsManager(self.cm, self.fm)
         yield
         for sock in self.sockets:
             sock.disconnect()
 
-    def _can_ping_server(self):
-        parsed_url = urlparse(self.url)
-        host, port = parsed_url.hostname, parsed_url.port
-        try:
-            sock = socket.create_connection((host, port))
-            success = True
-        except socket.error:
-            success = False
-        finally:
-            sock.close()
-        return success
-
     def _connect(self):
-        sio = socketio.Client(reconnection_attempts=5)
+        sio = self.sockio.test_client(self.app)
         self.sockets.append(sio)
-        assert self._can_ping_server()
-        sio.connect(self.url, transports='polling')
         sio.emit('connect')
         return sio
 
@@ -88,13 +69,8 @@ def _new_operation(self, operation_name, description):
         return operation
 
     def test_handle_connect(self):
-        sio = socketio.Client()
-        assert sio.sid is None
-        self.sockets.append(sio)
-        assert self._can_ping_server()
-        sio.connect(self.url, transports='polling')
-        sio.emit('connect')
-        assert len(sio.sid) > 5
+        sio = self._connect()
+        assert len(sio.eio_sid) > 5
 
     def test_join_creator_to_operatiom(self):
         sio = self._connect()
@@ -122,14 +98,12 @@ def test_join_collaborator_to_operation(self):
 
     def test_remove_collaborator_from_operation(self):
         pytest.skip("get_session_id has None result")
-        sio = self._connect()
         operation = self._new_operation('new_operation', "example description")
         sm = SocketsManager(self.cm, self.fm)
         sm.join_collaborator_to_operation(self.anotheruser.id, operation.id)
         perms = Permission(self.anotheruser.id, operation.id, "collaborator")
         assert perms is not None
         sm.remove_collaborator_from_operation(self.anotheruser.id, operation.id)
-        sio.sleep(1)
         perms = Permission(self.anotheruser.id, operation.id, "collaborator")
         assert perms is None
 
@@ -158,7 +132,6 @@ def test_send_message(self):
             "message_text": "® non ascii",
             "reply_id": -1
         })
-        sio.sleep(1)
 
         with self.app.app_context():
             message = Message.query.filter_by(text="message from 1").first()
@@ -185,7 +158,6 @@ def test_get_messages(self):
             "message_text": "message from 1",
             "reply_id": -1
         })
-        sio.sleep(5)
         with self.app.app_context():
             messages = self.cm.get_messages(1)
             assert messages[0]["text"] == "message from 1"
@@ -216,23 +188,21 @@ def test_get_messages_api(self):
             "message_text": "message from 1",
             "reply_id": -1
         })
-        sio.sleep(1)
 
         token = self.token
         data = {
             "token": token,
             "op_id": self.operation.id,
             "timestamp": datetime.datetime(1970, 1, 1, tzinfo=datetime.timezone.utc).isoformat()
         }
-        # returns an array of messages
-        url = urljoin(self.url, 'messages')
-        res = requests.get(url, data=data, timeout=(2, 10)).json()
-        assert len(res["messages"]) == 2
+        with self.app.test_client() as c:
+            res = c.get("/messages", data=data)
+            assert len(res.json["messages"]) == 2
 
-        data["token"] = "dummy"
-        # returns False due to bad authorization
-        r = requests.get(url, data=data, timeout=(2, 10))
-        assert r.text == "False"
+            data["token"] = "dummy"
+            # returns False due to bad authorization
+            r = c.get("/messages", data=data)
+            assert r.text == "False"
 
     def test_edit_message(self):
         sio = self._connect()
@@ -244,7 +214,6 @@ def test_edit_message(self):
             "message_text": "Edit this message",
             "reply_id": -1
         })
-        sio.sleep(1)
         with self.app.app_context():
             message = Message.query.filter_by(text="Edit this message").first()
         sio.emit('edit-message', {
@@ -253,16 +222,14 @@ def test_edit_message(self):
             "op_id": message.op_id,
             "token": self.token
         })
-        sio.sleep(1)
         token = self.token
         data = {
             "token": token,
             "op_id": self.operation.id,
             "timestamp": datetime.datetime(1970, 1, 1, tzinfo=datetime.timezone.utc).isoformat()
         }
-        # returns an array of messages
-        url = urljoin(self.url, 'messages')
-        res = requests.get(url, data=data, timeout=(2, 10)).json()
+        with self.app.test_client() as c:
+            res = c.get("messages", data=data).json
         assert len(res["messages"]) == 1
         messages = res["messages"][0]
         assert messages["text"] == "I have updated the message"
@@ -277,30 +244,29 @@ def test_delete_message(self):
             "message_text": "delete this message",
             "reply_id": -1
         })
-        sio.sleep(1)
+
         with self.app.app_context():
             message = Message.query.filter_by(text="delete this message").first()
         sio.emit('delete-message', {
             'message_id': message.id,
             'op_id': self.operation.id,
             'token': self.token
         })
-        sio.sleep(1)
 
         with self.app.app_context():
             assert Message.query.filter_by(text="delete this message").count() == 0
 
     def test_upload_file(self):
         sio = self._connect()
         sio.emit('start', {'token': self.token})
-        files = {'file': open(icons('16x16'), 'rb')}
         data = {
             "token": self.token,
             "op_id": self.operation.id,
-            "message_type": int(MessageType.IMAGE)
+            "message_type": int(MessageType.IMAGE),
+            "file": open(icons('16x16'), 'rb'),
         }
-        url = urljoin(self.url, 'message_attachment')
-        requests.post(url, data=data, files=files, timeout=(2, 10))
+        with self.app.test_client() as c:
+            c.post("message_attachment", data=data, content_type="multipart/form-data")
         upload_dir = os.path.join(mscolab_settings.UPLOAD_FOLDER, str(self.user.id))
         assert os.path.exists(upload_dir)
         file = os.listdir(upload_dir)[0]

tests/fixtures.py

@@ -28,14 +28,15 @@
 import multiprocessing
 import time
 import urllib
+import socketio
 import mslib.mswms.mswms
 import eventlet
 import eventlet.wsgi
 
 from PyQt5 import QtWidgets
 from contextlib import contextmanager
 from mslib.mscolab.conf import mscolab_settings
-from mslib.mscolab.server import APP, initialize_managers
+from mslib.mscolab.server import APP, sockio, cm, fm
 from mslib.mscolab.mscolab import handle_db_init, handle_db_reset
 from mslib.utils.config import modify_config_file
 from tests.utils import is_url_response_ok
@@ -104,17 +105,29 @@ def mscolab_session_managers(mscolab_session_app):
     This fixture should not be used in tests. Instead use :func:`mscolab_managers`,
     which handles per-test cleanup as well.
     """
-    return initialize_managers(mscolab_session_app)[1:]
+    return sockio, cm, fm
 
 
-@pytest.fixture(scope="session")
+# TODO: Having this fixture be autouse is a crutch. It seems like if it is not autouse some tests can bring the pytest
+# processes objects into a state in which the MSColab server will have trouble starting the Flask-SocketIO server once
+# it is forked. With autouse the fork happens first, before any test runs. After that, the pytest process can no longer
+# affect the now-running server, thus mitigating the issue. This is my understanding at time of writing.
+#
+# This issue would also be avoided if the background server process wasn't started with multiprocessing and a fork, but
+# with a real subprocess, which would solve some other issues (e.g. testing on Windows) as well.
+@pytest.fixture(scope="session", autouse=True)
 def mscolab_session_server(mscolab_session_app, mscolab_session_managers):
     """Session-scoped fixture that provides a running MSColab server.
 
     This fixture should not be used in tests. Instead use :func:`mscolab_server`, which
     handles per-test cleanup as well.
     """
     with _running_eventlet_server(mscolab_session_app) as url:
+        # Wait until the Flask-SocketIO server is ready for connections
+        sio = socketio.Client()
+        sio.connect(url, retry=True)
+        sio.disconnect()
+        del sio
         yield url
 
 
@@ -141,8 +154,7 @@ def mscolab_app(mscolab_session_app, reset_mscolab):
 def mscolab_managers(mscolab_session_managers, reset_mscolab):
     """Fixture that provides the MSColab managers and does cleanup actions.
 
-    :returns: A tuple (SocketIO, ChatManager, FileManager) as returned by
-        initialize_managers.
+    :returns: A tuple (SocketIO, ChatManager, FileManager).
     """
     return mscolab_session_managers