Detect memory leaks #4895

mjesun · 2017-11-14T22:32:03Z

Adding the usage of jest-leak-detector to the environment object. Whenever the experimental --detectLeaks flag is passed, a LeakDetector will be created for the environment object, and it will be checked later. If the result is positive, the test suite fails.

There are future plans to try improving the feedback we give to the user; I'm currently talking to someone on the V8 team see if we can do some really cool stuff parsing a heap snapshot taken immediately after. But what we currently have is just the ability of knowing if the global object got leaked.

Side note 1: please review only the last commit. This branch is put on top of the jest-leak-detector branch so that I can already use it, and the changes shown also include that other branch. Once we merge the first, I will rebase that one onto master.

Side note 2: if you are curious, some Jest tests actually leak. I haven't looked at them that much, but they mainly look related to file watchers.

cpojer · 2017-11-15T09:47:16Z

types/TestResult.js

@@ -138,6 +139,7 @@ export type TestResult = {|
  console: ?ConsoleBuffer,
  coverage?: RawCoverage,
  displayName: ?string,
+  leaks: LeakDetector | boolean,


Woah, this doesn't seem like a good type. What does it represent?

cpojer · 2017-11-15T09:48:11Z

packages/jest-cli/src/test_scheduler.js

+          'There is a number of things that can leak memory:',
+          '  - Async operations that have not finished (e.g. fs.readFile).',
+          '  - Timers not properly mocked (e.g. setInterval, setTimeout).',
+          '  - Missed global listeners (e.g. process.on).',


How about: "Global event listeners weren't cleaned up (e.g. process.on)"

With #4904 that one is no longer necessary 😃

cpojer · 2017-11-15T09:48:39Z

packages/jest-cli/src/test_scheduler.js

@@ -96,6 +96,23 @@ export default class TestScheduler {
        });
        return Promise.resolve();
      }
+      if (testResult.leaks) {
+        const message = [
+          'Your test suite is leaking memory! Please ensure all references are cleaned.',


I would put a bold statement on top and say "Experimental" etc. Nobody reads jest --help.

cpojer · 2017-11-15T09:48:57Z

packages/jest-cli/src/test_scheduler.js

+          '  - Timers not properly mocked (e.g. setInterval, setTimeout).',
+          '  - Missed global listeners (e.g. process.on).',
+          '  - Keeping references to the global scope.',
+        ].join('\n');


Can we not use an array and just use backticks + string concatenation like in the rest of Jest? :)

cpojer · 2017-11-15T09:49:27Z

packages/jest-cli/src/test_scheduler.js

+          message,
+          stack: new Error(message).stack,
+        });
+        return Promise.resolve();


return is enough, no? It's already an async function.

All this Promise.resolve() (not only mine, but all others) are there because of the eslint consistent-return rule 😞, but TBH I prefer keeping them and not adding an eslint-disable.

it's sufficient to just return onFailure(...) since it's already a promise (same thing can be fixed in testResult.testResults.length block above)

cpojer · 2017-11-15T09:50:01Z

packages/jest-runner/src/run_test.js

+// Keeping the core of "runTest" as a separate function (as "runTestHelper") is
+// key to be able to detect memory leaks. Since all variables are local to the
+// function, when "runTestHelper" finishes its execution, they can all be freed,
+// UNLESS someone else is leaking them (and that's why we can detect the leak!).


"UNLESS something else" not someone.

cpojer · 2017-11-15T09:50:28Z

packages/jest-runner/src/run_test.js

