Skip to content

Do not capture exception in Rails runner at_exit if the exit code is 0 / success#1988

Merged
st0012 merged 4 commits intogetsentry:masterfrom
sco11morgan:fix-system-exit
Feb 3, 2023
Merged

Do not capture exception in Rails runner at_exit if the exit code is 0 / success#1988
st0012 merged 4 commits intogetsentry:masterfrom
sco11morgan:fix-system-exit

Conversation

@sco11morgan
Copy link
Copy Markdown
Contributor

Description

Do not capture exception in Rails runner at_exit if the exit code is 0 / success.

Why

Recently a change went in to start capturing a Rails runner exceptions in at_exit #1820

Unfortunately this captures a Sentry alert if a Rails script finishes with exit 0.

Our application has existing scripts that exit with exit 0 and we started seeing Sentry alerts when we upgraded.

@sco11morgan sco11morgan marked this pull request as ready for review January 21, 2023 18:13
@t-h-man t-h-man requested a review from st0012 January 26, 2023 10:57
@codecov
Copy link
Copy Markdown

codecov Bot commented Feb 1, 2023

Codecov Report

Base: 98.63% // Head: 98.56% // Decreases project coverage by -0.07% ⚠️

Coverage data is based on head (84d67fa) compared to base (193446b).
Patch coverage: 74.07% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1988      +/-   ##
==========================================
- Coverage   98.63%   98.56%   -0.07%     
==========================================
  Files         157      157              
  Lines       10009    10035      +26     
==========================================
+ Hits         9872     9891      +19     
- Misses        137      144       +7     
Impacted Files Coverage Δ
sentry-rails/lib/sentry/rails/railtie.rb 98.68% <50.00%> (-1.32%) ⬇️
sentry-rails/spec/sentry/rails_spec.rb 96.13% <76.00%> (-3.23%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

exit exit_code
end

pipe_out.close
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@st0012 do you have opinions on this test, seems a bit like too much extra work

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Note this same test pattern was copied from sentry-raven

Comment thread sentry-rails/spec/sentry/rails_spec.rb Outdated
Copy link
Copy Markdown
Contributor

@st0012 st0012 left a comment

Choose a reason for hiding this comment

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

Thanks for the fix 👍
I think the test complexity is going to be high regardless of the approach when testing at_exit logic end-to-end. So I'm fine leaving it like this.

@st0012 st0012 added this to the 5.8.0 milestone Feb 3, 2023
@st0012 st0012 merged commit 75f289d into getsentry:master Feb 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants