test(ourlogs): Add integration test for otlp logs outcomes #5169
Conversation
AbhiPrasad
force-pushed
the
abhi-otlp-logs-outcome-test
branch
from
f5896a5 to
2f2f9e6
Compare
tests/integration/test_otlp_logs.py
Outdated
| def test_otlp_logs_outcomes( | ||
| mini_sentry, | ||
| relay, | ||
| relay_with_processing, | ||
| outcomes_consumer, | ||
| ): |
There was a problem hiding this comment.
Why a new test instead of adding outcome assertions to the existing tests? That would be significantly faster.
There was a problem hiding this comment.
That feels like a code smell to me, shouldn't tests be designed to have minimal assertions? I also feel like this is easier to debug (did outcomes fail vs. did we change serialization).
Can we not parallelize these tests if they are getting slow? I assume startup time is not that bad.
There was a problem hiding this comment.
These are integration tests not unit tests, I'd want to avoid running the same test multiple times just asserting different parts of the outcome.
If you're asserting a specific kind of outcome, having a separate test makes sense, but if you're just asserting that there are correct outcomes for the input, I don't see why we can't do this in an already existing test which is supposed to test that e2e in Relay the processing works.
There was a problem hiding this comment.
Okay yeah that feels reasonable to me, I pushed up changes to amend existing tests for this.
![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.
This is a sanity check integration test to make sure that we are recording outcomes properly for otlp logs.
The intial set of integration tests were added in #5130