test: remove unnecessary assignments #4408
Conversation
common.js needs to be loaded in all tests so that there is checking for variable leaks and possibly other things. However, it does not need to be assigned to a variable if nothing in common.js is referred to elsewhere in the test. The main tradeoff for this bit of code churn is that it gets the code base most of the way to being able to enable the no-unused-vars rule in eslint. (The non-tooling benefit is that it lessens cognitive load when reading tests as it is an immediate indication that none of the functions or properties in common.js will be used by the test.)
|
LGTM if CI is green |
|
EDIT: Er, partially landed. |
common.js needs to be loaded in all tests so that there is checking for variable leaks and possibly other things. However, it does not need to be assigned to a variable if nothing in common.js is referred to elsewhere in the test. PR-URL: #4408 Reviewed-By: James M Snell <[email protected]>
|
Landed properly in 082cc8d. Sorry about the mess-up. I made the snap judgment to force push on the grounds that it was soon enough after the error and a slow holiday weekend throughout much of the world that a force push would not be catastrophic. (I thought I was given the guideline at onboarding that a force push to fix a significant error was OK within 3 minutes, but I may have imagined that as I can't find it anywhere now.) I'm literally the only person who has landed any commits for nearly three days, so that also entered into my "it's a slow time right now" calculation. Hopefully I didn't make a wrong decision there, and if I did, then hopefully it's a bad decision with only minor ramifications. Apologies in advance for any heartburn this might cause. |
|
(Now that I've had a moment to reflect, I wouldn't choose to handle it that way again. Eh, so I would have had two commits for this instead of one. So what? 20/20 hindsight and all that...) Running a CI just to make sure everything is cromulent, and everything looks good so far... |
|
Unrelated PPC build failure. Looks good otherwise. https://ci.nodejs.org/job/node-test-commit/1540/ |
|
@Trott ... doing a force push in a circumstance like that is fine, and you caught it in enough time that all is good. We've all been there so no worries. And don't worry, I'll be back to pushing commits on Monday ;-) |
common.js needs to be loaded in all tests so that there is checking for variable leaks and possibly other things. However, it does not need to be assigned to a variable if nothing in common.js is referred to elsewhere in the test. PR-URL: nodejs#4408 Reviewed-By: James M Snell <[email protected]>
|
@Trott it looks like a bunch of edits here were made to files that don't exist in v4.x yet. Would you be able to adapt this PR and send one in specifically for lts against v4.x-staging? |
|
@thealphanerd Done. #4563 |
|
😄 |
|
Backport landed |
common.js needs to be loaded in all tests so that there is checking for variable leaks and possibly other things. However, it does not need to be assigned to a variable if nothing in common.js is referred to elsewhere in the test. PR-URL: nodejs#4408 Reviewed-By: James M Snell <[email protected]>
common.js needs to be loaded in all tests so that there is checking
for variable leaks and possibly other things. However, it does not
need to be assigned to a variable if nothing in common.js is referred
to elsewhere in the test.
The main tradeoff for this bit of code churn is that it gets the code
base most of the way to being able to enable the no-unused-vars rule in
eslint.
(The non-tooling benefit is that it lessens cognitive load when reading
tests as it is an immediate indication that none of the functions or
properties in common.js will be used by the test.)