sentry-ruby
sentry-ruby copied to clipboard
getsentry
Ensure breadcrumbs are cleared when using Sentry::TestHelper
Description
Dear maintainers,
I am a happy early adopter of the new Sentry::TestHelper (#1773), and I would like to make an improvement.
I expected the Sentry::TestHelper#teardown_sentry_test method to clear the breadcrumbs, because it is good practice to avoid state leaking between tests. So I added a call to Sentry.get_current_scope.clear_breadcrumbs in the teardown method.
If we choose not to clear them within the teardown method, then we should add a note in the doc to inform people that breadcrumbs need to be manually cleared using Sentry.get_current_scope.clear_breadcrumbs after each test.
I would love to have your well-informed feedback on this matter! :pray:
Note: This bug is due to the fact that we alter the current transport and scope instead of initializing a new one. I thus considered an alternative implementation, initializing a new scope within the setup_sentry_test method. However, my understanding is that we need to preserve the current scope because it might have been customized by the tested code, so all we may do is alter it. I also noted that the state altered in setup_sentry_test is not actually rolled back in teardown_sentry_test. Do you believe this might lead to similar issues in the future?
@francois-ferrandis Thank you so much for using the test helpers and open this PR 😄
Do you think the helpers make it easier to test more SDK related usages?
This bug is due to the fact that we alter the current transport and scope instead of initializing a new one.
I think transport and scope needs to be discussed separately. In tests we use dummy transport, which is basically a container of sent events. So clearing those events is probably easier than initializing new Transport (not hard either though).
Scopes, on the other hand, it's easier to be recreated in every tests for having a clean state. We do that for clients already so it makes sense to do that on scopes too. I think I missed this during the initial implementation and I'll make a PR for it.
Do you think the helpers make it easier to test more SDK related usages?
Are you asking about how accurately the behavior of the test helper should mimic the production code ? I'm not sure to understand the question, could you elaborate or give examples? :thinking:
Note : My knowledge of the Unified API is quite limited, and I have no need to test anything else than events and breadcrumbs.
Scopes, on the other hand, it's easier to be recreated in every tests for having a clean state. We do that for clients already so it makes sense to do that on scopes too. I think I missed this during the initial implementation and I'll make a PR for it.
Thank you! Feel free to reference this PR so I am notified. :pray:
@francois-ferrandis I meant if having the helper helped you write more tests to test Sentry related logics, like if an event is sent, or if the scope's data is set correctly...etc.
I've opened https://github.com/getsentry/sentry-ruby/pull/1900 to address the issue, but with a slightly different way than I proposed above.