Closed regisb closed 3 years ago
@jmaupetit Is there a way for me to make sure that I am not breaking anything? I found it very difficult to run unit tests locally -- are they still run somewhere? AFAIU the tutor/envs/lms/unittests.py settings are not compatible with Docker.
That's part of our technical debt: we've never seen those tests succeed... :grimacing:
As removing the fun-apps
from our stack is in our top priority, we didn't put effort in maintaining this legacy/deprecated project/approach. Sorry about that.
Does this mean that new features added on top of fun-apps should not come with new unit tests?
It is that hard to make them run on Docker? What is the issue?
FYI I've unlocked the CI to at least run the build. You'll need to (force) push in this branch to trigger the CI.
It is that hard to make them run on Docker? What is the issue?
Currently, the issue is that the unittests.py
settings load the edx-platform dev.py settings, which rely on an lms.env.json file. I didn't try too hard to fix the settings and to make all tests pass; that's because I first wanted to know if tests had been maintained. If they haven't, then it's very likely that many of them are failing and that fixing them will require a lot of time. Also, it's useless to fix them if they aren't run periodically in CI.
it's very likely that many of them are failing and that fixing them will require a lot of time.
You don't know that until you run them :wink:
Also, it's useless to fix them if they aren't run periodically in CI.
That could be easily fixed :muscle:
it's very likely that many of them are failing and that fixing them will require a lot of time.
You don't know that until you run them
I have, and I do... The last times the unittests.py
setting modules were touched were in July 2019 and March 2018. Between 20 and 40 (ish) feature commits were pushed to fun-apps since then. I can agree to give it a try to fix unit tests, pro bono, but I don't want this PR to depend on it.
With the latest round of changes I have fairly high confidence that they work as expected. I managed to successfully run the unit tests for the test_payment module. To do so, I created docker_run_test.py
:
from .docker_run_development import *
from path import path
ENVIRONMENT = 'test'
############# Disable useless logging
import logging
logging.getLogger("backoffice.views").setLevel(logging.ERROR)
logging.getLogger("edxmako.shortcuts").setLevel(logging.ERROR)
logging.getLogger("openedx.core.djangoapps.content.course_overviews.models").setLevel(logging.ERROR)
logging.getLogger("py.warnings").setLevel(logging.ERROR)
logging.getLogger("elasticsearch").setLevel(logging.ERROR)
logging.getLogger("urllib3.connectionpool").setLevel(logging.ERROR)
logging.getLogger("payment_api.views").setLevel(logging.ERROR)
################ Microsite test settings
FAKE_MICROSITE = {
"domain_prefix": "testmicrosite",
"university": "test_microsite",
"platform_name": "Test Microsite",
"logo_image_url": "test_microsite/images/header-logo.png",
"email_from_address": "test_microsite@edx.org",
"payment_support_email": "test_microsite@edx.org",
"ENABLE_MKTG_SITE": False,
"SITE_NAME": "test_microsite.localhost",
"course_org_filter": "TestMicrositeX",
"course_about_show_social_links": False,
"css_overrides_file": "test_microsite/css/test_microsite.css",
"show_partners": False,
"show_homepage_promo_video": False,
"course_index_overlay_text": "This is a Test Microsite Overlay Text.",
"course_index_overlay_logo_file": "test_microsite/images/header-logo.png",
"homepage_overlay_html": "<h1>This is a Test Microsite Overlay HTML</h1>",
"ALWAYS_REDIRECT_HOMEPAGE_TO_DASHBOARD_FOR_AUTHENTICATED_USER": False,
"COURSE_CATALOG_VISIBILITY_PERMISSION": "see_in_catalog",
"COURSE_ABOUT_VISIBILITY_PERMISSION": "see_about_page",
"ENABLE_SHOPPING_CART": True,
"ENABLE_PAID_COURSE_REGISTRATION": True,
"SESSION_COOKIE_DOMAIN": "test_microsite.localhost",
}
MICROSITE_ROOT_DIR = path("/edx/app/edxapp/fun-microsites")
MICROSITE_TEST_HOSTNAME = 'testmicrosite.testserver'
############ If you modify settings below this line don't forget to modify them both in lms/test.py and cms/test.py
from fun.envs import test
TEST_RUNNER = 'django_nose.NoseTestSuiteRunner'
NOSE_ARGS = test.nose_args(REPO_ROOT, 'lms')
TEST_ROOT = path("test_root")
COMMON_TEST_DATA_ROOT = COMMON_ROOT / "test" / "data"
DATABASES = test.databases(TEST_ROOT)
################# mongodb
MONGO_PORT_NUM = int(os.environ.get('EDXAPP_TEST_MONGO_PORT', '27017'))
MONGO_HOST = os.environ.get('EDXAPP_TEST_MONGO_HOST', 'mongodb')
CONTENTSTORE = test.contentstore(MONGO_HOST, MONGO_PORT_NUM)
PASSWORD_HASHERS = test.password_hashers
STATICFILES_STORAGE = 'pipeline.storage.NonPackagingPipelineStorage'
PIPELINE_ENABLED = False
CACHES.update(test.caches)
################ Disable Django debug toolbar
DEBUG_TOOLBAR_INSTALLED_APPS = ('debug_toolbar',)
DEBUG_TOOLBAR_MIDDLEWARE_CLASSES = (
'django_comment_client.utils.QueryCountDebugMiddleware',
'debug_toolbar.middleware.DebugToolbarMiddleware',
)
INSTALLED_APPS = tuple([app for app in INSTALLED_APPS if app not in DEBUG_TOOLBAR_INSTALLED_APPS])
MIDDLEWARE_CLASSES = tuple([m for m in MIDDLEWARE_CLASSES if m not in DEBUG_TOOLBAR_MIDDLEWARE_CLASSES])
FEATURES['USE_MICROSITES'] = False
# Disable costly calls to publish signals
COURSE_SIGNALS_DISABLED = True
# From Django 1.7 `syndb` do not exists anymore then `migrate` is used to create db
# it makes test database creation very long.
# See: https://groups.google.com/d/msg/django-developers/PWPj3etj3-U/QTpjBvD2QMcJ
MIGRATION_MODULES = dict((app, '%s.fake_migrations' % app) for app in INSTALLED_APPS)
# Ecommerce credentials
ECOMMERCE_API_SIGNING_KEY = "test"
Then I added the dev-bash
target to the openedx-docker Makefile:
dev-bash:
$(COMPOSE_RUN) --no-deps -v $(PWD)/$(FLAVORED_EDX_RELEASE_PATH)/src/fun-apps:/edx/app/edxapp/fun-apps lms-dev bash
I then run the lms-dev container with make dev-bash
. Install fun-apps in editable mode:
pip install -e ../fun-apps/
Run the unit tests:
./manage.py lms --settings=fun.docker_run_test test -s payment.tests.test_payment
...
26 tests run in 45.5 seconds (26 tests passed)
I believe this should be ready to merge.
Looks like we are good to go now ! Cheers @regisb and thanks for accompanying us @sampaccoud / @jmaupetit . The reviews were very useful and I can see the improvement. Any chance this can now be reviewed, merged and pushed onto prod ?
Looks like we are good to go now ! Cheers @regisb and thanks for accompanying us @sampaccoud / @jmaupetit . The reviews were very useful and I can see the improvement. Any chance this can now be reviewed, merged and pushed onto prod ?
Yes. One last comment. Make sure to ping me via review assignment when you need further review.
Please review again @sampaccoud.
The goal of this PR is to pick up from #694 and to address comments. This is a work-in-progress designed to trigger CI.