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

feat: Django 4.0, 4.1 / Python 3.10/3.11, mysql support, running tests on sqlite, postgres and mysql #287

Merged
merged 36 commits into from
Nov 28, 2022

Conversation

fsbraun
Copy link
Member

@fsbraun fsbraun commented Jun 25, 2022

Description

This PR extends the test suite considerably by introducing tests for Postgres and MySQL. It is a fix for #286 where a migration that fails to run on MySQL.

It turns out that some unit tests did neither run on Postgres nor MySQL, since they compared pk of Version objects with pk of the corresponding Content object which only accidentally worked on sqlite.

Features of the PR

Checklist

  • I have opened this pull request against master
  • I have added or modified the tests when changing logic
  • I have followed the conventional commits guidelines to add meaningful information into the changelog
  • I have read the contribution guidelines and I have joined #workgroup-pr-review on
    Slack to find a “pr review buddy” who is going to review my pull request.

@codecov
Copy link

codecov bot commented Jun 25, 2022

Codecov Report

Merging #287 (f82d312) into master (31f14df) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #287   +/-   ##
=======================================
  Coverage   86.60%   86.60%           
=======================================
  Files          23       23           
  Lines         799      799           
  Branches      105      117   +12     
=======================================
  Hits          692      692           
  Misses         81       81           
  Partials       26       26           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@fsbraun fsbraun requested a review from Aiky30 June 30, 2022 11:56
@Aiky30
Copy link
Contributor

Aiky30 commented Sep 12, 2022

@fsbraun Can you add more details to the error / issue such as MySQL version and the database backend in use, for postgres this tends to be: django.db.backends.postgresql, although this is up to the project author: https://docs.djangoproject.com/en/4.1/ref/databases/#postgresql-connection-settings

I ask because this chnage doesn't single out MySQL and affects all DB's. Ideally we would try and avoid that.

@fsbraun
Copy link
Member Author

fsbraun commented Sep 12, 2022

I've added test for the mysql backend: django.db.backends.mysql and django.db.backends.postgresql. The fix now only affects the mysql backend. The tests run the migrations (that's why I added migrations for the test apps, too).

At this point it turned out that some tests were confusing Version object pk with Content object pk. I've fixed that.

@fsbraun fsbraun self-assigned this Oct 14, 2022
@fsbraun fsbraun marked this pull request as draft October 28, 2022 11:55
@fsbraun fsbraun changed the title Fix: Temporarily set schema_editor to not do migration in atomic block for mysql feat: Django 4.0, 4.1 / Python 3.10/3.11 support, running tests on sqlite, postgres and mysql Nov 8, 2022
@fsbraun fsbraun changed the title feat: Django 4.0, 4.1 / Python 3.10/3.11 support, running tests on sqlite, postgres and mysql feat: Django 4.0, 4.1 / Python 3.10/3.11, mysql support, running tests on sqlite, postgres and mysql Nov 8, 2022
@fsbraun fsbraun marked this pull request as ready for review November 8, 2022 22:15
Copy link
Contributor

@Aiky30 Aiky30 left a comment

Choose a reason for hiding this comment

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

Looks OK to me. Nothing blocking.

Some comments below.

The python version list maybe a little extreme. this is very wasteful on resources (not environmentally friendly). As this is Django 4.1 I guess we need to support what django do, so we could remove 3.7 at least. https://docs.djangoproject.com/en/4.1/releases/4.1/#python-compatibility


# Unreleased django-cms 4.0 compatible packages
https://github.com/django-cms/django-cms/tarball/develop-4#egg=django-cms
https://github.com/divio/djangocms-text-ckeditor/tarball/support/4.0.x#egg=djangocms-text-ckeditor
git+https://github.com/django-cms/django-cms@develop-4#egg=django-cms
Copy link
Contributor

Choose a reason for hiding this comment

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

It's fine to change these, they were in the previous format because the Divio Cloud didn't like Pip urls, it's was more convenient to use the same urls across the projects i..e in and out of the Divio Cloud.

Copy link
Member Author

Choose a reason for hiding this comment

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

Was not aware of this. Will revert.

@@ -0,0 +1,28 @@
# Generated by Django 4.1.3 on 2022-11-08 17:46
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think that the migrations are actually required for test packages, maybe this is a side affect of the testing issue not using postgres. Doesn't really matter to be fair.

@fsbraun fsbraun merged commit 6a29418 into django-cms:master Nov 28, 2022
@fsbraun fsbraun deleted the fix/migration-009-mysql branch December 8, 2022 08:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants