Skip to content

Commit

Permalink
build: Set Django version for tests more safely; drop support for non…
Browse files Browse the repository at this point in the history
…-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
  • Loading branch information
timmc-edx authored Dec 12, 2022
1 parent cec560b commit b8a34f0
Show file tree
Hide file tree
Showing 17 changed files with 49 additions and 95 deletions.
4 changes: 4 additions & 0 deletions .github/workflows/pylint-checks.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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: |
Expand Down
9 changes: 3 additions & 6 deletions .github/workflows/quality-checks.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
26 changes: 16 additions & 10 deletions .github/workflows/unit-tests-gh-hosted.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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
Expand All @@ -80,7 +83,8 @@ jobs:
strategy:
matrix:
python-version: [ '3.8' ]
django-version: [ "3.2" ]
django-version:
- "pinned"
steps:
- uses: actions/checkout@v2

Expand All @@ -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
5 changes: 2 additions & 3 deletions .github/workflows/unit-tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
11 changes: 0 additions & 11 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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"

Expand Down
14 changes: 4 additions & 10 deletions pavelib/prereqs.py
Original file line number Diff line number Diff line change
Expand Up @@ -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']

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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):
Expand Down
13 changes: 0 additions & 13 deletions pavelib/tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -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."
Expand Down Expand Up @@ -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={
Expand Down Expand Up @@ -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."
Expand Down Expand Up @@ -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.
Expand Down
5 changes: 3 additions & 2 deletions pavelib/utils/envs.py
Original file line number Diff line number Diff line change
Expand Up @@ -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 <dir>` 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.
Expand Down
21 changes: 4 additions & 17 deletions pavelib/utils/test/suites/pytest_suite.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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")
Expand Down Expand Up @@ -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")
Expand Down
1 change: 0 additions & 1 deletion requirements/edx/django.txt

This file was deleted.

1 change: 1 addition & 0 deletions requirements/edx/testing.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
6 changes: 3 additions & 3 deletions scripts/ci-runner.Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
2 changes: 1 addition & 1 deletion scripts/generic-ci-tests.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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} --"
Expand Down
4 changes: 1 addition & 3 deletions scripts/jenkins-common.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
7 changes: 1 addition & 6 deletions scripts/xdist/prepare_xdist_nodes.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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=$(<pytest_worker_ips.txt)
for ip in $(echo $ip_list | sed "s/,/ /g")
do
worker_reqs_cmd="ssh -o StrictHostKeyChecking=no jenkins@$ip
'git clone --branch master --depth 1 -q https://github.com/openedx/edx-platform.git; cd edx-platform;
git fetch -fq origin ${XDIST_GIT_REFSPEC}; git checkout -q ${XDIST_GIT_BRANCH};
scripts/xdist/setup_worker.sh -p $PYTHON_VERSION -d $DJANGO_REQUIREMENT' & "
scripts/xdist/setup_worker.sh -p $PYTHON_VERSION & "

cmd=$cmd$worker_reqs_cmd
done
Expand Down
9 changes: 4 additions & 5 deletions scripts/xdist/setup_worker.sh
Original file line number Diff line number Diff line change
@@ -1,12 +1,11 @@
#!/usr/bin/env bash
# Set up worker node.

while getopts 'p:d:' opt; do
while getopts 'p:' opt; do
case "$opt" in
p) PYTHON_VERSION="$OPTARG";;
d) DJANGO_REQUIREMENT="$OPTARG";;
[?])
print >&2 "Usage: $0 -p python-version -d django-reqs-file"
print >&2 "Usage: $0 -p python-version"
exit 1
;;
esac
Expand All @@ -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
6 changes: 2 additions & 4 deletions tox.ini
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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
Expand Down

0 comments on commit b8a34f0

Please sign in to comment.