Fix OverflowError on Celery worker #3899
Conversation
guyco33
force-pushed
the
Fix_OverflowError_on_celery_worker
branch
3 times, most recently
from
4872fa0 to
f6cae5a
Compare
guyco33
changed the title
There was a problem hiding this comment.
Looks good, thanks @guyco33.
One last thing - should_schedule_next has a pretty elaborate test suite, and I believe this case should be covered there as well. Can you add a test for this?
guyco33
force-pushed
the
Fix_OverflowError_on_celery_worker
branch
from
f6cae5a to
6c1769a
Compare
| @@ -130,7 +130,7 @@ def test_backoff(self): | |||
| failures=5)) | |||
| self.assertFalse(models.should_schedule_next(two_hours_ago, now, | |||
| "3600", failures=10)) | |||
|
|
|||
| self.assertFalse(models.should_schedule_next(two_hours_ago, now, "3600", failures=32)) | |||
There was a problem hiding this comment.
@guyco33 sorry for being a pain in the ass about this, but I'm a firm believer in one assert per test, and specifically, one logical assert per test.
The two assertions above the one you added are kind of related - they test the flip side of the same conditional, and belong in the same test (although this is debatable). The assertion you added feels like it tests a different concept, and I believe it should be separated.
Also - in the absence self-explanatory code, it's the tests' job to provide documentation, and just asserting "false" when failures=32 feels like a magic number with magic behavior. A dedicated test, with an explanatory test name, would really help here.
There was a problem hiding this comment.
@rauchy I totally agree with you, especially when the tests cover special behaviour of magic numbers and unique corner cases.
I will pick a proper name for a new assertion and will update the PR accordingly
guyco33
force-pushed
the
Fix_OverflowError_on_celery_worker
branch
from
6c1769a to
c8f0927
Compare
|
Test summary
Run details
View run in Cypress Dashboard ➡️ This comment has been generated by cypress-bot as a result of your project's GitHub integration settings. You can manage this integration in your project's settings in the Cypress Dashboard |
|
Thanks @guyco33! |
What type of PR is this? (check all applicable)
Description
next_iteration += datetime.timedelta(minutes=2**failures)start failing when number of failures is larger then 31 and celery worker stops refreshing queries