tools: enable no-unused-vars linter rule and fix issues

[silverwind]

This enables the no-unused-vars rule. Issues that came up were mostly the variables on top of tests and there were a few cases where there was leftover dead code.

I intentionally didn't enable the feature for unused function arguments yet. I think the best to go about these is to comment out the arguments in lib while removing them in test, does that sound good?

Note that this is somewhat WIP, as there were a few cases where the intention seemed to be to use some of these variables, but I'm submitting this now as-is for some feedback.

[Fishrock123]

Requiring common actually just does things.

I suppose we don't need to assign it to a variable though.

[silverwind]

Are these 'things' relevant to tests that don't use the variable?

[Fishrock123]

Yes. See https://github.com/nodejs/node/blob/master/test/common.js#L304-L313 among other things. Actually, all tests should require common. (Perhaps it should just be pre-loaded, not sure.)

[silverwind]

If it's just this leaked global check, I see two options:

• We remove that leaked global check. These can be warned through the linter unless we do something like global.x, but that ought to be intentional.
• We preload common.js

[Trott]

There's this too but I think that's it:

node/test/common.js

Lines 142 to 151 in 68dc69a

	if (process.env.NODE_COMMON_PIPE) {
	exports.PIPE = process.env.NODE_COMMON_PIPE;
	// Remove manually, the test runner won't do it
	// for us like it does for files in test/tmp.
	try {
	fs.unlinkSync(exports.PIPE);
	} catch (e) {
	// Ignore.
	}
	}

[rvagg]

what's the big deal with requiring common? are you trying to shave off ms?

[silverwind]

Requiring common without actually using it violates the rule I'm trying to add 😉

[silverwind]

Just saw that the rule allows exceptions per varsIgnorePattern, so that could be another option.

[Trott]

Could also change var common = require('../common.js') to just require('../common.js');...

[silverwind]

That looks a bit strange imho and would confuse someone who doesn't know about this 'dependency'.

[silverwind]

Looks like option I need was added just in 1.3 (eslint/eslint@91fc1c5), making this PR depend on #2286.

[silverwind]

This is obsolete now with #4536.