build: Set Django version for tests more safely; drop support for non-GHA by timmc-edx · Pull Request #31387 · openedx/openedx-platform

timmc-edx · 2022-12-01T16:34:45Z

This is for #31374

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

Guide to the files in this PR:

Makefile: Used by all of the other files here -- make test-requirements is what we'll use for all test-dependencies installation going forward. Only changes here are to make compile-requirements (shared by make upgrade) in order to undo the testing.txt/django.txt split.
- requirements/edx/testing.txt and requirements/edx/django.txt are changed as part of this
.github/workflows/unit-tests.yml: Our main pytest CI runner; runs as a GitHub Action but uses the edx-platform-runner hosted by 2U. Only runs for edx-platform's main copy and a private fork.
- scripts/ci-runner.Dockerfile seems to be the docker image for this runner
- ✔️ The changes to this file are tested by the checks running on this PR. I've also confirmed that adding 4.0 to the matrix causes the appropriate failures (conflict with django-splash).
.github/workflows/unit-tests-gh-hosted.yml: Same role, but runs on any other fork of edx-platform instead, and uses GHA runners (slower). Should be otherwise identical.
- ✔️ Tested on a fork: [DO NOT MERGE] test gh-hosted w/expected 4.0 failure timmc-edx/edx-platform#2
.github/workflows/pylint-checks.yml: Our main pylint` checks
- ✔️ Tested on this PR
.github/workflows/quality-checks.yml: Runs various other quality checks using scripts/generic-ci-tests.sh(via./scripts/all-tests.sh`, unaffected here) which calls paver via tox
- ✔️ Tested on this PR
tox.ini: Configures the tox testing environment, used here for quality checks; I've removed the "django" specifiers from its environments.
- ✔️ Invoked by quality tests here
scripts/xdist/prepare_xdist_nodes.sh calls scripts/xdist/setup_worker.sh
- scripts/unit-tests.sh (unaffected by PR) calls in to these via generic-ci-tests.sh, but only if environment variable TEST_SUITE is set to one of lms-unit, cms-unit, or pavelib-unit
- They're used in splitting up and farming out pytest runs to multiple workers. Can allegedly be run locally, Maybe these were used only on Jenkins, or maybe the new hosted test runner still uses these?
- ✔️ I was able to run TEST_SUITE=lms-unit SHARD=1 scripts/generic-ci-tests.sh successfully after running make lms-up in devstack. Calling that good enough.
scripts/jenkins-common.sh seems to be part of our old (now shut down) Jenkins test runners
- ❓ Probably can't test any more
pavelib/prereqs.py installs Python dependencies for paver runs (used for quality checks, among other things)
- pavelib/utils/envs.py is used by this
- Their own unit tests were also affected: pavelib/tests.py and pavelib/utils/test/suites/pytest_suite.py
- ✔️ I think these can be considered tested thanks to the quality checks that run

timmc-edx · 2022-12-01T18:41:10Z

[Author's note] I believe that sh will pass through the PIP_SRC environment variable. This sh is actually from paver.shell, but it defaults to env=None and passes that to Popen, which should pass through the caller's environment for None.

timmc-edx · 2022-12-01T18:43:09Z

[Author's note] I'm not sure if this was really correct in the first place -- shouldn't the quality checks be using dev deps (pylint will complain about missing django-debug-toolbar) and pytest be using just testing deps?

We're looking into the feasibility of ripping out this paver code anyway; note that the GitHub Actions workflows already use pylint directly instead of going through paver.

…-GHA 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

jmbowman

Would have saved you some time if we'd already done the cleanup pass of all the old Jenkins cruft first...oh well. Looks pretty good, just a couple of suggestions.

jmbowman · 2022-12-06T19:35:02Z

We're looking into the feasibility of ripping out this paver code anyway; note that the GitHub Actions workflows already use pylint directly instead of going through paver.

jmbowman · 2022-12-06T19:39:25Z

 distlib==0.3.6
    # via virtualenv
+django==3.2.16
    # via


Ha, kind of funny that we were ripping out the dependency but leaving the comments for it.

jmbowman · 2022-12-06T19:41:42Z

-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
+RUN make test-requirements


Don't you need to copy in the Makefile also to be able to run this?

Hmm, seems likely! Adding that. :-)

jmbowman · 2022-12-06T19:45:50Z

@@ -1,5 +1,5 @@
 [tox]
-envlist = py38-django{32}, quality-django{32}
+envlist = py38, quality


Can we leave this as it was, but still make your change later in the file? I think the suffixes are harmless for now, and will be needed later when we start testing 4.0 and 4.1. And I like being more explicit about which Django version is being tested.

I'm not sure what benefit they would provide -- tox would no longer be controlling the Django version, and still wouldn't be once 4.x testing starts.

It would be for local testing of the 4.x upgrade. But I suppose it would be clearer to remove this for now and just add it back once it's actually implemented when we start that testing.

I think for local testing what we'd want is just a branch where Django is upgraded to 4.x and tests are otherwise run as normal.

iamsobanjaved

LGTM. All changes were nicely explained

edx-pipeline-bot · 2022-12-12T23:28:43Z

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production.

edx-pipeline-bot · 2022-12-13T00:00:57Z

EdX Release Notice: This PR has been deployed to the production environment.

timmc-edx force-pushed the timmc/django-pins branch from 96ba09e to 18c7365 Compare December 1, 2022 17:43

timmc-edx commented Dec 1, 2022

View reviewed changes

timmc-edx force-pushed the timmc/django-pins branch from 6a2eacc to 11009ff Compare December 1, 2022 19:36

timmc-edx marked this pull request as ready for review December 1, 2022 19:37

timmc-edx force-pushed the timmc/django-pins branch from 11009ff to c395d93 Compare December 1, 2022 20:04

timmc-edx mentioned this pull request Dec 1, 2022

Investigate non-repeatable edx-platform dependencies in tests #31368

Closed

timmc-edx force-pushed the timmc/django-pins branch from 96dac6c to bb8b8b5 Compare December 2, 2022 18:54

timmc-edx changed the title ~~ci: Override Django version for tests in a safer way~~ build: Set Django version for tests more safely; drop support for non-GHA Dec 2, 2022

timmc-edx force-pushed the timmc/django-pins branch from bb8b8b5 to 7021e12 Compare December 2, 2022 21:20

timmc-edx force-pushed the timmc/django-pins branch from 7021e12 to 760fdc7 Compare December 2, 2022 21:29

jmbowman reviewed Dec 6, 2022

View reviewed changes

fixup! Include Makefile for test runner image

11cb92c

iamsobanjaved approved these changes Dec 12, 2022

View reviewed changes

timmc-edx merged commit b8a34f0 into master Dec 12, 2022

timmc-edx deleted the timmc/django-pins branch December 12, 2022 21:45

jmbowman mentioned this pull request Jan 11, 2023

feat: improve requirements file structure #31085

Closed

Uh oh!

Conversation

timmc-edx commented Dec 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

timmc-edx Dec 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmbowman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iamsobanjaved left a comment

Choose a reason for hiding this comment

Uh oh!

edx-pipeline-bot commented Dec 12, 2022

Uh oh!

edx-pipeline-bot commented Dec 13, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

timmc-edx commented Dec 1, 2022 •

edited

Loading

timmc-edx Dec 1, 2022 •

edited

Loading