Add Sentry::DebugTransport for testing/debugging #2664
Conversation
solnic
force-pushed
the
add-debug-transport
branch
3 times, most recently
from
88ede05 to
6a63405
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
solnic
force-pushed
the
add-debug-transport
branch
from
a51b834 to
2c4ff7e
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
solnic
force-pushed
the
add-debug-transport
branch
from
2c4ff7e to
0735440
Compare
solnic
force-pushed
the
add-debug-transport
branch
from
0735440 to
5d4cdb2
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
solnic
force-pushed
the
add-debug-transport
branch
from
087c021 to
7336f55
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
solnic
force-pushed
the
add-debug-transport
branch
2 times, most recently
from
28bb893 to
7992e27
Compare
This is important because that's how actual Transports behave. The specs had to be changed because their expectations were based on behavior specific to DummyTransport. Reasons for changing spec expectations: Log spec creates a transaction and we run profiler for every transaction, but due to sampling decision profile event is being dropped and we end up with a client report. SessionFlush calls capture_envelope which bypasses send_event, but the test actually does send two error events, so now that DummyTransport#send_event calls super, we end up (correctly) with 3 envelopes: 2 from errors, and 1 from session.
solnic
force-pushed
the
add-debug-transport
branch
from
7992e27 to
0cec976
Compare
solnic
force-pushed
the
add-debug-transport
branch
from
0cec976 to
2b0d49e
Compare
|
@sl0thentr0py mind taking another look? I made a couple of extra improvements and refactored DebugTransport into a delegator. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
solnic
force-pushed
the
add-debug-transport
branch
from
2b0d49e to
adc27b3
Compare
| @@ -390,7 +390,8 @@ | |||
|
|
|||
| Sentry.get_current_client.flush | |||
|
|
|||
| expect(sentry_envelopes.size).to be(2) | |||
| # 3 envelopes: log, transaction, and client_report about dropped profile | |||
There was a problem hiding this comment.
we actually shouldn't be sending the dropped profile client report here since we don't have profiles_sample_rate enabled, we should fix this separately
There was a problem hiding this comment.
@sl0thentr0py yeah it seemed suspicious, I reported an issue about this #2669
| if Sentry.initialized? | ||
| transport = Sentry.get_current_client&.transport | ||
|
|
||
| if transport.is_a?(Sentry::DebugTransport) |
There was a problem hiding this comment.
we aren't actually using the DebugTransport anywhere in the tests so why is this needed?
There was a problem hiding this comment.
@sl0thentr0py another PR will actually use this in a couple of e2e test
| @@ -667,11 +667,15 @@ def will_be_sampled_by_sdk | |||
|
|
|||
| Sentry.session_flusher.flush | |||
|
|
|||
| expect(sentry_envelopes.count).to eq(1) | |||
| envelope = sentry_envelopes.first | |||
| expect(sentry_envelopes.count).to eq(3) | |||
There was a problem hiding this comment.
why did these values change?
There was a problem hiding this comment.
@sl0thentr0py because the test actually sends 2 errors and flushes session which sends another event, so total is 3 envelopes, previously dummy transport wouldn't catch 2 other envelopes
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2664 +/- ##
==========================================
+ Coverage 97.38% 97.41% +0.02%
==========================================
Files 134 135 +1
Lines 5169 5229 +60
==========================================
+ Hits 5034 5094 +60
Misses 135 135
🚀 New features to boost your workflow:
|
Long story short: I think this is something very useful to have. I missed something like this in the past and when I worked on e2e test setup, I figured it will be a good moment to finally add it because it helped with automated e2e testing too.