Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Tests, CI, and Matrix for different Django and Python versions, Fix compat issues found in Matrix #6

Merged
merged 20 commits into from
Oct 26, 2019

Conversation

agilgur5
Copy link
Owner

@agilgur5 agilgur5 commented Sep 14, 2019

PR Initial Summary

Need to write-up a real PR description for this elaborating on all my struggles with Poetry and versioning bugs found everywhere. Will edit this later, gonna go to bed soon. This ended up taking a lot longer and being a lot bigger than I expected/intended for 😅

One thing I wanted to note was that I need to add a CONTRIBUTING.md to this PR now that there's a bunch of stuff involved in Development and some confusing things (e.g. Poetry not doing the Packaging/Publishing, just used for dev).

But yea 92% test coverage across multiple versions of Django. And some bugs found and fixed. Will release v0.0.6 with this, but this library is now ready for Beta at least.


EDIT: Here's the real PR description, almost a month later (it's been extremely busy 😕 )

PR Description

This PR was originally just supposed to add like a single test for latest Django. It turned out a lot bigger than that.

DevDeps set-up

For one thing, adding testing means adding dev dependencies -- there are currently no actual dependencies (Django is a peerDep), so this means adding dependency management as well. requirements.txt used to be the norm, but I was looking at installing via tox as well since we will (and now do) have multiple versions to test against. I forget how I ended up at pipenv, but that's what I started using for dependency management, and immediately I ran into issues during basic installation. Due to pypa/pipenv#2396, installing a new dependency doesn't quite work the same as it would with, say npm. This was already causing me some hassle and can be done automatically, so I started looking for an alternative solution and remembered reading about poetry some time ago.

And yay, Poetry has some much more intuitive and automatic usage, particularly for semver users. Poetry is not without its own bugs though, as I immediately hit this issue: python-poetry/poetry#534 (comment). But once I fixed that, it worked all great until we got to multiple versions.
I was not able to use it to replace the publishing system of setup.py, MANIFEST.in, setup.cfg quite yet though. Some of the issues I ran into on this front (see first commit) might be fixed in the 1.0.0 prereleases however (may be worth upgrading).

Once I started adding testing dependencies I ran into some nuances with Poetry. If you don't specify multiple constraints, you will get errors when installing as one version of a dep generally won't fulfill both Python requirements -- so had to pin to ^3.5. Pinning a Python version also changes the publishing behavior, even if that version is only necessary for dev deps.

Test harness set-up

