-
Notifications
You must be signed in to change notification settings - Fork 549
Back port __wrapped__ attribute for functools.wrap to Python 2 #475
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
Conversation
Where do you need this? |
@untitaker My apologies, I accidentally opened this PR and isn't ready yet. I'm the author of celery-once and this PR is related to #421. For context, behind the scenes, celery once attempts to inspect the arguments of the task function to generate a "key" to lock the task. |
Got it. Conceptually I would approve this PR. Could you add a test that loads and uses celery-once? Also please move this new Ping me when it's ready :) |
Hi @untitaker the code should be ready for a review. I'm not sure exactly why the tests are failing on Travis failed, any insight? |
…ameronmaske/celery-once/issues/105) Add tests for celery_once. Move re-usable pytest fixture for celery into a common conftest.py.
@untitaker Thanks for the advice on the tests! Your suggestion are implemented and this should be ready for re-review. |
4cf0586
to
1bb851d
Compare
@cameronmaske I don't quite understand the test, but it still seems to pass if I revert all your changes in |
Also sorry for the slow review! |
@untitaker Just to check, with the reverted changes, were the tests run on Python 3 or 2? From my understanding, the tests should still pass on Python 3 without the changes, but fail on 2. The tests are a bit obtuse, but they are checking that backend is called with a correctly generated key from the example's tasks name and arguments. def dummy_task(x, y):
pass Should become qo_tests.integrations.celery.test_celery_once.dummy_task_x-1_y-1
# < Name assigned by celery to task> . (<Argument Key > <Arugment Value>, <Argument Key> <Arugment Value>,.... In this case, I put a dummy backend (simpler too test), but normally, this would be a redis or file backend to check the locks. |
I was running this:
...since you only use celery-once in celery 3. |
@cameronmaske any update on this? Right now the tests don't seem useful IMO |
I need to investigate this further on my end. I'm currently travelling and away from my development environment, will follow up when I return later next week. |
Just for clarification: This changes still will be needed? Even after upgrading celery-once to 3.0.1? If so, anything I can help here? Thanks. |
@Marcelo-Theodoro I would love it if somebody took over this PR and fixed merge conflicts + fixed the tests to actually fail when the bugfix is not applied I can't answer the first question, this is something for @cameronmaske |
@untitaker Sorry for the delayed follow up. Had a chance to test this, and it appears that switching from |
No description provided.