+// If we had all the code in a single function, we should manually nullify all
+// references to verify if there is a leak, which is not maintainable and error
+// prone. That's why "runTestHelper" CANNOT be inlined inside "runTest".
+async function runTestHelper(


Can we call it _runTest or runTestInternal? I don't like "util" or "helper" in general :D

cpojer · 2017-11-15T09:50:53Z

packages/jest-runner/src/run_test.js

-  try {
-    testSource = fs.readFileSync(path, 'utf8');
-  } catch (e) {
-    return Promise.reject(e);


Such clowny code.

The function wasn't async at the point it was written 🙂

cpojer · 2017-11-15T09:51:10Z

packages/jest-runner/src/run_test.js

@@ -66,6 +69,10 @@ export default (async function runTest(
  >);

  const environment = new TestEnvironment(config);
+  const environmentLeakDetector = config.detectLeaks


Can you call this variable just leakDetector? It's soooo long.

cpojer · 2017-11-15T09:51:16Z

packages/jest-runner/src/run_test.js

@@ -66,6 +69,10 @@ export default (async function runTest(
  >);

  const environment = new TestEnvironment(config);
+  const environmentLeakDetector = config.detectLeaks
+    ? new LeakDetector(environment)
+    : false;


cpojer · 2017-11-15T09:51:57Z

packages/jest-runner/src/run_test.js

-    return new Promise(resolve => setImmediate(() => resolve(result)));
+    await new Promise(resolve => setImmediate(resolve));
+
+    return result;


Is this just simplifying the code or is there another reason you made this change?

cpojer · 2017-11-15T09:52:53Z

packages/jest-runner/src/run_test.js

+
+  // Resolve leak detector, so that the object is JSON serializable.
+  if (result.leaks instanceof LeakDetector) {
+    result.leaks = result.leaks.isLeaking();


Ah, now I get it. This seems super hacky. How about instead of doing this we change runTestHelper to return {leakDetector, result} and turn result.leaks into a boolean?

mjesun · 2017-11-20T11:08:53Z

I'm almost sure the coverage report is wrong. According to it, the jest-leak-detector/src/index.js file dropped by 35%, and it reports as non-covered line 65, which is unconditionally called from line 29. How can we re-trigger the build?

SimenB · 2017-11-20T12:13:32Z

Does ./jest --coverage disagree locally?

You can restart builds by pressing rebuild on ciricle.

cpojer · 2017-11-20T13:20:27Z

packages/jest-cli/src/test_scheduler.js

+      // **EXPERIMENTAL!!** Throws when the context is leaked after executing
+      // a test.
+      if (testResult.leaks) {
+        const message = `Your test suite is leaking memory! Please ensure all references are cleaned.


Wait, the idea was to include the experimental text in the error message, not in a comment above the error message. The user should see this is experimental right now.

😂 OK, my bad; I thought you wanted me to add it as a comment so that when people looks why it broke, they will realize it's experimental. I will add it to the message.

Yeah, rm the comment :D

cpojer · 2017-11-20T13:57:35Z

packages/jest-cli/src/test_scheduler.js

+      if (testResult.leaks) {
+        const message = `Your test suite is leaking memory! Please ensure all references are cleaned.
+
+There is a number of things that can leak memory:


Can you intend this properly and use this style:

const foo = `…` + `…`;

etc.?

cpojer · 2017-11-20T13:58:03Z

packages/jest-runner/src/run_test.js

+  );
+
+  // Resolve leak detector, outside the "runTestInternal" closure.
+  result.leaks = leakDetector ? leakDetector.isLeaking() : false;


muuuch cleaner.

cpojer

This is awesome. Feel free to merge once the text is fixed.

I think this feature can also become really handy for bug reporting. When people report bugs related to memory leaks, we can point them to this flag which will help either us or them find memory leaks in Jest or user code (custom test environments etc.).

thymikee

Tested this today and works nice! It actually detects some leaks coming from graceful-fs in our codebase (interestingly and fortunately, they do not surface user tests, at least the most basic ones)

SimenB

This is missing an update to the changelog.

Also missing tests, but I'm not sure if it makes sense to add any?

mjesun · 2017-11-22T22:13:06Z

@thymikee Yeah,graceful-fs leaks because it modifies native modules. I'm working on a fix to re-inject native modules as well inside the vm context, so even if the code modifies them, they can be released at the end.

@SimenB Regarding tests, since this measures Jest leaks, the goal is to never have any test leaking; so I don't think it makes sense to make a test for that. In other words, if we create a test that leaks, we should fix what causes the leak instead of keeping it, so the test will become useless after some time.

codecov-io · 2017-11-25T17:05:49Z

Codecov Report

Merging #4895 into master will decrease coverage by 0.15%.
The diff coverage is 20%.

@@            Coverage Diff             @@
##           master    #4895      +/-   ##
==========================================
- Coverage   60.36%   60.21%   -0.16%     
==========================================
  Files         198      198              
  Lines        6608     6620      +12     
  Branches        4        4              
==========================================
- Hits         3989     3986       -3     
- Misses       2619     2634      +15

Impacted Files	Coverage Δ
packages/jest-config/src/index.js	`23.8% <ø> (ø)`	⬆️
packages/jest-config/src/normalize.js	`92.94% <ø> (ø)`	⬆️
packages/jest-cli/src/test_result_helpers.js	`12.9% <ø> (ø)`	⬆️
.../src/legacy_code_todo_rewrite/jest_adapter_init.js	`0% <ø> (ø)`	⬆️
packages/jest-config/src/defaults.js	`100% <ø> (ø)`	⬆️
packages/jest-runner/src/run_test.js	`2.22% <0%> (-0.16%)`	⬇️
packages/jest-cli/src/test_scheduler.js	`25.54% <60%> (-2.07%)`	⬇️
packages/jest-leak-detector/src/index.js	`65% <0%> (-35%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4883838...11f995f. Read the comment docs.

mjesun · 2017-11-26T00:41:54Z

There's definitely something wrong with the coverage shown by codecov. After executing it locally with the lcov reporter, by doing yarn jest --coverage packages/jest-leak-detector, the results show that everything is fully covered:

SimenB · 2017-11-26T15:33:44Z

Running full coverage locally agrees with you - it's fully covered. Not sure what's up...

I tried to run without -i, but it just hangs infinitely on the last test.

Codecov also says that some imports are hit while some are not, so it's confused.

thymikee · 2017-11-26T15:34:35Z

@SimenB have you tried on Node 8?

SimenB · 2017-11-26T15:37:54Z

I'm using node 8.9.1

mjesun · 2017-11-26T16:19:30Z

The import thing could be due to inlining requires? That would not explain why results don't match between codecov and local executions, though.

cpojer · 2017-11-27T09:02:04Z

So excited to get this in to track down memory leaks in Jest.

github-actions · 2021-05-13T01:32:34Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

mjesun requested review from cpojer and SimenB November 14, 2017 22:32

facebook-github-bot added the CLA Signed ✔️ label Nov 14, 2017

cpojer reviewed Nov 15, 2017

View reviewed changes

cpojer reviewed Nov 20, 2017

View reviewed changes

cpojer approved these changes Nov 20, 2017

View reviewed changes

thymikee approved these changes Nov 22, 2017

View reviewed changes

thymikee mentioned this pull request Nov 22, 2017

Build on node 9 on Circle #4851

Merged

SimenB approved these changes Nov 22, 2017

View reviewed changes

Miguel Jimenez Esun and others added 2 commits November 25, 2017 16:52

Make use of jest-leak-detector to detect tests leaking memory

7817781

Addressed feedback

11f995f

jestjs deleted a comment from codecov-io Nov 25, 2017

cpojer merged commit 3d2eb37 into jestjs:master Nov 27, 2017

mjesun deleted the detect-memory-leaks branch December 6, 2017 11:46

renovate-tests mentioned this pull request Feb 6, 2018

Update dependency jest to v22 - autoclosed renovate-tests/changelog-md#3

Closed

renovate bot mentioned this pull request Feb 14, 2018

fix(deps): update dependency jest to ^22.0.0 manekinekko/devoxx-assistant#3

Open

github-actions bot locked as resolved and limited conversation to collaborators May 13, 2021

Detect memory leaks #4895

Detect memory leaks #4895

Conversation

mjesun commented Nov 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjesun commented Nov 20, 2017

SimenB commented Nov 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpojer left a comment

Choose a reason for hiding this comment

thymikee left a comment

Choose a reason for hiding this comment

SimenB left a comment

Choose a reason for hiding this comment

mjesun commented Nov 22, 2017

codecov-io commented Nov 25, 2017

Codecov Report

mjesun commented Nov 26, 2017

SimenB commented Nov 26, 2017 • edited Loading

thymikee commented Nov 26, 2017

SimenB commented Nov 26, 2017

mjesun commented Nov 26, 2017

cpojer commented Nov 27, 2017

github-actions bot commented May 13, 2021

mjesun commented Nov 14, 2017 •

edited

Loading

SimenB commented Nov 20, 2017 •

edited

Loading

SimenB commented Nov 26, 2017 •

edited

Loading