Got through adding the test harness and first tests rather painlessly, though it took a bit to figure out the proper testing structure/directories and the minimum necessary Django files for the tests to actually run. Very helpful that poetry install also does an editable install of the package being built as well. Had to learn some of the nuances of how the testing and coverage defaults in Pytest differ from, say, Jest, and had a bit of confusion when adding testing settings (e.g. thought I could put everything in .coverragerc instead of pytest.ini, but that didn't work). Adding simple CI & CodeCov went fairly smoothly.

Multi-version tests

And then we get into the real time-consuming and big issues when I decided "hey might as well throw in extra tests for different versions in this PR as well!" and there went all my time into a debugging void 😅

For one thing, multiple constraints totally did not work in Poetry, at least not for devDeps: python-poetry/poetry#666 (comment) . So from there, I had to either do manual poetry add -D in CI commands, which only worked while all other constraints were satisfied by said deps, or just use straight pip install with pinned version numbers of all packages 😕 😕 . That's pretty far from ideal and is similar to using Tox. I was hoping I could use Poetry to manage dependencies across multiple versions of Python / Django, etc with all its benefits and tracking, but I couldn't, so had to do it a bunch of version and compatibility tracking manually. That meant I couldn't use poetry for finding correct dependency versions, installation, or testing, which is quite a few lost benefits. It also means that I can't remove setup.py once Poetry is used for publishing because it's needed for editable install when using pip directly in these different environments.

I also didn't use Tox because of reading various difficulties from a few places of integrating it with Travis. The downside is that a bunch of tests can only run in CI now, so I might want to change this in the future.

Poetry also doesn't quite support Python 3.4 (python-poetry/poetry#1223) despite saying that it does, and Django 1.5 can't use Python 3.5 (https://stackoverflow.com/questions/34827566/attributeerror-module-html-parser-has-no-attribute-htmlparseerror/36000103#36000103)

Also, a multiple constraint on something other than the Python version, for instance, on the version of Django, just caused Poetry to infinite loop and eventually error out.

Django 1.5 testing

The Django 1.5 test was incredibly difficult to figure out all the compat issues for.
Like we need to drop down to pytest-djangov2.5 for it to work with Django 1.5.
And then I got some really cryptic errors that were actually compatibility issues with pytestv3.6+, so I had to downgrade all the way down to pytestv3.5. The RemovedInPytest4Warning of course don't occur anymore in Pytest 4+, and especially not in Pytest 5+, hence some cryptic warnings. pytest-django also doesn't directly depend on pytest, so there's no error during installation from pip either.
pytest-cov then failed to install. This was at least a clear error, but it was hard to figure out which version would work until I found this commit and downgraded to pytest-covv2.6. The changelog for 2.6.1 doesn't quite mention that support for pytestv3.5 was dropped

Summary here is that lacking semver and peerDeps support in Python-land are missing quite a bit and that can cause quite a bit of issues

Python 2.7 testing

Fortunately this only required pytest<5 and this was written in its changelog.

As pointed out in the commit adding this test, I did get an incredibly weird, reproducible issue where editable install read from pyproject.toml despite this being Python 2.7 and pip using setup.py instead anyway. Reproduced here: https://travis-ci.org/agilgur5/django-serializable-model/jobs/584822609

- use poetry primarily for dependency management moving forward
  - would also like to use for simplifying packaging as well in the
    future

- add most of everything from MANIFEST.in, setup.cfg, and setup.py
  into poetry sections
  - TODO: remove MANIFEST.in, setup.cfg, setup.py etc once the builds
    are completely equivalent
    - currently doesn't build a (dated) egg, has less keywords, and
      custom URLs may not work in this version (needs a 1.0.0
      prerelease probably)
      - setup.py in tarball is missing a lot, including classifiers,
        long_description_content_type, keywords, project_urls, and
        is using distutils instead of setuptools
        - think most are fixed in the 1.0.0 prereleases
      - tarball is also missing egg-info
        - unsupported by poetry and probably unnecessary anyway tbh,
          not sure if any users used eggs
      - PKG-INFO has keywords comma separated instead of space
        separated as the setup.py (i.e. non-poetry) does
@codecov-io
Copy link

codecov-io commented Sep 14, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@b0cbb8d). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master       #6   +/-   ##
=========================================
  Coverage          ?   91.78%           
=========================================
  Files             ?        1           
  Lines             ?       73           
  Branches          ?       15           
=========================================
  Hits              ?       67           
  Misses            ?        4           
  Partials          ?        2
Impacted Files Coverage Δ
django_serializable_model.py 91.78% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b0cbb8d...7022149. Read the comment docs.

Copy link
Owner Author

@agilgur5 agilgur5 left a comment

Choose a reason for hiding this comment

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

A few changes needed already. Also, as said above, need a CONTRIBUTING.md

.travis.yml Outdated Show resolved Hide resolved
.gitignore Show resolved Hide resolved
.travis.yml Outdated Show resolved Hide resolved
.travis.yml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
.travis.yml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
- had to pin to python^3.5 as it's the common denominator between deps
  - add .python-version file to use python3.7 w/ pyenv for this project
  - will have to include python^2.7 again in order to publish a
    universal wheel compatible with 2.7 as the poetry python dep is
    used for both deps and publishing
  - will also have to add dep versions against python 2.7 in order to
    test against it and lower Django versions
    - either in Poetry itself or with something like Tox?
    - gitignore poetry.lock bc multiple pythons and dep versions
- all the models are taken straight from the README so that this
  ensures the README examples work as expected

- manage.py, settings.py, and apps.py are all necessary to get the
  tests to run in a barebones Django environment
- this is required in Django 2+ and gave errors
  - is best practice anyway so added to README as well
    - add compatibility note for less than 1.3
- all 3 are taken straight from the README as well, and cover
  whitelists, one-to-one relations, and foreign key relations
  - should add more tests for larger sets of relations, but these do
    cover the vast majority of functionality and most common use cases
    (that are even in the README)

(env): gitignore pytest_cache directory
- learning the pytest-cov args as I go...
  - most of these are defaults in Jest o.o
- poetry requires a bit of deviation from the defaults, but all good :)

(docs): add dat slick build | passing CI badge
- add after_script to upload coverage report
- add XML output pytest.ini as CodeCov requires it
  - gitignore XML output

(docs): add dat slick coverage badge to README
- now that there are actual tests for Django 2.2 and Python 3.7, it's
  more than just "probably" or "should" work, we have certainty
  - and instead less certainty about older versions as there aren't
    official tests for them

- be explicit that there are backward compat fixes/features in the
  codebase, lends a lot more certainty than just saying "should be ok",
  "these things make it so it should be ok"

- hopefully we can get some multi-version testing without too much
  hassle and this can be shortened a lot
  - though a bit worried that the test and coverage deps will break
