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

Support database_exists for MS SQL natively (fixes #411) #414

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

abitrolly
Copy link
Contributor

No description provided.

@kvesteri
Copy link
Owner

kvesteri commented May 3, 2020

Before merging this needs unit tests.

@abitrolly
Copy link
Contributor Author

@kvesteri it might already been tested by existing code, but without test coverage it is hard to say for sure.

@kvesteri
Copy link
Owner

kvesteri commented May 5, 2020

The unit tests are not covering MSSQL at the moment. Separate test should be added for this and setup of the travis should be changed to initialize a MSSQL database.

@abitrolly
Copy link
Contributor Author

@kvesteri 35d26fa adds MSSQL initialization to Travis. Why do you think that tests are not executed for MSSQL?

@kvesteri
Copy link
Owner

The tests in https://github.com/kvesteri/sqlalchemy-utils/blob/master/tests/functions/test_database.py do not currently include tests for MSSQL. Thus as part this PR those should be added.

@abitrolly
Copy link
Contributor Author

@kvesteri even if there are no MSSQL specific tests, if MSSQL is set as a default database, the test that cover the essential functionality should cover it. No?

@kvesteri
Copy link
Owner

@abitrolly no unfortunately it doesn't work like that currently. Ideally we would have a test matrix that executes all tests to various different database / sqlalchemy version combination setups.

abitrolly added a commit to abitrolly/sqlalchemy-utils that referenced this pull request May 25, 2020
@abitrolly
Copy link
Contributor Author

@kvesteri added PR for test coverage - PTAL #451

@codecov-commenter
Copy link

codecov-commenter commented Oct 14, 2021

Codecov Report

❗ No coverage uploaded for pull request base (master@b079239). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #414   +/-   ##
=========================================
  Coverage          ?   90.99%           
=========================================
  Files             ?       63           
  Lines             ?     3209           
  Branches          ?        0           
=========================================
  Hits              ?     2920           
  Misses            ?      289           
  Partials          ?        0           

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 b079239...793ff8e. Read the comment docs.

@abitrolly
Copy link
Contributor Author

From https://app.codecov.io/gh/kvesteri/sqlalchemy-utils/blob/793ff8ea5151c75e06f77fcce1e0fa6e3ab84b26/sqlalchemy_utils/functions/database.py (had to login) I see that this part of the code is covered, but it is not clear by which tests.

Need a better coverage report. Perhaps generate one that is directly uploaded to GitHub Pages branch.

@abitrolly
Copy link
Contributor Author

Removed Codecov as it gives less useful info than expected.

@kurtmckee
Copy link
Collaborator

kurtmckee commented Mar 16, 2022

Related issues: #411, #564

@abitrolly
Copy link
Contributor Author

Just to make it obvious, I am not going to invest more time into this PR as I don't deal with MS SQL anymore. Feel free to close, fork or add tests.

@kurtmckee
Copy link
Collaborator

kurtmckee commented Apr 13, 2022

@abitrolly Thanks for the quick response!

@kvesteri I recommend closing this PR.

I found a blog post for how to set up MSSQL in GitHub CI that may allow PR's like this to instantly benefit. I recommend keeping this PR open so @abitrolly's work isn't lost.

@JimiC
Copy link

JimiC commented Oct 19, 2022

Any chance this PR gets merged any time soon?

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