From b8a34f0c57accfe8a67e6a97863d13fab295b5fc Mon Sep 17 00:00:00 2001 From: Tim McCormack Date: Mon, 12 Dec 2022 21:45:15 +0000 Subject: [PATCH] build: Set Django version for tests more safely; drop support for non-GHA (#31387) We have a need to lock the version of Django for production and tests, but also to test on newer versions of Django so that we can get the repo ready for long-term-support releases. We've been doing that by extracting the `django==x.y.z` from the pip-compiled files and moving it to a django.txt that is then co-installed but can be overridden during tests. The problem is that this can result in broken packages. The approach here is to have `make test-requirements` continue to ensure a consistent set of packages, and then install a different Django on top of that in the CI script -- and call `pip check` to make sure that combination isn't broken. Adding Django 4.0 to the unit-tests.yml matrix will now correctly result in this error and a failing job: `django-splash 1.2.1 has requirement Django<4.0, but you have django 4.0.8.` The other half of this is to change other CI runners to remove their ability to control the Django version, since it's complicated to make this work, and we probably only need it in unit-tests.yml. Convert them to just use `make test-requirements`. Also: - Simplify handling of `pip --src` by setting `PIP_SRC` (rather than our own `PIP_SRC_DIR`, which pip ignores because `--src-dir` isn't an option that it knows). This is needed to allow `make test-requirements` to do the pip calls. An alternative would be to set a pip-options env var for the make target to use, but `PIP_SRC` already exists. - Remove outdated modifications to common_constraints - Add comment explaining why pylint tests need dev-requirements --- .github/workflows/pylint-checks.yml | 4 ++++ .github/workflows/quality-checks.yml | 9 +++----- .github/workflows/unit-tests-gh-hosted.yml | 26 +++++++++++++--------- .github/workflows/unit-tests.yml | 5 ++--- Makefile | 11 --------- pavelib/prereqs.py | 14 ++++-------- pavelib/tests.py | 13 ----------- pavelib/utils/envs.py | 5 +++-- pavelib/utils/test/suites/pytest_suite.py | 21 ++++------------- requirements/edx/django.txt | 1 - requirements/edx/testing.txt | 1 + scripts/ci-runner.Dockerfile | 6 ++--- scripts/generic-ci-tests.sh | 2 +- scripts/jenkins-common.sh | 4 +--- scripts/xdist/prepare_xdist_nodes.sh | 7 +----- scripts/xdist/setup_worker.sh | 9 ++++---- tox.ini | 6 ++--- 17 files changed, 49 insertions(+), 95 deletions(-) delete mode 100644 requirements/edx/django.txt diff --git a/.github/workflows/pylint-checks.yml b/.github/workflows/pylint-checks.yml index 780d98f2a90a..808294b03052 100644 --- a/.github/workflows/pylint-checks.yml +++ b/.github/workflows/pylint-checks.yml @@ -56,9 +56,13 @@ jobs: - name: Install required Python dependencies run: | + # dev-requirements is needed because the linter will otherwise + # trip over some dev-only things like django-debug-toolbar + # (import debug_toolbar) that aren't in testing.txt. make dev-requirements pip uninstall -y mysqlclient pip install --no-binary mysqlclient mysqlclient + pip check - name: Run quality tests run: | diff --git a/.github/workflows/quality-checks.yml b/.github/workflows/quality-checks.yml index e28be7aba70e..f6e99ffc92ea 100644 --- a/.github/workflows/quality-checks.yml +++ b/.github/workflows/quality-checks.yml @@ -57,19 +57,16 @@ jobs: - name: Install Required Python Dependencies env: - PIP_SRC_DIR: ${{ runner.temp }} + PIP_SRC: ${{ runner.temp }} run: | - pip install -r requirements/pip.txt - pip install -r requirements/edx/testing.txt --src $PIP_SRC_DIR - pip install -e . --src $PIP_SRC_DIR - pip install -r requirements/edx/django.txt + make test-requirements - name: Run Quality Tests env: TEST_SUITE: quality SCRIPT_TO_RUN: ./scripts/generic-ci-tests.sh SHARD: 4 - PIP_SRC_DIR: ${{ runner.temp }} + PIP_SRC: ${{ runner.temp }} TARGET_BRANCH: ${{ github.base_ref }} run: | ./scripts/all-tests.sh diff --git a/.github/workflows/unit-tests-gh-hosted.yml b/.github/workflows/unit-tests-gh-hosted.yml index 94473111404d..fc5f9bee6371 100644 --- a/.github/workflows/unit-tests-gh-hosted.yml +++ b/.github/workflows/unit-tests-gh-hosted.yml @@ -15,7 +15,8 @@ jobs: fail-fast: false matrix: python-version: [ '3.8' ] - django-version: [ "3.2" ] + django-version: + - "pinned" shard_name: [ "lms-1", "lms-2", @@ -60,16 +61,18 @@ jobs: uses: actions/cache@v2 with: path: ${{ steps.pip-cache-dir.outputs.dir }} - key: ${{ runner.os }}-pip-${{ hashFiles('requirements/edx/development.txt') }} + key: ${{ runner.os }}-pip-${{ hashFiles('requirements/edx/testing.txt') }} restore-keys: ${{ runner.os }}-pip- - name: Install Required Python Dependencies + env: + PIP_SRC: ${{ runner.temp }} run: | - pip install -r requirements/pip.txt - pip install -r requirements/edx/development.txt --src ${{ runner.temp }} - # edx-platform installs some Python projects from within the edx-platform repo itself. - pip install -e . + make test-requirements + if [[ "${{ matrix.django-version }}" != "pinned" ]]; then pip install "django~=${{ matrix.django-version }}.0" + pip check # fail if this test-reqs/Django combination is broken + fi - name: Setup and run tests uses: ./.github/actions/unit-tests @@ -80,7 +83,8 @@ jobs: strategy: matrix: python-version: [ '3.8' ] - django-version: [ "3.2" ] + django-version: + - "pinned" steps: - uses: actions/checkout@v2 @@ -102,14 +106,16 @@ jobs: uses: actions/cache@v2 with: path: ${{ steps.pip-cache-dir.outputs.dir }} - key: ${{ runner.os }}-pip-${{ hashFiles('requirements/edx/development.txt') }} + key: ${{ runner.os }}-pip-${{ hashFiles('requirements/edx/testing.txt') }} restore-keys: ${{ runner.os }}-pip- - name: Install Required Python Dependencies run: | - pip install -r requirements/pip.txt - make dev-requirements + make test-requirements + if [[ "${{ matrix.django-version }}" != "pinned" ]]; then pip install "django~=${{ matrix.django-version }}.0" + pip check # fail if this test-reqs/Django combination is broken + fi - name: verify unit tests count uses: ./.github/actions/verify-tests-count diff --git a/.github/workflows/unit-tests.yml b/.github/workflows/unit-tests.yml index 66cb5f7fd13f..54ad4f5891dc 100644 --- a/.github/workflows/unit-tests.yml +++ b/.github/workflows/unit-tests.yml @@ -62,10 +62,9 @@ jobs: - name: install requirements run: | sudo make test-requirements - if [[ "${{ matrix.django-version }}" == "pinned" ]]; then - sudo pip install -r requirements/edx/django.txt - else + if [[ "${{ matrix.django-version }}" != "pinned" ]]; then sudo pip install "django~=${{ matrix.django-version }}.0" + sudo pip check # fail if this test-reqs/Django combination is broken fi - name: list installed package versions diff --git a/Makefile b/Makefile index dd97cbb6b960..7aa7334fdfac 100644 --- a/Makefile +++ b/Makefile @@ -118,12 +118,6 @@ $(COMMON_CONSTRAINTS_TXT): compile-requirements: export CUSTOM_COMPILE_COMMAND=make upgrade compile-requirements: $(COMMON_CONSTRAINTS_TXT) ## Re-compile *.in requirements to *.txt - @# This is a temporary solution to override the real common_constraints.txt - @# In edx-lint, until the pyjwt constraint in edx-lint has been removed. - @# See BOM-2721 for more details. - sed 's/Django<2.3//g' requirements/common_constraints.txt > requirements/common_constraints.tmp - mv requirements/common_constraints.tmp requirements/common_constraints.txt - pip install -q pip-tools pip-compile --allow-unsafe --upgrade -o requirements/edx/pip.txt requirements/edx/pip.in @@ -139,11 +133,6 @@ compile-requirements: $(COMMON_CONSTRAINTS_TXT) ## Re-compile *.in requirements pip install -qr requirements/edx/pip.txt pip install -qr requirements/edx/pip-tools.txt - # Let tox control the Django version for tests - grep -e "^django==" requirements/edx/base.txt > requirements/edx/django.txt - sed '/^[dD]jango==/d' requirements/edx/testing.txt > requirements/edx/testing.tmp - mv requirements/edx/testing.tmp requirements/edx/testing.txt - upgrade: pre-requirements ## update the pip requirements files to use the latest releases satisfying our constraints $(MAKE) compile-requirements COMPILE_OPTS="--upgrade" diff --git a/pavelib/prereqs.py b/pavelib/prereqs.py index 3b588f65faf6..33cbab7ef987 100644 --- a/pavelib/prereqs.py +++ b/pavelib/prereqs.py @@ -24,10 +24,8 @@ # a corresponding change to circle.yml, which is how the python # prerequisites are installed for builds on circleci.com toxenv = os.environ.get('TOXENV') -if toxenv and toxenv != 'quality-django32': +if toxenv and toxenv != 'quality': PYTHON_REQ_FILES = ['requirements/edx/testing.txt'] -elif toxenv and toxenv == 'quality-django32': - PYTHON_REQ_FILES = ['requirements/edx/testing.txt', 'requirements/edx/django.txt'] else: PYTHON_REQ_FILES = ['requirements/edx/development.txt'] @@ -173,11 +171,7 @@ def python_prereqs_installation(): def pip_install_req_file(req_file): """Pip install the requirements file.""" pip_cmd = 'pip install -q --disable-pip-version-check --exists-action w' - - if Env.PIP_SRC_DIR: - sh(f"{pip_cmd} -r {req_file} --src {Env.PIP_SRC_DIR}") - else: - sh(f"{pip_cmd} -r {req_file}") + sh(f"{pip_cmd} -r {req_file}") @task @@ -313,8 +307,8 @@ def install_python_prereqs(): files_to_fingerprint.append(sysconfig.get_python_lib()) # In a virtualenv, "-e installs" get put in a src directory. - if Env.PIP_SRC_DIR: - src_dir = Env.PIP_SRC_DIR + if Env.PIP_SRC: + src_dir = Env.PIP_SRC else: src_dir = os.path.join(sys.prefix, "src") if os.path.isdir(src_dir): diff --git a/pavelib/tests.py b/pavelib/tests.py index 28988e8f05e9..39c0fb4f7abd 100644 --- a/pavelib/tests.py +++ b/pavelib/tests.py @@ -32,10 +32,6 @@ ("test-id=", "t", "Test id"), ("fail-fast", "x", "Fail suite on first failed test"), ("fasttest", "a", "Run without collectstatic"), - make_option( - "--django_version", dest="django_version", - help="Run against which Django version (3.2)." - ), make_option( "--eval-attr", dest="eval_attr", help="Only run tests matching given attribute expression." @@ -106,10 +102,8 @@ def test_system(options, passthrough_options): """ system = getattr(options, 'system', None) test_id = getattr(options, 'test_id', None) - django_version = getattr(options, 'django_version', None) assert system in (None, 'lms', 'cms') - assert django_version in (None, '3.2') if hasattr(options.test_system, 'with_wtw'): call_task('fetch_coverage_test_selection_data', options={ @@ -161,10 +155,6 @@ def test_system(options, passthrough_options): ("test-id=", "t", "Test id"), ("failed", "f", "Run only failed tests"), ("fail-fast", "x", "Run only failed tests"), - make_option( - "--django_version", dest="django_version", - help="Run against which Django version (3.2)." - ), make_option( "--eval-attr", dest="eval_attr", help="Only run tests matching given attribute expression." @@ -201,9 +191,6 @@ def test_lib(options, passthrough_options): """ lib = getattr(options, 'lib', None) test_id = getattr(options, 'test_id', lib) - django_version = getattr(options, 'django_version', None) - - assert django_version in (None, '3.2') if test_id: # Testing a single test id. diff --git a/pavelib/utils/envs.py b/pavelib/utils/envs.py index ef8ce87e5662..9e2e2fe5b831 100644 --- a/pavelib/utils/envs.py +++ b/pavelib/utils/envs.py @@ -216,8 +216,9 @@ class Env: # Directory for i18n test reports I18N_REPORT_DIR = REPORT_DIR / 'i18n' - # Directory for keeping src folder that comes with pip installation - PIP_SRC_DIR = os.environ.get("PIP_SRC_DIR") + # Directory for keeping src folder that comes with pip installation. + # Setting this is equivalent to passing `--src ` to pip directly. + PIP_SRC = os.environ.get("PIP_SRC") # Service variant (lms, cms, etc.) configured with an environment variable # We use this to determine which envs.json file to load. diff --git a/pavelib/utils/test/suites/pytest_suite.py b/pavelib/utils/test/suites/pytest_suite.py index 06de92334d34..0509170c0490 100644 --- a/pavelib/utils/test/suites/pytest_suite.py +++ b/pavelib/utils/test/suites/pytest_suite.py @@ -24,11 +24,6 @@ def __init__(self, *args, **kwargs): self.failed_only = kwargs.get('failed_only', False) self.fail_fast = kwargs.get('fail_fast', False) self.run_under_coverage = kwargs.get('with_coverage', True) - django_version = kwargs.get('django_version', None) - if django_version is None: - self.django_toxenv = None - else: - self.django_toxenv = 'py27-django{}'.format(django_version.replace('.', '')) self.disable_courseenrollment_history = kwargs.get('disable_courseenrollment_history', '1') self.disable_capture = kwargs.get('disable_capture', None) self.report_dir = Env.REPORT_DIR / self.root @@ -151,18 +146,14 @@ def _under_coverage_cmd(self, cmd): @property def cmd(self): - if self.django_toxenv: - cmd = ['tox', '-e', self.django_toxenv, '--'] - else: - cmd = [] - cmd.extend([ + cmd = [ 'python', '-Wd', '-m', 'pytest', '--ds={}'.format(f'{self.root}.envs.{self.settings}'), f"--junitxml={self.xunit_report}", - ]) + ] cmd.extend(self.test_options_flags) if self.verbosity < 1: cmd.append("--quiet") @@ -279,17 +270,13 @@ def __init__(self, *args, **kwargs): @property def cmd(self): - if self.django_toxenv: - cmd = ['tox', '-e', self.django_toxenv, '--'] - else: - cmd = [] - cmd.extend([ + cmd = [ 'python', '-Wd', '-m', 'pytest', f'--junitxml={self.xunit_report}', - ]) + ] cmd.extend(self.passthrough_options + self.test_options_flags) if self.verbosity < 1: cmd.append("--quiet") diff --git a/requirements/edx/django.txt b/requirements/edx/django.txt deleted file mode 100644 index e92ee8ee6a63..000000000000 --- a/requirements/edx/django.txt +++ /dev/null @@ -1 +0,0 @@ -django==3.2.16 diff --git a/requirements/edx/testing.txt b/requirements/edx/testing.txt index b242ad107af3..331937ff811c 100644 --- a/requirements/edx/testing.txt +++ b/requirements/edx/testing.txt @@ -254,6 +254,7 @@ dill==0.3.6 # via pylint distlib==0.3.6 # via virtualenv +django==3.2.16 # via # -c requirements/edx/../common_constraints.txt # -r requirements/edx/base.txt diff --git a/scripts/ci-runner.Dockerfile b/scripts/ci-runner.Dockerfile index e9acdeb34aed..999b89941eff 100644 --- a/scripts/ci-runner.Dockerfile +++ b/scripts/ci-runner.Dockerfile @@ -46,10 +46,10 @@ COPY openedx/core/lib openedx/core/lib COPY lms lms COPY cms cms COPY requirements/pip.txt requirements/pip.txt +COPY requirements/edx/pip-tools.txt requirements/edx/pip-tools.txt COPY requirements/edx/testing.txt requirements/edx/testing.txt -COPY requirements/edx/django.txt requirements/edx/django.txt -RUN pip install -r requirements/pip.txt && \ -pip install -r requirements/edx/testing.txt -r requirements/edx/django.txt +COPY Makefile Makefile +RUN make test-requirements FROM base as runner diff --git a/scripts/generic-ci-tests.sh b/scripts/generic-ci-tests.sh index eddc29879183..ea4dcb4ae542 100755 --- a/scripts/generic-ci-tests.sh +++ b/scripts/generic-ci-tests.sh @@ -70,7 +70,7 @@ if [ -z ${TOX_ENV+x} ] || [[ ${TOX_ENV} == 'null' ]]; then echo "TOX_ENV: ${TOX_ENV}" TOX="" elif tox -l |grep -q "${TOX_ENV}"; then - if [[ "${TOX_ENV}" == 'quality-django32' ]]; then + if [[ "${TOX_ENV}" == 'quality' ]]; then TOX="" else TOX="tox -r -e ${TOX_ENV} --" diff --git a/scripts/jenkins-common.sh b/scripts/jenkins-common.sh index 4d5bbc1512e8..5eec5b5b4922 100644 --- a/scripts/jenkins-common.sh +++ b/scripts/jenkins-common.sh @@ -48,9 +48,7 @@ source $VENV_PATH/edx-venv/bin/activate # Hack to fix up egg-link files given that the virtualenv is not relocatable sed -i "s|^/home/jenkins/shallow-clone|`pwd`|" -- \ $VENV_PATH/edx-venv/lib/python*/site-packages/*.egg-link -pip install pip==21.3 -pip install -qr requirements/edx/pip-tools.txt -pip-sync -q requirements/edx/testing.txt requirements/edx/django.txt +make test-requirements # add the node packages dir to PATH PATH=$PATH:node_modules/.bin diff --git a/scripts/xdist/prepare_xdist_nodes.sh b/scripts/xdist/prepare_xdist_nodes.sh index 5eddab8c7690..8346f2d4619c 100644 --- a/scripts/xdist/prepare_xdist_nodes.sh +++ b/scripts/xdist/prepare_xdist_nodes.sh @@ -10,18 +10,13 @@ python scripts/xdist/pytest_worker_manager.py -a up -n ${XDIST_NUM_WORKERS} \ -key ${XDIST_WORKER_KEY_NAME} \ -iam ${XDIST_WORKER_IAM_PROFILE_ARN} -# Install the correct version of Django depending on which tox environment (if any) is in use -if [[ -z ${TOXENV+x} ]] || [[ ${TOXENV} == 'null' ]] || [[ ${TOXENV} == *'django32'* ]]; then - DJANGO_REQUIREMENT="requirements/edx/django.txt" -fi - ip_list=$(&2 "Usage: $0 -p python-version -d django-reqs-file" + print >&2 "Usage: $0 -p python-version" exit 1 ;; esac @@ -21,7 +20,7 @@ source $venv/bin/activate # Hack to fix up egg-link files given that the virtualenv is not relocatable sed -i "s|\(^/home/jenkins\)/shallow-clone|\1/edx-platform|" -- \ $venv/lib/python*/site-packages/*.egg-link -pip install -qr requirements/edx/pip-tools.txt -pip-sync -q requirements/edx/testing.txt "${DJANGO_REQUIREMENT}" + +make test-requirements mkdir reports diff --git a/tox.ini b/tox.ini index 0bcc2ae0bb14..689407650699 100644 --- a/tox.ini +++ b/tox.ini @@ -1,5 +1,5 @@ [tox] -envlist = py38-django{32}, quality-django{32} +envlist = py38, quality # This is needed to prevent the lms, cms, and openedx packages inside the "Open # edX" package (defined in setup.py) from getting installed into site-packages @@ -64,9 +64,7 @@ passenv = XDIST_WORKER_KEY_NAME XDIST_WORKER_SECURITY_GROUP XDIST_WORKER_SUBNET -deps = - django32: -r requirements/edx/django.txt - -r requirements/edx/testing.txt +commands_pre = make test-requirements whitelist_externals = /bin/bash /usr/bin/curl