zivl / sentry-testkit

A Sentry plugin to allow Sentry report interception and further inspection of the data being sent
https://zivl.github.io/sentry-testkit/
MIT License
112 stars 26 forks source link

[BUG]: Unable to capture events with Sentry upgrade #176

Closed nikkhn closed 1 year ago

nikkhn commented 1 year ago

Describe the bug When we upgraded @sentry/node from 7.57 to v7.58.1, sentry testkit was no longer able to trace sentry transactions in integration our tests. This was working prior to the @sentry/node upgrade

To Reproduce

// Integration Test
// This HTTP request sends a transaction to Sentry
    await request(dummyAuthApp)
      .post('/applicants')
      .send(body)
      .expect(200);

    expect(testkit.reports()).toHaveLength(0);
    expect(testkit.transactions()).toHaveLength(1); << testkit.transactions is now empty 

Expected behavior We expect the previous behavior to remain, where testkit.transactions() would capture transactions from @sentry/node

Screenshots

Sentry Versions (please complete the following information):

TomPridham commented 1 year ago

I'm not sure when the behavior changed because we were pretty behind on upgrading, but it seems there is now some async code in between capturing the transaction/event and it being added to testkit.reports() and testkit.transactions(). Adding a waitFor fixed the problem for us

await waitFor(() => {
  expect(testkit.transactions()).toHaveLength(1);
})
atrout commented 1 year ago

Thanks @TomPridham. We had already added an await time for these tests, but with the release of sentry-javascript 7.57 the tests started failing. I just tried adding wait-for-expect to the tests but it seems that the testkit transactions are never populated, even after sleeping for 4 seconds.

zivl commented 1 year ago

hey sorry to be late for the party 🥳
@nikkhn @TomPridham @atrout - Have any of you reported this issue to Sentry guys?

zivl commented 1 year ago

@nikkhn can you pls create a PR or other environment for reproduce this and I will report to Sentry

atrout commented 1 year ago

Hi @zivl, @nikkhn and I are still planning on submitting some code to reproduce this. We're hoping to have some time to do that next week.

zivl commented 1 year ago

@atrout @nikkhn I was unable to reproduce this issue. I set @sentry/node and @sentry/trace to 7.58.1 but all tests I've run seems to be working. So maybe I am missing something or I don't know... I'd be happy if you could create a PR with reproducible code where the issue can be well spotted and we can investigate further (+invite the Sentry team to join us debugging)

atrout commented 1 year ago

Hi @zivl, we made some changes to our code to clean up some tech debt and in the process managed to resolve this problem.

I tried to figure out which individual change resolved the issue but I wasn't able to narrow it down. We have an Express app that uses prisma to connect to a postgres database, and in the course of this cleanup we:

but reverting these changes individually doesn't result in failing tests.

Thank you for your help looking into this.

zivl commented 1 year ago

@atrout ok thanks for letting me know. I really was trying to reproduce it but for all scenarios my tests were passing 🤷‍♂️

I'll wait a few more days for @nikkhn who first opened the issue and if she won't be able to reproduce it or no response from her side I'll close this issue.

nikkhn commented 1 year ago

@zivl thank you for your responsiveness and attempts to reproduce! I will go ahead and close as @atrout mentioned how she was able to resolve our teams issue.

zivl commented 1 year ago

@nikkhn sure it was my pleasure :)