search

getsentry / sentry-cocoa

The official Sentry SDK for iOS, tvOS, macOS, watchOS.
https://sentry.io/for/cocoa/
MIT License
812 stars 324 forks source link

Fix flaky `SentrySessionGeneratorTests` test #2102

Closed armcknight closed 2 years ago

armcknight commented 2 years ago

Description

Disabled here: https://github.com/getsentry/sentry-cocoa/blob/86e8170960ad82b7f1ace0523d64dde5b751b5c5/Tests/SentryTests/Integrations/Session/SentrySessionGeneratorTests.swift#L62-L67

kevinrenskers commented 2 years ago

It's not a flaky tests, it disabled on purpose because it doesn't actually test anything. So why don't we just delete this? 🤔 @philipphofmann what's the reason for the existence of this test class when its only test function is disabled on purpose?

armcknight commented 2 years ago

Yes, you're right @kevinrenskers, I must've gotten mixed up creating a couple tickets like this.

I don't understand why it was originally written this way in https://github.com/getsentry/sentry-cocoa/pull/614. It wasn't running at some point and later disabled, it was never functional.

philipphofmann commented 2 years ago

Isn't the comment clear for you, @armcknight?

Disabled on purpose. This test just sends sessions to Sentry, but doesn't verify that they arrive there properly.

It's handy for testing if sessions arrive correctly in Sentry.

kevinrenskers commented 2 years ago

So you need to manually enable the test again, run it, and then manually check inside of Sentry that they arrived correctly? Does anyone ever do that? Shouldn't we just remove this test? 🤔

philipphofmann commented 2 years ago

So you need to manually enable the test again, run it, and then manually check inside of Sentry that they arrived correctly? Does anyone ever do that? Shouldn't we just remove this test?

Yes, exactly. I do it when I touch the session logic.

philipphofmann commented 2 years ago

I would like to keep that test, please. It was useful in the past for me.