graphql-tools

graphql-tools copied to clipboard

Description

Add memory leak detection feature from jest.

Related #4346

Type of change

Please delete options that are not relevant.

• [X] Bug reproduction

How Has This Been Tested?

N/A

Checklist:

• [X] I have followed the CONTRIBUTING doc and the style guidelines of this project
• [X] I have performed a self-review of my own code
• [ ] I have commented my code, particularly in hard-to-understand areas
• [ ] I have made corresponding changes to the documentation
• [ ] My changes generate no new warnings
• [ ] I have added tests that prove my fix is effective or that my feature works
• [ ] New and existing unit tests and linter rules pass locally with my changes
• [ ] Any dependent changes have been merged and published in downstream modules

⚠️ No Changeset found

Latest commit: 21162ad54c3402f7cb00a8fb14654c21a8b1cb06

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/theguild/graphql-tools/GHeCCeyp6gMEfgTpvoG5WFKZ98on
✅ Preview: Failed

[Deployment for 21162ad failed]

I don't think failure tests are related to makeExecutableSchema, are they?

@ardatan I remember specifically removing this import and seeing the warning disappear. Then I cloned this repo, switched the detectLeaks flag and watched a bunch of positives. Checked the referred tests and saw makeExecutableSchema somewhere, and came to the (I agree hasty) conclusion that this import still was the cause for the leaks. Didn't take much more time to investigate any further when I reported. Now that I've taken a closer look at one of the positives (url-loader.spec.ts), it seems that UrlLoader is the culprit!

But makeExecutableSchema and the entire @graphql-tools/schema have nothing to do with UrlLoader.

@ardatan absolutely, I meant that my assumption was wrong, at least for this test file.

@ardatan When I find the time, I'll try to see if I can reproduce my issue in a minimal setup, and give you updates. As for this PR, I think at this point it is entirely up to you to close it, or fork it to apply your own fixes for those leaks that are seemingly internal. I am going to close the original issue as it was based on false assumptions.

Even if it is not related the issue, I see this beneficial :) Thank you @jsamr to show us this feature of Jest. I'll merge it once I fix tests

Thanks for the PR :) Now we have leak detection in the tests :)