Skip to content

Support reset_sequences #308

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

Closed
wants to merge 20 commits into from
Closed

Conversation

cb109
Copy link
Member

@cb109 cb109 commented Jan 16, 2016

This adds support for using the reset_sequences feature of Django's TransactionTestCase.

It will try to reset all automatic increment values before test execution, if the database supports it. This is useful for when you have tests that rely on such values, like ids or other primary keys.

  • Use as a mark option:
@pytest.mark.django_db(transaction=True, reset_sequences=True)
def test_something():
    pass
  • Use as a fixture:
@pytest.fixture
def setup(reset_sequences_db):
    pass

Please let me know your thoughts.

``transactional_db``, with additional support for reset of auto increment
sequences (if your database supports it). This is only required for
fixtures which need database access themselves. A test function would
normally use the :py:func:`~pytest.mark.django_db` mark to signal it
Copy link
Contributor

Choose a reason for hiding this comment

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

s/mark.django_db/mark.reset_sequences_db/

Copy link
Member Author

Choose a reason for hiding this comment

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

I am sorry, I don't understand 😕 Is that sphinx syntax?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, sed.. ;)
Change "mark.django_db" to "mark.reset_sequences_db".

It seems you have copied it from the paragraph above, but it should be changed here - if I understand it correctly.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ahh okay 😌 ! There is only one marker (django_db), but it has some optional keywords that make the difference. I updated that section of the docs to make it clear which one to use for a test to get the same effect as using the fixture.

@blueyed
Copy link
Contributor

blueyed commented Jan 16, 2016

Awesome, thanks!

@cb109
Copy link
Member Author

cb109 commented Jan 17, 2016

Thanks for your review @blueyed 🙇 !

@das-g
Copy link

das-g commented Apr 5, 2016

Any news on this?

@pelme
Copy link
Member

pelme commented Jun 22, 2016

Thanks for the PR! It mostly looks good and this would be a very useful feature.

A couple of notes/things that I think should be changed before merge:

This PR changes the return value of db and transactional_db to return the Django TestCase instance that is used internally. I don't think this is the right thing to do - you ask for the db fixture and you get a testcase instance - it does not make sense to me. I guess this is done to be able to test this. However, I think this can be tested with a test that calls runpytest, that just creates a bunch of Item objects and look at their pk to make sure they start at 1.

@cb109
Copy link
Member Author

cb109 commented Jun 22, 2016

Hi Andreas, you are probably right about me taking a nasty shortcut there 😊 I will look into it and try to resolve it as you proposed. Thanks for the review 🙇

@blueyed blueyed mentioned this pull request Jun 24, 2016
1 task
@blueyed
Copy link
Contributor

blueyed commented Jun 24, 2016

@cb109
Great. I've just created #353 (from #331 - serialized_rollback feature) and noticed that they touch the same code. Do you plan to work on this today/tomorrow?
@pelme and me are at the pytest sprint together and would be glad to help out with getting this merged.

@cb109
Copy link
Member Author

cb109 commented Jun 25, 2016

I might find time to work at this tomorrow, but not sure. If you want to do it yourself, please go ahead! I hope you enjoy the sprint guys, give a shout to @hackebrot for me please 😄

@hackebrot
Copy link
Member

@cb109 💩

@cb109
Copy link
Member Author

cb109 commented Jun 25, 2016

👊 😆

@pelme
Copy link
Member

pelme commented Jun 28, 2016

@cb109 feel free to continue working in this, it would for sure be a useful feature! :)

cb109 added 2 commits July 1, 2016 18:37
This breaks the test for the reset_sequences feature.
We actually check for the resulting sequence ids now instead of internal pytest-django-attributes.
On the flipside this test must now be skipped if the current database does not support the feature.
@cb109
Copy link
Member Author

cb109 commented Jul 3, 2016

I refactored the test to use runpytest and check actual database ids instead of internal pytest-django values. That test should be skipped for databases that do not support it (like sqlite3). I had some issues getting postgresql green with this, but it now is on my machine. Would like to see what the big testsuite on travis says @pelme is there anything I must do to trigger it or will it just test the commits at some later point?

@pelme
Copy link
Member

pelme commented Jul 3, 2016

@cb109 Thanks for keeping up the work in this PR!

I think the Travis builds have not been run because automatic merge with master failed -- can you try to merge with the latest master and push again to the PR and see if it gets picked up?

I think the fixture should be renamed to django_db_reset_sequences. In the upcoming release I plan to rename and deprecate the old fixture names (but still keep the old names for compatibility for quite a while) to make it more uniform, i.e. we will have
db -> django_db, transactional_db -> django_db_transactional and then we will have django_db_reset_sequences and django_db_serialized_rollback (PR #331).

@cb109
Copy link
Member Author

cb109 commented Jul 4, 2016

@pelme Alright I will rename it once travis is green. It is weird to see some postgresql configurations succeed and others fail, no idea yet what the problem is.

@cb109
Copy link
Member Author

cb109 commented Nov 2, 2017

Well, being honest with myself I will probably not get back to this, so let's close it off to not be a burden for you guys. As far as I remember some travis tests where failing and I couldn't figure out why, while the actual feature seemed only supported by very few database setups on top.

I have since come to write tests to not rely on ids at all, which I guess is good practice anyway, so there isn't much motivation left to finish this off. Sorry and thanks for your help on this nonetheless!

@cb109 cb109 closed this Nov 2, 2017
@sliverc sliverc mentioned this pull request Mar 16, 2018
@sliverc sliverc mentioned this pull request Jun 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants