Skip to content

ref(test): Refactor Node integration tests to prevent nock leaks. #5579

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

Merged
merged 5 commits into from
Aug 16, 2022

Conversation

onurtemizkan
Copy link
Collaborator

@onurtemizkan onurtemizkan commented Aug 12, 2022

Currently, Node integration tests initialize nock interceptors on server initialization and end them after a certain timeout. This recently created problems with asynchronous event processors, and caused random cases of envelope leaks between tests.

To resolve it, this approach returns http.Server and nock.Scope instances back to the test, to eventually be provided to a helper that either collects envelopes or makes requests. The responsibility of ending those two instances is moved to the collection helpers like getEnvelopeRequest or getAPIResponse.

Tests that do not use those helpers should close the server and nock interceptors manually.
This way, we ensure that we do not lose the context of the server and the nock interceptors between tests.

Needs to be tested on #5512

@onurtemizkan onurtemizkan self-assigned this Aug 12, 2022
@onurtemizkan onurtemizkan marked this pull request as ready for review August 15, 2022 12:52
Copy link
Member

@AbhiPrasad AbhiPrasad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we update the README with these details?

IMO not the end of the world for us to have this explicit passing of server and scope. If we think it's too messy, we could also choose to create a class that manages this, and then have the url/server/scope be internal vars to that class. What do you think about that?

If we choose to do the class method, let's first merge this in to unblock Tim, and then come back to it.

@@ -19,6 +19,7 @@
"@remix-run/dev": "^1.6.5",
"@types/react": "^17.0.47",
"@types/react-dom": "^17.0.17",
"nock": "^13.1.0",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did we have to add this dep?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TS falls back to the global installation of nock, which seems to have a slight difference between what we use in node-integration-tests. Had to add this (the same version with node-integration-tests) to resolve it.

Comment on lines 87 to 89
count: number,
method: 'get' | 'post' = 'get',
endServer: boolean = true,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that there are three options, can we pass this in as an options obj (2nd arg)?

Copy link
Collaborator

@timfish timfish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@onurtemizkan
Copy link
Collaborator Author

If we think it's too messy, we could also choose to create a class that manages this, and then have the url/server/scope be internal vars to that class.

Yes, I think it would make it much cleaner and easier/safer to use. 👍

Copy link
Member

@AbhiPrasad AbhiPrasad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thanks for taking a look at this @onurtemizkan!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants