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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 4 commits into from
Jan 31, 2020
Merged

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

merged 4 commits into from
Jan 31, 2020

Conversation

tim-schilling
Copy link
Member

@tim-schilling tim-schilling commented Dec 11, 2019 •
edited
Loading

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
Member 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 •
edited
Loading

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
matthiask
matthiask reviewed Jan 8, 2020
View reviewed changes
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.)

debug_toolbar/management/commands/debugsqlshell.py
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
Member 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
Member Author

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

@tim-schilling tim-schilling mentioned this pull request Jan 13, 2020
Closed
@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 •
edited
Loading

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

@tim-schilling
Copy link
Member 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 django-commons: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
Reviewers

@matthiask matthiask Awaiting requested review from matthiask

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@tim-schilling @pauloxnet @gabbork @filippo-20tab @matthiask