Document best practices

[andywer]

Here is a draft for what the best practices should contain. Follow-up of #18.

Topics

No console.log() on non-primitive values in tests

As @mjhea0 pointed out, the console.* methods will keep a reference to any object/array/function you pass them, thus artificially introducing a memory leak. See http://stackoverflow.com/a/28859493/1799408.

Leakage tests are quite slow

... so think twice before running it together with your other tests everytime. Point out why they are quite slow (as in the FAQ/Timeouts). Ideally present alternatives.

Make test fail first, then fix it

Similar to other kinds of test it is also best practice to first check that your leakage test actually works, by purposely introducing a leak and watching the test fail. Remove the initial leak again and make sure the test passes.

(to be filled with additional content)

[andywer]

@btmills Do you have further recommendations?

[btmills]

I saw you added a note to the readme about increasing Mocha's timeout. I'm using the API method rather than the CLI parameter so that it only changes the timeout for the leak test.

I like to intentionally introduce a leak when I'm writing a leak test so I can see the test fail as expected first to avoid the possibility that there's a bug in my test hiding a leak in my code.

[andywer]

Yeah, actually I was going to describe the API way, too, first, but then recognized that it only works when not using arrow functions (so this refers to the Mocha test suite) and I could almost already see people open issues because they use arrow functions and it does not work 🙈

Making the test fail first, then fixing it is always a good idea. Will add it to the list :)

[btmills]

Good point. I saw answer given for one that works with arrow functions:

it('should not leak, () => {
    // Leak test
}).timeout(5000);

Unfortunately that doesn't seem to be part of the public API, so probably best not to encourage it.

Making the test fail first, then fixing it is always a good idea.

I don't always TDD, but when I do, it's because I don't trust myself to write bug-free tests 😺

[bitjson]

Thanks for the awesome project!

On documentation, I'd love to see some examples of code that leaks. Maybe in in the "Memory Management in JS?" section?

I see the value here from debugging memory leaks in very large Node.js codebases, but after thinking about it a bit, I've only seen issues which could have been found by a linter preventing implicit variable declaration.

Other than that, my simple examples look really contrived. E.g.

() => {
  const data = getBigObject();
  somethingOutsideTheIterateScope.data.push(data);
}

Is it always an issue of code attaching data to globally accessible objects? I'd love to see some simple examples of issues that would be likely to slip past eslint or tslint.

[andywer]

@bitjson Good feedback, thanks!

Well, yes and no. Basically it's always about mutating something on an object that exceeds the mutation-causing code's scope, but sometimes it is really subtle. andywer/threads.js#32, for instance, is a PR fixing a memory leak in one of my other OSS projects.

That leak is quite hard to spot: The method set up a success listener with .once() and an error listener with .once(). You might think that using .once() will do the clean-up for you, since it will unsubscribe after the first invocation automatically. Except that won't suffice, since there will only be either one success or one error invocation, leaving the other event handler in place. You will end up with two new listeners on each iteration, but only one of them will be removed.

I'm really short on time these days, but I hope we can have a sort-of community effort to improve the docs over time.