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

Allow override_config for pytest #338

Merged

Conversation

TamoshaytisV
Copy link
Contributor

@TamoshaytisV TamoshaytisV commented Aug 11, 2019

override_config is currently possible to apply only to unittest TestCase classes.
This PR adds constance.test.pytest module that can be used as follows

Examples can be found in doc

It's still possible to do import as usual for unittest

from constance.test import override_config

...

@camilonova
Copy link
Member

Vladas thank you for your contribution. Why this is needed?

@TamoshaytisV
Copy link
Contributor Author

TamoshaytisV commented Aug 12, 2019

There are a lot of django projects where i use pytest as a test runner. Django-constance forces me to use TestCase inheritance to make override work
I have the workaround to set CONSTANCE_CONFIG values in test.py settings but often i want to have different overrides for specific methods or classes

@camilonova please let me know if it's clear enough

@camilonova
Copy link
Member

Got it. I'm not able to make a decision here. Maybe @jezdez knows best.

constance/test/base.py Outdated Show resolved Hide resolved
constance/test/base.py Outdated Show resolved Hide resolved
@jezdez
Copy link
Member

jezdez commented Aug 15, 2019

Thanks @camilonova for raising this with me, I agree that this is a non-trivial feature that we need to discuss first.

In general I'm in favor of improving the use of override_config when used with Pytest. But I'm not sure if providing a decorator like override_config fits the best practices of the Pytest community.

E.g. pytest-django didn't implement a custom overrride_settings decorator to override Django's settings but instead chose to implement a Pytest fixture that allows modifying of settings in the test session.

Similarly I would encourage you to implement this feature as a constance or config fixture that would have the ability to override config values during individual tests. I would consider that to be a more practical user experience than having to know which decorator to use.

@TamoshaytisV
Copy link
Contributor Author

TamoshaytisV commented Aug 15, 2019

@jezdez I agree that for pytest it's better to use fixtures or markers but what initially stopped me from implementing in that way is complexity to make it work preserving simplicity of decorator and ability to change config for any scope.
I will investigate more using pytest-django example and will get back with results soon.
Thank you

@jezdez
Copy link
Member

jezdez commented Aug 15, 2019

@jezdez I agree that for pytest it's better to use fixtures or markers but what initially stopped me from implementing in that way is complexity to make it work preserving simplicity of decorator and ability to change config for any scope.

I understand, and appreciate your openness to investigate this further. As always I try to apply the mantra "it's easy to add, but is it also easy to maintain?" to such discussion. Your input and contribution is valued and welcome nevertheless.

I will investigate more using pytest-django example and will get back with results soon.
Thank you

@TamoshaytisV TamoshaytisV changed the title Allow override_config for pytest WIP Allow override_config for pytest Aug 19, 2019
@TamoshaytisV TamoshaytisV changed the title WIP Allow override_config for pytest Allow override_config for pytest Jan 12, 2020
@TamoshaytisV
Copy link
Contributor Author

TamoshaytisV commented Jan 12, 2020

@jezdez @camilonova sorry for being inactive for a long time. I finalized refactoring and feature in general, please review or feel free to assign propper reviewers if you are not available.

yield


class ConstanceConfigWrapper(object):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably the need to have __setattr__ in this class may appear in future but ATM i can't think of many usage cases and all they can be solved using context manager or using fixture and marking tests/classes/modules

constance/test/pytest.py Outdated Show resolved Hide resolved
docs/testing.rst Outdated Show resolved Hide resolved
docs/testing.rst Outdated Show resolved Hide resolved
@marco-silva0000
Copy link

I like what's been done here, I'm using a copy paste on my code and it makes tests really clean.
+1 for this to be merged in

@TamoshaytisV
Copy link
Contributor Author

@jezdez will you be so kind to assign other reviewers if you are busy with smth else?

Copy link
Member

@pawelzar pawelzar left a comment

Choose a reason for hiding this comment

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

Nice work @TamoshaytisV 👍. I have a few suggestions - instead of pytest.mark and ConstanceConfigWrapper and override_config fixture we could define override_config as ContextDecorator which could server both as decorator and context manager.

constance/test/pytest.py Outdated Show resolved Hide resolved
constance/test/pytest.py Outdated Show resolved Hide resolved
Co-Authored-By: Paweł Zarębski <ppjzarebski@gmail.com>
@camilonova
Copy link
Member

Please check the latest changes/comments and I would be happy to merge this.

Thank you all!

@camilonova camilonova assigned camilonova and unassigned jezdez Mar 5, 2020
@TamoshaytisV
Copy link
Contributor Author

TamoshaytisV commented Apr 3, 2020

@pawelzar I used ContextDecorator so now it's possible to use override_config in the same way as it is for unittest + context manager. Nevertheless, I insist to keep fixture and mark so users can use it in pytest way but with less code

@camilonova camilonova merged commit bd8041c into jazzband:master Apr 4, 2020
@camilonova
Copy link
Member

Thank you all

@utapyngo
Copy link
Member

utapyngo commented Apr 7, 2020

Is this going to be released soon?

@camilonova
Copy link
Member

If someone can confirm it works fine if the package is installed from master I can make a release

@TamoshaytisV
Copy link
Contributor Author

TamoshaytisV commented Apr 8, 2020

I've tried it in my project with

-e git+https://github.com/jazzband/django-constance.git#egg=constance[database]

I've got the warning
WARNING: Generating metadata for package constance produced metadata for project name django-constance. Fix your #egg=constance fragments.

In fact, lib was really installed with database backend anyway, so in general, it's working
Can someone please correct my egg= part and also try it as I'm not sure what's the correct way to specify extras_require for github URL requirement?

"""
Validate constance override marker params. Run test with overrided config.
"""
marker = item.get_closest_marker("override_config")
Copy link
Member

Choose a reason for hiding this comment

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

Need something like here (in pytest-django) to prevent #410

Suggested change
marker = item.get_closest_marker("override_config")
if not django_settings_is_configured():
return
marker = item.get_closest_marker("override_config")

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

7 participants