Test Improvements

Remove unmaintained flask testing library and improve tests speed by clearing
tables instead of dropping and recreating them between each test. These
changes also help in upgrading the other flask dependencies.

admin/sql/clear_tables.sql

@@ -0,0 +1,13 @@
+DELETE FROM comment_revision;
+DELETE FROM comment;
+DELETE FROM vote;
+DELETE FROM spam_report;
+DELETE FROM revision;
+DELETE FROM oauth_grant;
+DELETE FROM oauth_token;
+DELETE FROM oauth_client;
+DELETE FROM moderation_log;
+DELETE FROM review;
+DELETE FROM "user";
+DELETE FROM license;
+DELETE FROM avg_rating;

critiquebrainz/data/testing.py

@@ -2,7 +2,7 @@
 
 from flask_testing import TestCase
 
-from critiquebrainz.data.utils import create_all, drop_tables, drop_types
+from critiquebrainz.data.utils import create_all, drop_tables, drop_types, clear_tables
 from critiquebrainz.frontend import create_app
 
 
@@ -21,6 +21,4 @@ def tearDown(self):
 
     @staticmethod
     def reset_db():
-        drop_tables()
-        drop_types()
-        create_all()
+        clear_tables()

critiquebrainz/data/utils.py

@@ -30,6 +30,10 @@ def drop_types():
     db.run_sql_script(os.path.join(ADMIN_SQL_DIR, 'drop_types.sql'))
 
 
+def clear_tables():
+    db.run_sql_script(os.path.join(ADMIN_SQL_DIR, 'clear_tables.sql'))
+
+
 def explode_db_uri(uri):
     """Extracts database connection info from the URI.
 

critiquebrainz/db/users.py

@@ -4,7 +4,6 @@
 import sqlalchemy
 
 from critiquebrainz import db
-from critiquebrainz.db import revision as db_revision
 
 
 USER_GET_COLUMNS = [
@@ -305,6 +304,7 @@ def list_users(limit=None, offset=0):
         result = connection.execute(sqlalchemy.text("""
             SELECT {columns}
               FROM "user"
+          ORDER BY musicbrainz_row_id
              LIMIT :limit
             OFFSET :offset
         """.format(columns=','.join(USER_GET_COLUMNS))), {
@@ -357,6 +357,7 @@ def has_voted(user_id, review_id):
     Returns:
         (bool): True if has voted else False.
     """
+    from critiquebrainz.db import revision as db_revision
     last_revision = db_revision.get(review_id, limit=1)[0]
     with db.engine.connect() as connection:
         result = connection.execute(sqlalchemy.text("""

critiquebrainz/frontend/external/__init__.py

@@ -10,6 +10,7 @@
 
 from critiquebrainz.frontend.external.entities import get_entity_by_id, get_multiple_entities
 
+
 class MBDataAccess(object):
     """A data access object which switches between database get methods or development versions
     This is useful because we won't show a review if we cannot find its metadata in the
@@ -51,6 +52,7 @@ def get_multiple_entities(self):
                 ctx.get_multiple_entities = self.get_multiple_entities_method
             return ctx.get_multiple_entities
 
+
 mbstore = MBDataAccess()
 
 

critiquebrainz/frontend/testing.py

@@ -1,34 +1,17 @@
 import os
-from critiquebrainz.ws.oauth import oauth
-
-from flask_testing import TestCase
 
-from critiquebrainz.data.utils import create_all, drop_tables, drop_types
 from critiquebrainz.frontend import create_app
+from critiquebrainz.testing import ServerTestCase
+from critiquebrainz.ws.oauth import oauth
 
 
-class FrontendTestCase(TestCase):
+class FrontendTestCase(ServerTestCase):
 
-    def create_app(self):
-        app = create_app(config_path=os.path.join(os.path.dirname(os.path.realpath(__file__)), '..', '..', 'test_config.py'))
+    @classmethod
+    def create_app(cls):
+        app = create_app(
+            config_path=os.path.join(os.path.dirname(os.path.realpath(__file__)), '..', '..', 'test_config.py')
+        )
         oauth.init_app(app)
+        app.config['TESTING'] = True
         return app
-
-    def setUp(self):
-        self.reset_db()
-        # TODO(roman): Add stuff form fixtures.
-
-    def tearDown(self):
-        pass
-
-    @staticmethod
-    def reset_db():
-        drop_tables()
-        drop_types()
-        create_all()
-
-    def temporary_login(self, user):
-        """Based on: http://stackoverflow.com/a/16238537."""
-        with self.client.session_transaction() as session:
-            session['_user_id'] = user.id
-            session['_fresh'] = True

critiquebrainz/frontend/views/review.py

@@ -1,6 +1,5 @@
 from math import ceil
 
-from brainzutils.musicbrainz_db.exceptions import NoDataFoundException
 from flask import Blueprint, render_template, request, redirect, url_for, jsonify
 from flask_babel import gettext, get_locale, lazy_gettext
 from flask_login import login_required, current_user

critiquebrainz/frontend/views/test/test_artist.py

@@ -1,9 +1,9 @@
 from unittest import mock
 
-from critiquebrainz.db.user import User
 from critiquebrainz.frontend.testing import FrontendTestCase
 from critiquebrainz.db import users as db_users
 
+
 def return_release_groups(*, artist_id, release_types=None, limit=None, offset=None):
     # pylint: disable=unused-argument
     if release_types == ['ep']:
@@ -30,6 +30,7 @@ def return_release_groups(*, artist_id, release_types=None, limit=None, offset=N
 class ArtistViewsTestCase(FrontendTestCase):
 
     def setUp(self):
+        from critiquebrainz.db.user import User
         super(ArtistViewsTestCase, self).setUp()
         self.reviewer = User(db_users.get_or_create(
             1, "Reviewer", new_user_data={"display_name": u"Reviewer"}

critiquebrainz/frontend/views/test/test_oauth.py

@@ -9,6 +9,7 @@
 
 class OauthTestCase(FrontendTestCase):
     def setUp(self):
+        super().setUp()
         from critiquebrainz.db.user import User
         self.user = User(db_users.get_or_create(2, "9371e5c7-5995-4471-a5a9-33481f897f9c", new_user_data={
             "display_name": u"User",

critiquebrainz/frontend/views/test/test_review.py

@@ -2,7 +2,7 @@
 from unittest.mock import patch
 from urllib.parse import urlparse
 
-from flask import current_app, url_for
+from flask import url_for
 
 import critiquebrainz.db.license as db_license
 import critiquebrainz.db.review as db_review

critiquebrainz/frontend/views/test/test_user.py

@@ -12,7 +12,9 @@ def setUp(self):
         self.user = User(db_users.get_or_create(1, "Tester", new_user_data={
             "display_name": u"Tester",
         }))
-        self.hacker = User(db_users.create(musicbrainz_row_id = 2, display_name = u"Hacker"))
+        self.hacker = User(db_users.get_or_create(2, "Hacker", new_user_data={
+            "display_name": u"Hacker",
+        }))
         self.admin = User(db_users.get_or_create(3, "Admin", new_user_data={
             "display_name": u"Admin",
         }))

critiquebrainz/testing.py

@@ -0,0 +1,153 @@
+import unittest
+from urllib import parse
+
+from flask import template_rendered, message_flashed, g
+
+from critiquebrainz.data.utils import create_all, drop_tables, drop_types, clear_tables
+
+
+class ServerTestCase(unittest.TestCase):
+    """ TestCase for Flask App tests, most of the code here has been borrowed from flask_testing.TestCase
+    (https://github.com/jarus/flask-testing/blob/5107691011fa891835c01547e73e991c484fa07f/flask_testing/utils.py#L118).
+    A key difference is that flask_testing's TestCase creates the flask app for each test method whereas
+    our ServerTestCase creates it once per class. flask_testing's test case allows also for custom Response mixins
+    and other stuff to be compatible across flask versions. Since, we don't need that the implementation can be
+    simplified.
+
+    Notably, the implementation of the setUpClass, setUp, tearDown, tearDownClass, assertMessageFlashed,
+    assertTemplateUsed has been adapted to use class level variables. The implementation of assertRedirects has been
+    modified to fix concerning absolute and relative urls in Location header.
+    """
+
+    def reset_db(self):
+        clear_tables()
+
+    def temporary_login(self, user):
+        # flask-login stores the logged in user in the global g which lasts for the entire duration of a test
+        # (a test request context is pushed in setUp and popped in teardown). therefore, we need to remove
+        # the field manually for if multiple logins are needed in a test.
+        if hasattr(g, "_login_user"):
+            del g._login_user
+        with self.client.session_transaction() as session:
+            session['_user_id'] = user.id
+            session['_fresh'] = True
+
+    @classmethod
+    def setUpClass(cls):
+        cls.app = cls.create_app()
+        cls.client = cls.app.test_client()
+
+        template_rendered.connect(cls._set_template)
+        message_flashed.connect(cls._add_flash_message)
+
+    def setUp(self) -> None:
+        self._ctx = self.app.test_request_context()
+        self._ctx.push()
+
+        ServerTestCase.template = None
+        ServerTestCase.flashed_messages = []
+
+        self.reset_db()
+
+    @classmethod
+    def _add_flash_message(cls, app, message, category):
+        ServerTestCase.flashed_messages.append((message, category))
+
+    @classmethod
+    def _set_template(cls, app, template, context):
+        ServerTestCase.template = (template, context)
+
+    def tearDown(self):
+        self._ctx.pop()
+        del self._ctx
+
+    @classmethod
+    def tearDownClass(cls):
+        template_rendered.disconnect(cls._set_template)
+        message_flashed.disconnect(cls._add_flash_message)
+        del cls.client
+        del cls.app
+
+    def assertMessageFlashed(self, message, category='message'):
+        """
+        Checks if a given message was flashed.
+        Only works if your version of Flask has message_flashed
+        signal support (0.10+) and blinker is installed.
+        :param message: expected message
+        :param category: expected message category
+        """
+        for _message, _category in ServerTestCase.flashed_messages:
+            if _message == message and _category == category:
+                return True
+
+        raise AssertionError("Message '%s' in category '%s' wasn't flashed" % (message, category))
+
+    def assertTemplateUsed(self, name):
+        """
+        Checks if a given template is used in the request.
+        Only works if your version of Flask has signals
+        support (0.6+) and blinker is installed.
+        If the template engine used is not Jinja2, provide
+        ``tmpl_name_attribute`` with a value of its `Template`
+        class attribute name which contains the provided ``name`` value.
+        :versionadded: 0.2
+        :param name: template name
+        """
+        if ServerTestCase.template is None:
+            self.fail("No template used")
+        used_template = ServerTestCase.template[0].name
+        self.assertEqual(used_template, name, f"Template {name} not used. Template used: {used_template}")
+
+    def get_context_variable(self, name):
+        if ServerTestCase.template is None:
+            self.fail("No template used")
+        context = ServerTestCase.template[1]
+        if name in context:
+            return context[name]
+        raise ValueError()
+
+    def assertContext(self, name, value, message=None):
+        try:
+            self.assertEqual(self.get_context_variable(name), value, message)
+        except ValueError:
+            self.fail(message or "Context variable does not exist: %s" % name)
+
+    assert_context = assertContext
+
+    def assertRedirects(self, response, location, message=None, permanent=False):
+        if permanent:
+            valid_status_codes = (301, 308)
+        else:
+            valid_status_codes = (301, 302, 303, 305, 307, 308)
+
+        valid_status_code_str = ', '.join(str(code) for code in valid_status_codes)
+        not_redirect = f"HTTP Status {valid_status_code_str} expected but got {response.status_code}"
+
+        self.assertIn(response.status_code, valid_status_codes, message or not_redirect)
+
+        response_location = parse.unquote(response.location)
+        location_mismatch = f"Expected redirect location {location} but got {response_location}"
+        self.assertTrue(response_location.endswith(location), message or location_mismatch)
+
+    def assertStatus(self, response, status_code, message=None):
+        message = message or 'HTTP Status %s expected but got %s' \
+                             % (status_code, response.status_code)
+        self.assertEqual(response.status_code, status_code, message)
+
+    def assert200(self, response, message=None):
+        self.assertStatus(response, 200, message)
+
+    def assert400(self, response, message=None):
+        self.assertStatus(response, 400, message)
+
+    def assert401(self, response, message=None):
+        self.assertStatus(response, 401, message)
+
+    def assert403(self, response, message=None):
+        self.assertStatus(response, 403, message)
+
+    def assert404(self, response, message=None):
+        self.assertStatus(response, 404, message)
+
+    def assert500(self, response, message=None):
+        self.assertStatus(response, 500, message)

critiquebrainz/ws/__init__.py

@@ -86,6 +86,9 @@ def create_app(debug=None, config_path=None):
     from critiquebrainz.frontend import babel
     babel.init_app(app, domain='cb_webservice')
 
+    from critiquebrainz.frontend.external import mbstore
+    mbstore.init_app(app)
+
     # OAuth
     from critiquebrainz.ws.oauth import oauth
     oauth.init_app(app)

critiquebrainz/ws/testing.py

@@ -1,34 +1,22 @@
 import os
 
-from flask_testing import TestCase
-
 import critiquebrainz.db.oauth_client as db_oauth_client
 import critiquebrainz.db.users as db_users
-from critiquebrainz.data.utils import create_all, drop_tables, drop_types
+from critiquebrainz.testing import ServerTestCase
 from critiquebrainz.ws import create_app
 from critiquebrainz.ws.oauth import oauth
 
 
-class WebServiceTestCase(TestCase):
+class WebServiceTestCase(ServerTestCase):
 
-    def create_app(self):
-        app = create_app(config_path=os.path.join(os.path.dirname(os.path.realpath(__file__)), '..', '..', 'test_config.py'))
+    @classmethod
+    def create_app(cls):
+        app = create_app(
+            config_path=os.path.join(os.path.dirname(os.path.realpath(__file__)), '..', '..', 'test_config.py')
+        )
         oauth.init_app(app)
         return app
 
-    def setUp(self):
-        self.reset_db()
-        # TODO(roman): Add stuff form fixtures.
-
-    def tearDown(self):
-        pass
-
-    @staticmethod
-    def reset_db():
-        drop_tables()
-        drop_types()
-        create_all()
-
     @staticmethod
     def create_dummy_client(user):
         db_oauth_client.create(

requirements.txt

@@ -4,7 +4,6 @@ click==8.1.3
 Flask-Babel==2.0.0
 Flask-Login==0.6.0
 Flask-SQLAlchemy==2.5.1
-Flask-Testing==0.8.1
 Flask-WTF==1.0.1
 Markdown==3.3.6
 bleach==5.0.1