-
-
Notifications
You must be signed in to change notification settings - Fork 494
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Sentry::TestHelper
#1773
Add Sentry::TestHelper
#1773
Conversation
@sl0thentr0py If we're going to do this, I think the Wdyt? |
I spent an hour trying the separation and I think the ideal modeling would be:
But that'll require changing the SDK's modeling standard. So I'll leave it for the SDK team to decide and just add comments about the |
@st0012 I kinda agree with you that this inheritance causes coupling and confusion. But the other side is that even in the relay protocols we have a single Just another point to consider, I still haven't made up my mind / thought through completely on what I'd like to have here tbh. |
Oh cool. So on the server side it's actually modeled better, but the SDK side's spec and implementation are the "dirty" ones? |
Even between SDKs there's a fair amount of inconsistency.
So yes I think basically your comment is the way to go. I'll put it on our backlog but dunno when we'll actually get to it. :) |
@sl0thentr0py Since that looks like the right direction, I'd be happy to do it this week. Otherwise, it'd become another debt if we exposed them with all the test helpers. |
ok so we do a breaking release then? |
Personally I don't think so as users shouldn't have any actual usage to touch From my understanding, the behavioral changes would be
So I don't think it's more like a private API change and doesn't need a breaking release. Wdyt? |
sounds good! I'm slightly worried because the specs will also change but we'll just have to review carefully. But you're right that no end-user API is changing. 👍 |
Codecov Report
@@ Coverage Diff @@
## master #1773 +/- ##
==========================================
+ Coverage 98.38% 98.40% +0.01%
==========================================
Files 146 148 +2
Lines 8744 8825 +81
==========================================
+ Hits 8603 8684 +81
Misses 141 141
Continue to review full report at Codecov.
|
56b26da
to
07e5082
Compare
This pull request has gone three weeks without activity. In another week, I will close it. But! If you comment or otherwise update it, I will reset the clock, and if you label it "A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀 |
@st0012 do you know what we wanna do with this? Should it go out with the next release or do we wait till later? |
Since some users start asking for the I like the idea that SDK provides some helpers for testing and encourage users to test the SDK on CI. In the long-term it should reduce the chance for SDK bug to break on production and will also help issue reports. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@st0012 I merged master and updated some specs, do you want to add a changelog before merging?
@sl0thentr0py can we wait for the close api first? I want to refresh the implementation here with it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
It seems as if changes introduced by getsentry/sentry-ruby#2206 were interfering with Sentry test teardown. Fortunately, getsentry/sentry-ruby#1773 adds some test helpers which manage Sentry test setup/teardown, which resolves this issue.
It seems as if changes introduced by getsentry/sentry-ruby#2206 were interfering with Sentry test teardown. Fortunately, getsentry/sentry-ruby#1773 adds some test helpers which manage Sentry test setup/teardown, which resolves this issue.
It seems as if changes introduced by getsentry/sentry-ruby#2206 were interfering with Sentry test teardown. Fortunately, getsentry/sentry-ruby#1773 adds some test helpers which manage Sentry test setup/teardown, which resolves this issue.
As explained in #1680, the SDK should encourage testing the error capturing logic by providing useful tools to users. So as a first step, we decided to add
Sentry::TestHelper
for the following functionalities:setup_sentry_test
- Alters the SDK configuration to for testing. Based on how user sets up the test environment, this may be called once before all tests, or before every test.sentry_events
- An easy way to access all the event objects that have been captured.last_sentry_event
- Basicallysentry_events.last
.teardown_sentry_test
- Clears all captured events.Example