Skip to content

fix: Fix all test/quality failures in python-library and ensure it stays fixed#355

Merged
timmc-edx merged 11 commits intomasterfrom
timmc/test-fixes
Jun 21, 2023
Merged

fix: Fix all test/quality failures in python-library and ensure it stays fixed#355
timmc-edx merged 11 commits intomasterfrom
timmc/test-fixes

Conversation

@timmc-edx
Copy link
Contributor

@timmc-edx timmc-edx commented Jun 13, 2023

The output of python-library had a number of lint, quality, and test failures. This commit 1) fixes all of those failures, and 2) ensures that we have test coverage of those quality checks so that we don't regress in the future.

Fixes to quality checks and unit tests:

  • Don't try to pylint manage.py in base template, since not all of them have one, and this causes a quality check failure for python-library. Simply removing the explicit reference is fine, since it's already covered by *.py.
  • Add a placeholder test so pytest will succeed for python-library. (It fails if there are no tests.) This placeholder test is marked to be skipped, which will hopefully stand out a bit in the resulting repo's test runs.
  • Move doc theme overrides CSS to base template (deleting from layered cookiecutters). There's a line in python-template's doc config that expects a _static dir (html_static_path = ['_static']) but there's no dir in the base, so docs build breaks for python-library (which did not have a copy of the CSS in a _static dir). Moving the CSS to the base fixes this so all cookiecutters can use it.
  • Remove logo_only from base docs config, as it was breaking the docs build for python-library.
    • sphinx-book-theme 1.x apparently has an undocumented breaking change in Remove JQuery and update versions executablebooks/sphinx-book-theme#668 -- logo_only is no longer supported, and instead there is a logo option that I haven't looked into. I'm not sure what we want, but I'd rather unbreak and move on so that we at least don't have a broken docs build.
  • Prevent duplicate target error in various READMEs.
    • The "Getting Help" section header creates an implicit duplicate target that conflicts with the _Getting Help: reference. Rather than doing something more complicated, just inline the link. (This required using a double-underscore link to make it anonymous, otherwise you still get an implicit target!)
    • But also this "getting started" text was copied into the cookiecutter readmes, which are instructions for using the cookiecutters, not used in the template output. This duplicates the root readme in the repo, so I removed that text from those readmes. (Same for in the repo's own readme.)

Other improvements:

  • Simplify python-library's test_upgrade and test_quality unit tests to just be a single call to make upgrade requirements test-all. This allows us to exercise the cookiercutter's Makefile and run all of its quality checks without reproducing the full list of checks in test code. (I haven't tackled the other cookiecutters yet, since some of them actually have test failures if you try to use this approach, or don't yet have a make test-all or equivalent.)
  • Include the docs build and validation in python-library's unit tests.
  • Update django-app's placeholder test to match the skipped one in cookiecutter-python-library.
  • Remove deprecated --recursive arg from isort calls. (Redundant in isort 5+ and causes a warning.) Not related to python-library cc tests, but easy fix.

Merge checklist:
Check off if complete or not applicable:

  • Changelog record added
  • Documentation updated (not only docstrings)
  • Fixup commits are squashed away
  • Unit tests added/updated
  • Manual testing instructions provided
  • Noted any: Concerns, dependencies, deadlines, tickets

… tests

The base template's Makefile had a call to `pylint ... manage.py *.py`
which is inappropriate for some of the cookiecutters, as not all of them
create a manage.py. Simply removing the explicit reference is fine, since
it's already covered by the `*.py`.

This also simplifies the `test_upgrade` and `test_quality` tests for the
cookiecutter by running the existing make targets in a virtualenv in the
baked output, rather than reproducing the full list of checks in test code.

(I haven't tackled the other cookiecutters yet, since some of them
actually have test failures if you try to use this approach, or don't yet
have a `make test-all` or equivalent.)
@timmc-edx
Copy link
Contributor Author

The remaining error was that there weren't any tests for pytest to run, which causes pytest to exit with an error. I've added a placeholder test, copied from the one in cookiecutter-django-app.

There's a line in python-template's doc config that expects a _static dir
(`html_static_path = ['_static']`) but there's no dir, so docs build
breaks for some cookiecutters. This moves the CSS to the base as well so
that all cookiecutters can use it.
This currently breaks because 1) there's a `logo_only` reference that
breaks in recent sphinx-book-theme, and 2) there's a duplicate "getting
help" implicit reference in the README.rst.
Not related to python-library cc tests, but easy fix.
sphinx-book-theme 1.x apparently has an undocumented breaking change in
executablebooks/sphinx-book-theme#668 -- `logo_only`
is no longer supported, and instead there is a `logo` option that I haven't
looked into. I'm not sure what we want, but I'd rather unbreak and move on
so that we at least don't have a broken docs build.
The "Getting Help" section creates an implicit duplicate target that
conflicts with the `_Getting Help:` reference. Rather than doing something
more complicated, just inline the link.

I also noticed this "getting started" text was copied into the cookiecutter
readmes, which are *instructions* for using the cookiecutters, not used in
the template output. This duplicates the root readme in the repo, so I
removed that text. (Same for in the repo's own readme.)

(This required using a double-underscore link to make it anonymous,
otherwise you *still* get an implicit target!)

At this point, tests should pass.
Also use this in cookiecutter-django-app, which requires a bit of rework
of the template in order to deduplicate the import. (Getting the right
number of newlines is a little delicate.)
OK, so that's why the whitespace was managed that way...
======================

Once you have a release, and assuming you have an account there, just go to https://www.djangopackages.com/packages/add/ and add it there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, exactly. (I mentioned this in one of the commits, but I haven't yet updated the PR description to roll up all the change descriptions... need to do that.)

Our real-time conversations are on Slack. You can request a `Slack invitation`_, then join our `community Slack workspace`_.

For more information about these options, see the `Getting Help`_ page.
For more information about these options, see the `Getting Help <https://openedx.org/getting-help>`__ page.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See notes in this commit message: 6c30582 -- happy to have other suggestions, though.

@timmc-edx timmc-edx changed the title fix: Remove manage.py reference from base template; unit-test library tests fix: Fix all test/quality failures in python-library and ensure it stays fixed Jun 21, 2023
Copy link
Contributor

@robrap robrap left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops. I had pending comments that I forgot to submit.

@timmc-edx timmc-edx merged commit 64826fd into master Jun 21, 2023
@timmc-edx timmc-edx deleted the timmc/test-fixes branch June 21, 2023 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants