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

on_commit support #752

Closed
tolomea opened this issue Jul 27, 2019 · 4 comments · Fixed by #933
Closed

on_commit support #752

tolomea opened this issue Jul 27, 2019 · 4 comments · Fixed by #933
Labels
enhancement help wanted Help us implement this! Feel free to ask for guidance.

Comments

@tolomea
Copy link

tolomea commented Jul 27, 2019

It would be nice if pytest-django had on_commit support that didn't require a django_db(transaction=True) mark, something like this maybe.
https://medium.com/gitux/speed-up-django-transaction-hooks-tests-6de4a558ef96

@tolomea
Copy link
Author

tolomea commented Jul 27, 2019

For my particular usecase my task system submits tasks in on commit. So if we error out of a transaction we don't run any associated tasks.
In tests I have helpers to run some or all of the queued events.
In those helpers I also have an assert that fails when in a transaction, this exists to catch being in a test but no a transaction test, because I have on so many occasions spent so much time trying to work out where the hell my task has disappeared to.

It would be nice if there was a django_db variant that just ran the on_commit code at the same place it would run if transaction=True. It would also be nice if it had roughly equivalent performance as transaction=False so I could just blanket use it everywhere.

@trawick
Copy link

trawick commented Nov 21, 2020

FWIW for testing a scenario where on_commit calls mytask.delay(), I was able to validate the task invocation by patching on_commit like so:

with mock.patch('mypackage.tasks.mytask.delay') as mock_task_delay:
    with mock.patch('mypackage.models.transaction.on_commit', new=lambda fn: fn()):
        mymodel.save()
        assert mock_task_delay.call_count == 1
        ...

@tolomea
Copy link
Author

tolomea commented Nov 22, 2020

Since I created this issue I've also come across https://pypi.org/project/django-capture-on-commit-callbacks/

@trawick if you just want to validate that the call happens that is a simple and effective soution.
If you want to actually run the task code it has the downside that the task is scheduled immediately and so depending on how tasks are run in your tests it might fail intermittently if it depends on operations done later in the transaction.

@bluetech
Copy link
Member

bluetech commented May 7, 2021

Django 3.2 added TestCase.captureOnCommitCallbacks. It should be straightforward enough to add to pytest-django. I would use the django_assert_num_queries fixture as a template.

I would be happy for a PR if anyone would like to work on it!

@bluetech bluetech added the help wanted Help us implement this! Feel free to ask for guidance. label May 7, 2021
bluetech added a commit to bluetech/pytest-django that referenced this issue May 24, 2021
Similar to Django's `TestCase.captureOnCommitCallbacks`. Documentation
is cribbed from there.

Fixes pytest-dev#752.
bluetech added a commit to bluetech/pytest-django that referenced this issue May 24, 2021
Similar to Django's `TestCase.captureOnCommitCallbacks`. Documentation
is cribbed from there.

Fixes pytest-dev#752.
bluetech added a commit to bluetech/pytest-django that referenced this issue May 24, 2021
Similar to Django's `TestCase.captureOnCommitCallbacks`. Documentation
is cribbed from there.

Fixes pytest-dev#752.
bluetech added a commit that referenced this issue May 25, 2021
Similar to Django's `TestCase.captureOnCommitCallbacks`. Documentation
is cribbed from there.

Fixes #752.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement help wanted Help us implement this! Feel free to ask for guidance.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants