refactor(tests): more test stability fixes and improvements#1035
Merged
refactor(tests): more test stability fixes and improvements#1035
Conversation
f1defe3 to
0e1c0b9
Compare
f21c4fb to
931dd1c
Compare
18f0729 to
2f6bc7e
Compare
7b1481c to
712c7d0
Compare
712c7d0 to
dd59cec
Compare
dd59cec to
aef3eb5
Compare
Make the Bypass DSN override in Sentry.Test.Registry unconditional so an externally-configured DSN (e.g. leaking from SENTRY_DSN in the environment) can never cause the test suite to ship synthetic events to a real Sentry endpoint. Emit a Logger.warning when an override replaces a previously-configured DSN so the developer sees exactly what is being replaced and why. Add a catch-all clause to maybe_warn_about_dsn_override/1 so an unexpected DSN shape cannot crash registry init and take down the test harness. Expose the function (@doc false) so the warning path can be covered directly, and add a regression test asserting both the warning-emitted and silent cases. Document the unconditional DSN override in the CHANGELOG since test_mode: true suites that intentionally point at a local Sentry/mock collector are affected. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
aef3eb5 to
acfbe2d
Compare
0a2d87a to
0a5eee2
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 99019f2. Configure here.
Tighten the ancestor walk used by Sentry.Test.Config so per-test overrides resolve predictably under async tests and edge OTP scenarios. find_via_ancestors/2 previously returned :default both when no ancestor had set a key and when multiple ancestors disagreed on one. The latter is a load-bearing safety property of per-test isolation, but a silent fallback to global config produced passing-but-wrong assertions with no hint that ambiguity was detected. Distinguish zero from ambiguous and emit a Logger.warning on the ambiguous branch. collect_ancestors/3 threaded the `seen` set on the outer call, but each recursive branch then walked independently, allowing siblings to re-enter each other's subtrees. Thread `seen` through the reduce so each ancestor is visited at most once across the entire walk, and build the collected list with Enum.reverse/++ for idiomatic list construction. ancestors_of/1 matched a wildcard on Process.info/2 that swallowed any unexpected return shape. Match nil explicitly so future OTP changes surface instead of silently regressing, and gate the Process.info(pid, :parent) path behind an :otp_25_plus tag since that return is only guaranteed on OTP 25+ (the project declares elixir ~> 1.13 which is compatible back to OTP 22). Add regression coverage: conflicting overrides on two ancestors of a manually-spawned descendant, and an orphan-process leak test for a pid whose chain has neither \$callers nor \$ancestors linking back to the test — the exact scenario that motivated dropping the old resolve_from_active_scopes/1 path. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Raise the shared receive_timeout used by Sentry.Case so async test suites don't flake when an envelope takes longer than the old default to reach the collecting test process under load. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The handler-level :enable_logs override added alongside the Sentry.Test config-isolation rework previously accepted any value and silently coerced non-booleans to the global default. Reject non-boolean overrides with a clear error so misconfigured handler configs surface at attach time, not as missing logs in production. Add positive-path coverage for the inverse case the rejection test doesn't cover: global config has enable_logs: true and the handler-level :enable_logs: false override wins. That's the regression most likely to resurface silently if precedence between the handler override and the global Config.enable_logs?() is reordered. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
no idea why this is needed TBF
99019f2 to
fd7dd65
Compare
sl0thentr0py
approved these changes
Apr 16, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

I had to make more tweaks for simplicity and better stability. There are cases where it simply must not be allowed to use
async: trueas tests are relying on changing configuration of globals that we don't own and control.It's worth mentioning that our end-goal here should be to introduce a proper client/transport split like we have in other SDKs. This would be a cleaner architecture with easier testability being a nice side-effect. Leaving this for 14.0.
This PR can be reviewed commit-by-commit.
Summary
Sentry.Testisolation around an explicitScopeabstraction. Replace the flat:persistent_termlayout (one key per{pid, config_key}plus separate scope-marker, allow-map, and counter keys) with aSentry.Test.Scopevalue struct (one entry per test) and aSentry.Test.Scope.Registrythat owns lifecycle and resolution.Sentry.Test.Configbecomes a thin adapter —namespace/1,put/1,allow/2keep their signatures.resolve_from_active_scopes/1routed any unclaimed pid to whichever test happened to be active, which caused stray events (e.g. OTP's async callback-crashed meta-event) to leak across tests. The new resolution tries three precise strategies and stops: walk[pid | $callers], walk$ancestorsagainst each scope'sowner_pid, walk[pid | $ancestors]against each scope'sallowed_pids. Unmatched pids fall through to:default.Sentry.Test.Config.put/1auto-soft-allows:logger,:logger_sup, andSentry.Supervisoronto the calling scope so downstream suites get per-test routing with zero opt-in. Soft-allow silently skips when a live peer scope already owns a global (async-safe); explicitallow/2keeps strict conflict detection viaScope.AllowConflictError.:persistent_termkey. Lookups no longer iterate active scopes;lookup_allow_owner/1andfetch/1are direct gets. This keeps Sentry config reads cheap under async load.collect_ancestors/3across siblings, tightenancestors_of/1, gate the OTP 25+ parent-pid probe behind a tag.receive_timeoutinSentry.Caseso async suites don't flake under load.:enable_logshandler-config hardening. Reject non-boolean overrides at attach time; add positive-path coverage for handler-levelenable_logs: falseoverriding a globalenable_logs: true.Sentry.Test.Registry. Make the Bypass DSN override unconditional so a leakedSENTRY_DSNcan't ship synthetic events to a real endpoint; warn when overriding; harden with a catch-all and regression tests.