- Django 1.9 and below has the use_for_related_fields backward compat
- use Python 3.5 as it's compatible with both, but will have to do
  some more complex multi-Python and multi-Django (and test deps) later
- got a `TypeError: 'class Meta' got invalid attribute(s): base_manager_name`
  on Django 1.9
  - guess the `Meta` subclass ends up being type-checked at some point,
    so unlike most cases an extra attribute can't exist :/

- w00t multi-version CI already found a bug at least!
- Django 1.5 and below has the backward compat feature for
  get_query_set
- Django 1.5 also happens to be the first version that supports
  Python 3

- add backward compat to the test_project's settings
- do some hacky pinning of dependencies to get running
  - this took A TON of time to figure out how they all line up as they
    don't all directly depend on each other and Python 3.4 broke stuff
    too (including Poetry, which is separately bugging on multiple
    constraints for devDeps)
- the requirement is removed/bugfixed for abstract classes in 1.8

- since django_serializable_model is not imported with a dotted name
  (the package name is a module), Django < 1.8 will error out when it
  tries to automatically infer the app_label via __name__
  - it assumes there must be a dot as it's a Model and therefore must
    be part of an app, but this is an abstract Model without an app
  - it didn't error out at Yorango bc it was imported as
    'utilities.serializable' and so it had a dotted __name__

- do some hackiness to set __name__ to a dotted name in order for a
  app_label to be inferred
  - if app_label were to be set in Model.Meta, then any Model that
    extends will _have to_ override app_label
    - this is far from ideal, and if it doesn't, it will cause errors,
      like the tests even failed because of incorrectly inferred DB
      table names
  - had to set SerializableModel.__module__ as this is apparently also
    used by Django to access sys.modules, and by default it is set to
    the __name__ of the module it's in, which is different from its
    name in sys.modules
    - so Django will error during the look-up unless this is also
      hackishly overriden for the class
- needed to rm pyproject.toml because pip decided to read it during
  editable install and errored out
  - how it read the poetry section I don't even know or why that
    instead of setup.py I haven't a clue
  - also not sure why it didn't error out on the Python 3.4 test....
  - dafuq is going on here, it occurs even on job restarts...
- so future readers have "why?" answered immediately and don't have
  to scour through old docs/release notes to find out why
- the previous comments were also redundant
- Have a thorough build matrix now, so can be pretty confident that it
  will work in various versions
  - no need to mention `2to3` as it's tested on both
    - might want to add a test that runs it though for even more
      certainty?
  - no need to mention what versions it was originally on anymore
    - and we can probably bump the package to Beta status at any time
      now
- so that there's a clear place where all the tasks are defined and
  so anything you might use for development is written out there
  - this should make for less confusion between poetry and setup.py
    use-cases
  - this isn't quite a CONTRIBUTING.md, but is kind of an in-between
    - and is a necessary tool for myself to not forget and to help
      contributors anyway

- replace release.sh and changelog.sh with their own tasks
  - and split release into a few tasks that are then combined
    - makes it easier to publish to test PyPI and harder to
      accidentally publish to production PyPI too
- in case someone stumbles into the tests and aren't sure what they're
  looking at -- a README appears!
- also to be more explicit about some decisions around why some Django
  specific files exist and why others don't (as minimal as possible)
- and finally to give directions on how to run the tests, as it's not
  from inside the tests/ directory
- has increased over time due to comments and more backward and
  forward compatibility fixes
  - actual core code has not really changed
Copy link
Owner Author

@agilgur5 agilgur5 left a comment

Choose a reason for hiding this comment

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

Had one final look-through, and think it's all good to go now... finally!

Decided to add a maidfile.md right now instead of a full CONTRIBUTING.md as that'll take some time to write well and intentionally to give potential contributors and potential novices the right impression. I also don't have a CONTRIBUTING.md (or issue or PR templates, or a Code of Conduct for that matter) in any of my repos yet, so it'll be a first for me and can't just copy it from another of my repos (but can use other folks' for inspiration)

Also added a small README to test_project to clarify some things.

One last thing missing is just a publish commit!

- A huge release, but no actual API changes except for bugfixes for
  certain versions of Django
  - fixes for Django < 1.10 and < 1.8
  - add tests with 92% code coverage
  - add test matrix for Django 2.2, 1.11, 1.9, and 1.5 as well as
    Python 3.5, 3.4, and 2.7
    - and improve compatibility docs
  - add Travis CI and CodeCov support
  - add Poetry build-system and testing dependencies
  - add maidfile.md to consolidate tasks and add some contributing docs
@agilgur5 agilgur5 merged commit cd96105 into master Oct 26, 2019
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.

2 participants