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

fix(escalating_issues): Fix tests regression #48272

Merged
merged 2 commits into from
May 2, 2023

Conversation

armenzg
Copy link
Member

@armenzg armenzg commented May 1, 2023

There are some changes I picked up in #48247 which disable tests for DailyGroupCountsEscalating since we need TestCase to be in the inheritance tree.

There are some changes I picked up in #48247 which disable running tests since we need to import `TestCase` directly.
from sentry.types.group import GroupSubStatus
from sentry.utils.cache import cache
from sentry.utils.snuba import to_start_of_hour

TIME_YESTERDAY = (datetime.now() - timedelta(hours=24)).replace(hour=6)


class BaseGroupCounts(SnubaTestCase): # type: ignore[misc]
class BaseGroupCounts(TestCase): # type: ignore[misc]
Copy link
Member Author

Choose a reason for hiding this comment

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

DailyGroupCountsEscalating inherits from BaseGroupCounts

If TestCase is not in the inheritance tree it stops executing.

This is why the tests got regressed in #48228 yet they did not turn the checks red.

@@ -211,7 +210,7 @@ def test_is_escalating_issue(self, record_mock: MagicMock) -> None:

# Test cache
assert (
cache.get(f"daily-group-count:{group_escalating.project.id}:{group_escalating.id}")
cache.get(f"hourly-group-count:{group_escalating.project.id}:{group_escalating.id}")
Copy link
Member Author

Choose a reason for hiding this comment

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

This test started failing with the changes in #48228 but as I explained in the previous comment, we disabled the tests.

@@ -241,18 +240,11 @@ def test_not_escalating_issue(self) -> None:

@freeze_time(TIME_YESTERDAY.replace(minute=12, second=40, microsecond=0))
def test_hourly_count_query(self) -> None:
"""Test the hourly count query only aggregates events from today"""
Copy link
Member Author

Choose a reason for hiding this comment

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

I do not think this test is actually regressed but I cleaned it up a bit.

@armenzg armenzg marked this pull request as ready for review May 1, 2023 20:17
@armenzg armenzg requested review from a team, jangjodi and NisanthanNanthakumar May 1, 2023 20:17
@codecov
Copy link

codecov bot commented May 1, 2023

Codecov Report

Merging #48272 (20af4bf) into master (4bcd3dc) will increase coverage by 0.01%.
The diff coverage is n/a.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #48272      +/-   ##
==========================================
+ Coverage   80.91%   80.93%   +0.01%     
==========================================
  Files        4773     4773              
  Lines      201862   201862              
  Branches    11521    11521              
==========================================
+ Hits       163345   163370      +25     
+ Misses      38262    38237      -25     
  Partials      255      255              

see 3 files with indirect coverage changes

from sentry.types.group import GroupSubStatus
from sentry.utils.cache import cache
from sentry.utils.snuba import to_start_of_hour

TIME_YESTERDAY = (datetime.now() - timedelta(hours=24)).replace(hour=6)


class BaseGroupCounts(SnubaTestCase): # type: ignore[misc]
class BaseGroupCounts(TestCase): # type: ignore[misc]
Copy link
Member

Choose a reason for hiding this comment

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

You need SnubaTestCase here as well if you're using snuba in these tests. Otherwise we won't reset snuba afterwards and the tests will be unstable

Copy link
Member Author

Choose a reason for hiding this comment

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

That's good to know.

I will follow up on this.

Copy link
Member Author

Choose a reason for hiding this comment

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

#48330 fixes the problem with SnubaTestCase not executing.

#48332 is the follow up to your comment.

@armenzg armenzg merged commit db11a03 into master May 2, 2023
@armenzg armenzg deleted the armenzg/escalating/fix-tests branch May 2, 2023 12:34
armenzg added a commit that referenced this pull request May 2, 2023
In PR #48272 we noticed that just inheriting from `SnubaTestCase` would no make the tests execute.

Making `SnubaTestCase` inherit from `django.TestCase` fixes this.
@github-actions github-actions bot locked and limited conversation to collaborators Jun 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants