Skip to content

feat(tests): restore manual pid allowance for capturing events#1053

Merged
solnic merged 1 commit into
masterfrom
1052-bring-back-manual-sentrytest-allowance
May 13, 2026
Merged

feat(tests): restore manual pid allowance for capturing events#1053
solnic merged 1 commit into
masterfrom
1052-bring-back-manual-sentrytest-allowance

Conversation

@solnic
Copy link
Copy Markdown
Collaborator

@solnic solnic commented May 12, 2026

Closes #1052

@solnic solnic linked an issue May 12, 2026 that may be closed by this pull request
@solnic solnic changed the title WIP feat(tests): restore manual pid allowance for capturing events May 12, 2026
@solnic solnic force-pushed the 1052-bring-back-manual-sentrytest-allowance branch 2 times, most recently from 4f3c513 to a59a4c5 Compare May 13, 2026 09:24
@solnic solnic marked this pull request as ready for review May 13, 2026 09:26
cursor[bot]

This comment was marked as resolved.

@solnic solnic marked this pull request as draft May 13, 2026 09:44
@solnic solnic force-pushed the 1052-bring-back-manual-sentrytest-allowance branch from a59a4c5 to 365fb15 Compare May 13, 2026 10:13
@solnic solnic marked this pull request as ready for review May 13, 2026 10:51
Comment thread lib/sentry/test.ex
Comment on lines +818 to +822
{:ok, owner_pid} ->
@ownership_server
|> NimbleOwnership.get_owned(owner_pid, %{})
|> Map.get(@collector_key)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Bug: The pattern match for NimbleOwnership.fetch_owner/3 is incorrect. It expects {:ok, owner_pid} but receives {:ok, {pid, metadata}}, causing a runtime error in subsequent calls.
Severity: HIGH

Suggested Fix

Update the case statement in find_collector/0. The success clause should pattern match on {:ok, {owner_pid, _metadata}} to correctly extract the owner_pid. The error clause should match {:error, _reason} to correctly handle cases where no owner is found and avoid a CaseClauseError.

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: lib/sentry/test.ex#L818-L822

Potential issue: The `find_collector/0` function incorrectly pattern-matches the return
value from `NimbleOwnership.fetch_owner/3`. According to NimbleOwnership v1.0
documentation, `fetch_owner/3` returns `{:ok, {owner_pid, metadata}}`. However, the code
at line 818 matches `{:ok, owner_pid}`, which incorrectly binds the variable `owner_pid`
to the tuple `{actual_pid, metadata}`. This tuple is then passed as the `owner` argument
to `NimbleOwnership.get_owned/3`, which expects a PID, leading to a runtime failure.
Additionally, the `case` statement's error handling clause incorrectly matches `:error`
instead of `{:error, reason}`, which will raise a `CaseClauseError` if no owner is
found. This will cause all event collection in test mode to fail.

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

@solnic solnic merged commit 1dd0ef9 into master May 13, 2026
12 checks passed
@solnic solnic deleted the 1052-bring-back-manual-sentrytest-allowance branch May 13, 2026 12:59
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.

Bring back manual Sentry.Test allowance

2 participants