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

Test with postgresql and mariadb on multiple versions of Django. #1226

Merged
merged 4 commits into from Jan 31, 2020

Conversation

tim-schilling
Copy link
Contributor

@tim-schilling tim-schilling commented Dec 11, 2019

Expands the tests for the three latest versions of Django.

  • Fixes issues with Django 3.0.0 and postgres.
  • Unpins mysqlclient done with 995db40

@tim-schilling
Copy link
Contributor Author

I was able to track down the problem to postgres having a different CursorDebugWrapper class defined which was not being monkeypatched by debugsqlshell.

@codecov
Copy link

codecov bot commented Dec 20, 2019

Codecov Report

Merging #1226 into master will increase coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1226      +/-   ##
==========================================
+ Coverage   86.59%   86.62%   +0.03%     
==========================================
  Files          25       25              
  Lines        1432     1436       +4     
  Branches      205      206       +1     
==========================================
+ Hits         1240     1244       +4     
  Misses        140      140              
  Partials       52       52
Impacted Files Coverage Δ
debug_toolbar/management/commands/debugsqlshell.py 100% <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 a8ae50b...0caa826. Read the comment docs.

@tim-schilling tim-schilling force-pushed the tox-expand-db-tests branch 3 times, most recently from 314762f to ef6117a Compare December 27, 2019 20:04
Copy link
Member

@matthiask matthiask left a comment

Choose a reason for hiding this comment

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

Wouldn't you want to change .travis.yml too to include the new test environments? (Of course this makes tests slower again.)

if connection.vendor == "postgresql":
from django.db.backends.postgresql import base as base_module
else:
from django.db.backends import utils as base_module
Copy link
Member

Choose a reason for hiding this comment

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

Why is the postgresql branch necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Django has a new subclass for the CursorDebugWrapper for postgres. With Django 3 and postgres, the current version of the toolbar's debug_shell command doesn't work.

@tim-schilling
Copy link
Contributor Author

You're absolutely right about .travis.yml. For some reason I thought it was using the .tox file.

@pauloxnet
Copy link
Member

Hi @tim-schilling thanks for your job! I have the same problem described in issue #1231 . Do you have any news about this PR ?

@gabbork
Copy link

gabbork commented Jan 28, 2020

Hi, I have the same issue too. Any news? Thanks a lot

@filippo-20tab
Copy link

filippo-20tab commented Jan 28, 2020

Same issue here. Please, fix this ASAP! Thanks a lot for your amazing job.

@tim-schilling
Copy link
Contributor Author

Sorry @matthiask I didn't notice the errors. Everything should be good now, unless you want me to change how to handle the conditional import logic.

@matthiask matthiask merged commit 98308a2 into jazzband:master Jan 31, 2020
@matthiask
Copy link
Member

Awesome, thanks!

@tim-schilling tim-schilling deleted the tox-expand-db-tests branch January 31, 2020 16:29
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.

None yet

5 participants