Add `Sentry::TestHelper` by st0012 · Pull Request #1773 · getsentry/sentry-ruby

st0012 · 2022-03-24T12:18:41Z

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 - Basically sentry_events.last.
teardown_sentry_test - Clears all captured events.

Example

require 'rails_helper'

RSpec.describe "Welcomes", type: :request do
  before do
    setup_sentry_test
  end

  after do
    teardown_sentry_test
  end

  describe "GET /" do
    it "captures and sends exception to Sentry" do
      get "/"
      expect(response).to have_http_status(500)
      expect(sentry_events.count).to eq(2)

      error_event = sentry_events.first
      expect(error_event.transaction).to eq("WelcomeController#index")
      error = extract_sentry_exceptions(error_event).first
      expect(error.type).to eq("ZeroDivisionError")
      expect(error_event.tags).to match(counter: 1, request_id: anything)

      transaction_event = sentry_events.last
      expect(transaction_event.spans.count).to eq(3)
    end
  end
end

st0012 · 2022-03-28T17:27:39Z

@sl0thentr0py If we're going to do this, I think the Sentry::TransactionEvent refactor needs to be done as well. Otherwise the extra interfaces on it (e.g. it still has #exception) will be confusing to users.
Another reason is that "conceptually", both Sentry::Event and Sentry::TransactionEvent will become public to users as they'll be poking with it in tests. And that'll make the "refactor" a breaking change.

Wdyt?

st0012 · 2022-03-28T20:54:02Z

I spent an hour trying the separation and I think the ideal modeling would be:

Event = breadcrumbs + request + contexts + tags....etc.
ErrorEvent = Event + threads + exception + level
TransactionEvent = Event + spans

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 Sentry::Event & Sentry::TransactionEvent relationship.

sl0thentr0py · 2022-03-29T11:16:04Z

@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 Event type that then branches based on the type field for further processing. So this inheritance/derivation of both Error and Transaction from Event exists throughout Sentry.

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.

st0012 · 2022-03-29T13:55:50Z

Oh cool. So on the server side it's actually modeled better, but the SDK side's spec and implementation are the "dirty" ones?
If that's the case, I think we (or you) can initiate the modeling change from the Ruby SDK?

sl0thentr0py · 2022-03-29T14:51:13Z

Even between SDKs there's a fair amount of inconsistency.

python doesn't have any Event class, just a hash and event_from_exception and transaction.finish that makes the corresponding event hash.
js actually has the cleanest modeling here which fits with your comment and also is more or less one-one with the relay protocol.

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. :)

st0012 · 2022-03-29T14:54:53Z

@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.

sl0thentr0py · 2022-03-29T14:55:54Z

ok so we do a breaking release then?

st0012 · 2022-03-29T15:11:48Z

Personally I don't think so as users shouldn't have any actual usage to touch Sentry::Event or Sentry::TransactionEvent directly? And the top-level capture_* interfaces should remain the same, so should the final payload.

From my understanding, the behavioral changes would be

Interface like exception and threads being removed from Sentry::TransactionEvent.
- But they're not used and documented anyway. If users somehow used them themselves, it's not officially supported. So I consider them private interfaces and removing them aren't breaking.
Sentry::ErrorEvent will be added to represent error events.
- If the user uses is_a?(Sentry::Event) in any part of the code, it should still work the same (presenting both Sentry::ErrorEvent and Sentry::TransactionEvent) because of inheritance.

So I don't think it's more like a private API change and doesn't need a breaking release. Wdyt?

sl0thentr0py · 2022-03-29T15:23:16Z

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-commenter · 2022-03-30T12:14:10Z

Codecov Report

Merging #1773 (b419775) into master (e55cbf1) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            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

Impacted Files	Coverage Δ
sentry-ruby/spec/sentry/transport_spec.rb	`100.00% <ø> (ø)`
sentry-ruby/lib/sentry/test_helper.rb	`100.00% <100.00%> (ø)`
...-ruby/spec/sentry/breadcrumb/sentry_logger_spec.rb	`100.00% <100.00%> (ø)`
...ntry-ruby/spec/sentry/client/event_sending_spec.rb	`99.59% <100.00%> (-0.01%)`	⬇️
sentry-ruby/spec/sentry/client_spec.rb	`96.88% <100.00%> (ø)`
sentry-ruby/spec/sentry/configuration_spec.rb	`100.00% <100.00%> (ø)`
sentry-ruby/spec/sentry/event_spec.rb	`100.00% <100.00%> (ø)`
sentry-ruby/spec/sentry/hub_spec.rb	`100.00% <100.00%> (ø)`
...y-ruby/spec/sentry/rack/capture_exceptions_spec.rb	`100.00% <100.00%> (ø)`
sentry-ruby/spec/sentry/scope_spec.rb	`100.00% <100.00%> (ø)`
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e55cbf1...b419775. Read the comment docs.

