-
Notifications
You must be signed in to change notification settings - Fork 211
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
Update Django/Python support #458
Update Django/Python support #458
Conversation
And copy required pytest fixtures from Celery celery/celery#7077
a44b7a9
to
9033da2
Compare
bf04f86
to
146e65a
Compare
requirements/test.txt
Outdated
@@ -1,4 +1,3 @@ | |||
case>=1.3.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one might deserve its own separate PR... Thoughts?
t/conftest.py
Outdated
""" | ||
return _patching(monkeypatch, request) | ||
|
||
|
||
@pytest.fixture(autouse=True) | ||
def test_cases_shortcuts(request, app, patching): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I can tell, the patching
fixture was only thing that was coming from the case
dependency. I took the current implementation from Celery (found here celery/celery#7077)
Not sure if duplicating these ~100 lines of code is the best way forward but that does the job
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we might be using that from celery instead of case if those are already available there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or we can handle this in a separate PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The version from Celery is defined under the tests directory (celery/t
), which not included in the wheel distribution, so it's not available in the version we have installed... I tried to install from the source distribution, which seems to have the t
directory included, but couldn't get to import the fixture.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I have 2 approaches:
- Extract the commit as is from this PR Remove case dependency and copy Celery fixture #462 - just works but fixture is duplicated with Celery
- Use the fixture from Celery PoC: Remove case dependency and use Celery fixture #461 - not working right now, so I assume it needs more work in Celery... Perhaps move the fixture from
t
to undercelery
to be included in the distribution, but I'm not sure about the side effects of doing so.
Approach 1 is more practical, and can unblock this PR immediately. Approach 2 is more purist but needs more work and guidance from one of the Celery maintainers.
t/conftest.py
Outdated
""" | ||
return _patching(monkeypatch, request) | ||
|
||
|
||
@pytest.fixture(autouse=True) | ||
def test_cases_shortcuts(request, app, patching): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or we can handle this in a separate PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for this PR. I will take parts from this PR for keeping older versions for a new release, to let the old users easier path to upgrade
EDIT: right I just saw that #456 was merged in the meantime... What is your plan in terms of adding and removing support? |
EDIT: perhaps duplicated with #456
Fix #420