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.

Sign up for GitHub

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

Fix OverflowError on Celery worker #3899

Merged
merged 1 commit into from
Jun 16, 2019
Merged

Fix OverflowError on Celery worker #3899

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion redash/models/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -354,7 +354,10 @@ def should_schedule_next(previous_iteration, now, interval, time=None, day_of_we
next_iteration = (previous_iteration + datetime.timedelta(days=days_delay) +
datetime.timedelta(days=days_to_add)).replace(hour=hour, minute=minute)
if failures:
next_iteration += datetime.timedelta(minutes=2**failures)
try:
jezdez marked this conversation as resolved.
Show resolved Hide resolved
next_iteration += datetime.timedelta(minutes=2**failures)
except OverflowError:
return False
return now > next_iteration


Expand Down
5 changes: 5 additions & 0 deletions tests/test_models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,11 @@ def test_backoff(self):
self.assertFalse(models.should_schedule_next(two_hours_ago, now,
"3600", failures=10))

def test_next_iteration_overflow(self):
now = utcnow()
two_hours_ago = now - datetime.timedelta(hours=2)
self.assertFalse(models.should_schedule_next(two_hours_ago, now, "3600", failures=32))
Copy link
Contributor

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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



class QueryOutdatedQueriesTest(BaseTestCase):
# TODO: this test can be refactored to use mock version of should_schedule_next to simplify it.
Expand Down