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

Reset sequences only for tables that exists. #79

Merged
merged 2 commits into from Sep 9, 2012
Merged

Reset sequences only for tables that exists. #79

merged 2 commits into from Sep 9, 2012

Conversation

shacka
Copy link

@shacka shacka commented Jun 8, 2012

If using multiple databases setup we need to make sure we aren't trying
to reset sequences that doesn't exists in particular DB.

I've faced problem with using REUSE_DB=1 with multidb setup. The problem was that NoseTestSuiteRunner.setup_databases() was trying to execute reset statements for all the models on every connection. My patch adds a check that the model's table is present in DB before executing reset statement.

If using multiple databases setup we need to make sure we aren't trying
to reset sequences that doesn't exists in particular DB.
@erikrose
Copy link
Contributor

erikrose commented Jun 8, 2012

Good catch! I'll give it a try and merge it in.

I'm trying to improve test coverage in django-nose. You don't have a test case lying around, do you?

@shacka
Copy link
Author

shacka commented Jun 8, 2012

Unfortunately I don't have any. I can write some, however I don't see 'django_nose/tests', where should I put test for this piece of code?

@erikrose
Copy link
Contributor

erikrose commented Jun 8, 2012

If you're willing, that's awesome! Tests are a little weird in django-nose at the moment. Essentially, we have a shell script that runs tests against a dedicated test app over and over, with various settings. So for now, your tests should go in that test app.

You can make a fresh settings file (copying settings_with_plugins.py, for example), define multiple DBs in there, and add a line to runtests.sh saying --settings=testapp.settings_with_multi_db. The actual assertions can go in testapp/tests/test_databases.py.

Does that make sense? Thanks again!

@shacka
Copy link
Author

shacka commented Jun 10, 2012

Hi Eric, I must agree that tests in the project are weird :) I was trying to follow your guide, create testapp.settings_with_multi_db and put my tests into testapp/tests. However to much of other things needs to be done to create a test case I need:

  • we need to have some models in testapp
  • tests should be run with REUSE_DB=1 and DBs should be already present at that point

Also there was a few other issues. That's why I've tried to go with unittests for my method only. Please take a look and tell me what you think.

@shacka
Copy link
Author

shacka commented Jul 9, 2012

Any chance that this will be merged into master?

@erikrose
Copy link
Contributor

erikrose commented Jul 9, 2012

I promise it. Sorry I've been unresponsive; blame the startup life and an overabundance of good patches!

@tehranian
Copy link

+1 for this pull request. thanks!

@erikrose
Copy link
Contributor

Just have to find an hour or 3 to pay attention to this! :-)

@camilonova
Copy link
Member

+1 I have tried with the current repository status and it works like charm.

@erikrose is ready to check-in

@erikrose
Copy link
Contributor

erikrose commented Sep 9, 2012

The new tests mock the model-loading cache but don't put it back afterward. Working on a fix.

@erikrose erikrose merged commit a08d977 into jazzband:master Sep 9, 2012
@shacka
Copy link
Author

shacka commented Sep 10, 2012

Thanks!

@erikrose
Copy link
Contributor

Thank you!

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

4 participants