github-actions · 2022-05-01T00:16:57Z

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 Status: Backlog or Status: In Progress, I will leave it alone ... forever!

"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

sl0thentr0py · 2022-07-14T11:52:23Z

@st0012 do you know what we wanna do with this? Should it go out with the next release or do we wait till later?

st0012 · 2022-07-16T10:04:43Z

Since some users start asking for the close API for their testing, I think test helpers like these should also benefit them? If so, we can release it now and reiterate them later.

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.
But it also comes back to if Sentry wants to support them?

sl0thentr0py

@st0012 I merged master and updated some specs, do you want to add a changelog before merging?

st0012 · 2022-07-18T13:07:31Z

@sl0thentr0py can we wait for the close api first? I want to refresh the implementation here with it.

sl0thentr0py

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.

st0012 added enhancement sentry-ruby labels Mar 24, 2022

st0012 added this to the 5.3.0 milestone Mar 24, 2022

st0012 self-assigned this Mar 24, 2022

sl0thentr0py mentioned this pull request Mar 29, 2022

Clean up Event and TransactionEvent inheritance logic #1776

Closed

st0012 mentioned this pull request Mar 29, 2022

Correct inaccurate event model relationships #1777

Merged

st0012 force-pushed the implement-#1680 branch from 0c63470 to dcd72c4 Compare March 30, 2022 12:12

st0012 force-pushed the implement-#1680 branch from dcd72c4 to 891e377 Compare March 30, 2022 12:24

st0012 marked this pull request as ready for review March 30, 2022 12:27

st0012 force-pushed the implement-#1680 branch 2 times, most recently from 56b26da to 07e5082 Compare March 30, 2022 12:28

st0012 mentioned this pull request Mar 30, 2022

Should the SDK provide test helpers? #1680

Closed

st0012 modified the milestones: 5.3.0, 5.4.0 Apr 9, 2022

github-actions bot added the Status: Stale label May 1, 2022

st0012 added Status: Backlog and removed Status: Stale labels May 1, 2022

sl0thentr0py approved these changes Jul 18, 2022

View reviewed changes

vladanpaunovic linked an issue Jul 21, 2022 that may be closed by this pull request

Should the SDK provide test helpers? #1680

Closed

st0012 force-pushed the implement-#1680 branch from 64ed86e to 3c55b2f Compare July 21, 2022 10:31

st0012 requested a review from sl0thentr0py July 21, 2022 11:04

st0012 added testing and removed wip labels Jul 21, 2022

st0012 added 4 commits July 21, 2022 12:07

Add Sentry::TestHelper

80101f3

Add Sentry::TestHelper usage examples

a62fefd

Add tests for TestHelper

6b54d5a

Update changelog

b419775

st0012 force-pushed the implement-#1680 branch from d7ed830 to b419775 Compare July 21, 2022 11:08

sl0thentr0py approved these changes Jul 21, 2022

View reviewed changes

st0012 merged commit c9b4eac into master Jul 23, 2022

st0012 deleted the implement-#1680 branch July 23, 2022 09:12

francois-ferrandis mentioned this pull request Sep 5, 2022

Ensure breadcrumbs are cleared when using Sentry::TestHelper #1881

Closed

Uh oh!

Conversation

st0012 commented Mar 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Example

Uh oh!

st0012 commented Mar 28, 2022

Uh oh!

st0012 commented Mar 28, 2022

Uh oh!

sl0thentr0py commented Mar 29, 2022

Uh oh!

st0012 commented Mar 29, 2022

Uh oh!

sl0thentr0py commented Mar 29, 2022

Uh oh!

st0012 commented Mar 29, 2022

Uh oh!

sl0thentr0py commented Mar 29, 2022

Uh oh!

st0012 commented Mar 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sl0thentr0py commented Mar 29, 2022

Uh oh!

codecov-commenter commented Mar 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions bot commented May 1, 2022

Uh oh!

sl0thentr0py commented Jul 14, 2022

Uh oh!

st0012 commented Jul 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sl0thentr0py left a comment

Choose a reason for hiding this comment

Uh oh!

st0012 commented Jul 18, 2022

Uh oh!

sl0thentr0py left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

st0012 commented Mar 24, 2022 •

edited

Loading

st0012 commented Mar 29, 2022 •

edited

Loading

codecov-commenter commented Mar 30, 2022 •

edited

Loading

st0012 commented Jul 16, 2022 •

edited

Loading