Skip to content

test: isolate viewer context between pytest tests - #129727

Merged
sehr-m merged 2 commits into
masterfrom
sehrm/isolate-test-viewer-context
Nov 3, 2026
Merged

sehr-m merged 2 commits into
masterfrom
sehrm/isolate-test-viewer-context

Conversation

@sehr-m

@sehr-m sehr-m commented Nov 3, 2026

Copy link
Copy Markdown
Member

Tests that directly call request handlers can update the ViewerContext ContextVar without entering the middleware scope that restores it. The value then survives into later tests in the same worker. After #129261, a full backend run observed UNKNOWN actors in statistical-detector tests that expected organization-scoped SYSTEM actors; the PR's selective run did not include those detector tests. That change was subsequently reverted.

Add an autouse fixture that starts each test with no ViewerContext and restores the incoming value using its ContextVar token in a finally block. A subprocess regression checks isolation after both a successful UNKNOWN-context mutation and a failing USER-context mutation, and verifies restoration of an outer SYSTEM context at session exit. This prepares test isolation without reintroducing the reverted Slack/indexing change or changing production behavior.

Validation:

  • Pytest infrastructure and ViewerContext suites: 51 passed.
  • Regression fails with the isolation fixture disabled and passes with it enabled.
  • Required prek checks and commit/push hooks passed.
  • Attempted the original Slack-validation → statistical-detector pair against the reverted Slack code, but local setup stopped on an outdated SCM dependency (missing GitGrantKeyError), before reproducing the original assertions.

Production assessment: normal HTTP handling encloses Slack validation in ViewerContextMiddleware's restoring scope, so the cross-test contamination is not evidence of cross-request production leakage. Direct calls outside a restoring scope can still retain context, and the detector's existing handling of inherited UNKNOWN contexts is unchanged. This PR addresses test isolation only.

@sehr-m
sehr-m requested a review from a team as a code owner November 3, 2026 18:39
@sehr-m sehr-m added the Trigger: Override Selective Testing Run the full test suite; necessary in cases where selected tests are flaky due to reshuffling label Nov 3, 2026
@github-actions github-actions Bot added the Scope: Backend Automatically applied to PRs that change backend components label Nov 3, 2026
@sehr-m
sehr-m merged commit d2dfc7f into master Nov 3, 2026
97 checks passed
@sehr-m
sehr-m deleted the sehrm/isolate-test-viewer-context branch November 3, 2026 21:15
sehr-m added a commit that referenced this pull request Nov 3, 2026
…ion (#129729)

Stacked on #129727. Merge the test-isolation fix first.

Reapply #129261, which was reverted after full backend CI observed
UNKNOWN viewer actors in statistical-detector tests. Slack validation
assigns the authenticated linked human as the USER actor; bot and
unlinked requests remain UNKNOWN, preserving already resolved scope and
respecting SENTRY_VIEWER_CONTEXT_ENABLED. Knowledge indexing preserves a
valid USER actor only within the target organization, otherwise scopes
the background operation to a SYSTEM actor and restores the incoming
context afterward.

The original failure is now reproduced locally: running
SlackRequestTest::test_validate_using_sdk followed by
test_detect_function_change_points_groups_viewer_context_by_organization
fails with UNKNOWN instead of SYSTEM and organization_id=None when
#129727's isolation fixture is disabled. The same pair passes with it
enabled.

Validation:
- Both changed test files plus the two originally failing
statistical-detector tests: 76 passed, 1 xfailed.
- Original two-test reproduction: 1 failed / 1 passed without isolation;
2 passed with isolation.
- Required prek checks passed.
- Current SCM and protocol packages were loaded from a temporary
directory for local testing without changing the shared virtual
environment.

Only adjustments from the original patch are resolving conflicts with
newer test imports/type annotations. This PR intentionally retains the
original runtime behavior so full backend CI can validate it on top of
the test-isolation fix. The draft requests the full suite via Trigger:
Override Selective Testing.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Scope: Backend Automatically applied to PRs that change backend components Trigger: Override Selective Testing Run the full test suite; necessary in cases where selected tests are flaky due to reshuffling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants