-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
ref(tests): Refactor Node integration tests further. #5596
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@onurtemizkan - what do you think about trying to turn this into a class while we are here doing fixes? #5579 (review). If you think it adds too much complexity, no worries, we table it for later. Also, happy to pair program on this any time! Just let me know :) |
7e25c6f
to
a065c41
Compare
@AbhiPrasad, yes I was planning to include that, but would prefer if we do that on a clean scope after #5512 gets merged, if that's OK? I'm not entirely sure if there is still a potentially fragile test around. |
4e6adf3
to
c47033f
Compare
Sounds like a plan @onurtemizkan, lets do that! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice - I think this is cleaner and easier to understand as well!
size-limit report 📦
|
Following up: #5579
Apparently, the updates on #5579 were not enough to resolve leaking issues on #5512.
Debugging further, it seems that whenever a manual
http
request is initiated from a test (or test server) without waiting for / asserting on events or transactions, there are also transactions as side effects which may or may not finish before the test ends. This specifically happens with tests that assert request headers fortrace
andbaggage
, because they don't need to wait for the request / scenario to finish.So it looks like the leakage to the next test doesn't come from the
nock
interceptor or an unclosed server, it comes from an unflushed Sentry transaction. The other tests usinggetEnvelopeRequest
orgetMultipleEnvelopeRequest
finish wait for the events to be flushed.While debugging this, I also made some more refactors in utilities, which I think will make them more concise.
Summary:
getAPIResponse
orrunScenario
complete.http
usages in helpers toaxios
calls for simplicity and debuggability.nock
initialization out ofrunServer
so we don't need to carrynock.Scope
around anymore. Thenock
instance is either manually initialized, or inside thegetMultipleEnvelopeRequest
function. The tests that are using manualhttp
calls don't normally need nock anyways.nock
level, so if there is a need to filter them, we don't lose the expectedcount
. Also, that makes the envelope indices inside tests simpler.nock.cleanAll()
andnock.removeInterceptor(mock)
to endnock
instead of.persist(false)
.I have tested this locally on #5512 and it seems to be working well.
🤞