fix: Don't mutate original enabled_environments

ixti commented 5 months ago

Ensure enabled_environments contains test environment without mutating original list. Otherwise it can cause unexpected spec failures in case original config has unfrozen Array, or fail in attempt to mutate said array.

Alternative approach would be to override Sentry::Configuration#dup to dup underlying arrays and hashes.

See: https://github.com/getsentry/sentry-ruby/pull/2269#issuecomment-2002637738

codecov[bot] commented 5 months ago

Codecov Report

Merging #2275 (ebf04a4) into master (ffffce9) will decrease coverage by 0.05%. The diff coverage is 100.00%.

:exclamation: Current head ebf04a4 differs from pull request most recent head a573841. Consider uploading reports for the commit a573841 to get more accurate results

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #2275 +/- ## ========================================== - Coverage 97.61% 97.56% -0.05% ========================================== Files 112 112 Lines 4154 4155 +1 ========================================== - Hits 4055 4054 -1 - Misses 99 101 +2 ``` | [Components](https://app.codecov.io/gh/getsentry/sentry-ruby/pull/2275/components?src=pr&el=components&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=getsentry) | Coverage Δ | | |---|---|---| | [sentry-ruby](https://app.codecov.io/gh/getsentry/sentry-ruby/pull/2275/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=getsentry) | `98.27% <100.00%> (-0.04%)` | :arrow_down: | | [sentry-rails](https://app.codecov.io/gh/getsentry/sentry-ruby/pull/2275/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=getsentry) | `95.05% <ø> (-0.18%)` | :arrow_down: | | [sentry-sidekiq](https://app.codecov.io/gh/getsentry/sentry-ruby/pull/2275/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=getsentry) | `94.70% <ø> (ø)` | | | [sentry-resque](https://app.codecov.io/gh/getsentry/sentry-ruby/pull/2275/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=getsentry) | `90.76% <ø> (ø)` | | | [sentry-delayed_job](https://app.codecov.io/gh/getsentry/sentry-ruby/pull/2275/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=getsentry) | `95.60% <ø> (ø)` | | | [sentry-opentelemetry](https://app.codecov.io/gh/getsentry/sentry-ruby/pull/2275/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=getsentry) | `100.00% <ø> (ø)` | | | [Files](https://app.codecov.io/gh/getsentry/sentry-ruby/pull/2275?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=getsentry) | Coverage Δ | | |---|---|---| | [sentry-ruby/lib/sentry/test\_helper.rb](https://app.codecov.io/gh/getsentry/sentry-ruby/pull/2275?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=getsentry#diff-c2VudHJ5LXJ1YnkvbGliL3NlbnRyeS90ZXN0X2hlbHBlci5yYg==) | `100.00% <100.00%> (ø)` | | ... and [2 files with indirect coverage changes](https://app.codecov.io/gh/getsentry/sentry-ruby/pull/2275/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=getsentry)

st0012 commented 4 months ago

Thanks for the PR, but why did you find it necessary to freeze the enabled_environments array in the first place?

ixti commented 4 months ago

My tests were randomly passing randomly failing. Adding a freeze helped to nail down that Sentry helper was changing configuration inplace - thus specs became order-dependent

sl0thentr0py commented 3 months ago

superseded by #2317

ixti commented 1 month ago

I still think |= better reflects the desire than += :D

Vagab commented 1 month ago

didn’t work for me :(

ixti commented 1 month ago

didn’t work for me :(

Oh. That's weird. Either way, as long as Sentry does not mutate original config - we're good :D

:heart: :heart: :heart:

getsentry / sentry-ruby

fix: Don't mutate original enabled_environments #2275

Codecov Report