Skip to content

Conversation

@yuvmen
Copy link
Member

@yuvmen yuvmen commented Nov 10, 2025

taking over #102503

Comment on lines 491 to 501
assert nodestore.backend.get(event_node_id)
assert self.select_error_events(self.project.id) == expected_error

# The Issue Platform group and occurrence have been deleted
assert not Group.objects.filter(id=issue_platform_group.id).exists()
# assert not nodestore.backend.get(occurrence_node_id)
assert self.select_issue_platform_events(self.project.id) is None

@mock.patch("sentry.deletions.tasks.nodestore.bulk_snuba_queries")
def test_issue_platform_batching(self, mock_bulk_snuba_queries: mock.Mock) -> None:
# Patch max_rows_to_delete to a small value for testing
Copy link

Choose a reason for hiding this comment

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

Bug: The select_rows() method hardcodes Dataset.IssuePlatform.value, failing to dynamically select the correct dataset based on the provided entity.
Severity: HIGH | Confidence: 1.00

🔍 Detailed Analysis

The select_rows() method in tests/sentry/deletions/test_group.py hardcodes dataset=Dataset.IssuePlatform.value for all queries. This ignores the entity parameter, leading to incorrect data retrieval when EntityKey.Events.value is passed. Error events, which correspond to EntityKey.Events.value, must be queried from Dataset.Events.value. This mismatch will cause queries for error events to return no results or incorrect results, leading to assertion failures in tests like line 467.

💡 Suggested Fix

Modify select_rows() to map the entity parameter to the correct dataset. If entity is EntityKey.Events.value, use Dataset.Events.value; otherwise, use Dataset.IssuePlatform.value for EntityKey.IssuePlatform.value.

🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: tests/sentry/deletions/test_group.py#L441-L501

Potential issue: The `select_rows()` method in `tests/sentry/deletions/test_group.py`
hardcodes `dataset=Dataset.IssuePlatform.value` for all queries. This ignores the
`entity` parameter, leading to incorrect data retrieval when `EntityKey.Events.value` is
passed. Error events, which correspond to `EntityKey.Events.value`, must be queried from
`Dataset.Events.value`. This mismatch will cause queries for error events to return no
results or incorrect results, leading to assertion failures in tests like line 467.

Did we get this right? 👍 / 👎 to inform future reviews.

Copy link
Contributor

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

Bug: Incorrect Group Type Causes Batching Failure

The test sets the Group type field to GroupCategory.FEEDBACK (value 6), but the Group model's type field expects a group type ID, not a category. This should be FeedbackGroup.type_id (value 6001) to properly represent a feedback issue type. Using the category value instead will likely cause the batching logic being tested to fail or behave incorrectly since it won't match actual feedback groups.

tests/sentry/deletions/test_group.py#L512-L516

# Set times_seen for each group
Group.objects.filter(id=group1.id).update(times_seen=3, type=GroupCategory.FEEDBACK)
Group.objects.filter(id=group2.id).update(times_seen=1, type=GroupCategory.FEEDBACK)
Group.objects.filter(id=group3.id).update(times_seen=3, type=GroupCategory.FEEDBACK)
Group.objects.filter(id=group4.id).update(times_seen=3, type=GroupCategory.FEEDBACK)

Fix in Cursor Fix in Web


Copy link
Contributor

@cvxluo cvxluo left a comment

Choose a reason for hiding this comment

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

the test seems to be identical to the one removed in #102354, do we know what was causing it to fail?

@yuvmen
Copy link
Member Author

yuvmen commented Nov 10, 2025

armen mentions it on the PR this is copied from, this should be fixed after #102369, opened a new PR becuse

  1. I want Armen to be able to approve it after I took over
  2. the other PR has a bunch of my attempts to refactor and get fancy which I am pulling back from

@yuvmen yuvmen force-pushed the yuvmen/add-issue-platform-group-deletion-test branch from a0f6558 to 9a12aae Compare November 10, 2025 18:38
@codecov
Copy link

codecov bot commented Nov 10, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@             Coverage Diff             @@
##           master   #103075      +/-   ##
===========================================
+ Coverage   78.24%    80.64%   +2.39%     
===========================================
  Files        9200      9205       +5     
  Lines      392932    393467     +535     
  Branches    24992     24992              
===========================================
+ Hits       307461    317312    +9851     
+ Misses      85045     75729    -9316     
  Partials      426       426              

@yuvmen yuvmen force-pushed the yuvmen/add-issue-platform-group-deletion-test branch from c1d0e7a to a2f68a0 Compare November 10, 2025 21:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants