Remove uncommon functions from common module #17781

Trott · 2017-12-20T06:29:27Z

In an effort to make the common module slightly less of a monolith and junk drawer, and to reduce the needed parsing/loading of some function from around 2000 times per test run to just 1, this PR removes functions that are only used once.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

test

Trott · 2017-12-20T06:34:19Z

CI: https://ci.nodejs.org/job/node-test-pull-request/12222/

TimothyGu · 2017-12-20T06:39:03Z

test/sequential/test-async-wrap-getasyncid.js

+    let tty_fd = 0;
+    if (!tty.isatty(tty_fd)) tty_fd++;
+    else if (!tty.isatty(tty_fd)) tty_fd++;
+    else if (!tty.isatty(tty_fd)) tty_fd++;


Can someone explain to me how this works?

I didn't write the code but I assume:

The default file descriptors for I/O are stdin, stdout and stderr (0, 1 and 2 respectively). It's not always guaranteed to be the case, so we try the first 3 and then try /dev/tty which usually works (it maps for the controlling terminal of a process for each process) but it's slower to call than isatty it if it's 0.

It was a cut-and-paste, I didn't write it, but yeah, it's not just hard to follow, but also buggy. If the first if fails, on line 253, it increments tty_fd but doesn't then check the condition on line 254 because that's an else so it skips that line. Ugh.

If 0 and 1 are not TTYs but 2 is, we won't get 2 (unless that's what's returned by the try/catch block subsequently).

I think something more like this is what we want:

let tty_fd = [0, 1, 2].find(tty.isatty); if (tty_fd === undefined) { try { tty_fd = fs.openSync('/dev/tty'); } catch (e) { // There aren't any tty fd's available to use. return -1; } } return tty_fd;

(And while I'm in there fixing this function so it hopefully works correctly, I should probably refactor tty_fd to have a name that doesn't include _.)

Added a commit here to fix this. PTAL.

common.projectDir is used in one test, so it's not so common. Remove from common module to the one test file that needs it.

common.getTTYfd() is used in one test only. Move it's definition to that test and out of the common module.

common.firstInvalidFD() is used in only one test. Move it out of the common module and into the one test that uses it.

Ref: nodejs#17781 (comment)

Trott · 2017-12-20T14:22:34Z

Some code refactored and bug-fixed.

CI: https://ci.nodejs.org/job/node-test-pull-request/12229/

benjamingr · 2017-12-20T16:23:23Z

test/sequential/test-async-wrap-getasyncid.js

-  if (tty_fd >= 0) {
+  function getTTYfd() {
+    const tty = require('tty');
+    let ttyFd = [0, 1, 2].find(tty.isatty);


Now that I read it - I'm surprised isatty is synchronous :D Change LGTM

lpinca · 2017-12-20T17:33:43Z

test/common/index.js

-  const tty = require('tty');
-  let tty_fd = 0;
-  if (!tty.isatty(tty_fd)) tty_fd++;
-  else if (!tty.isatty(tty_fd)) tty_fd++;


Was this branch ever reached?

Nvm, 8be1259 is the answer.

Trott · 2017-12-20T21:40:59Z

(CI failures are unrelated.)

Trott · 2017-12-21T18:12:39Z

Landed in fc8e821 ddf3655 40bab30 and 1af82f3

common.projectDir is used in one test, so it's not so common. Remove from common module to the one test file that needs it. PR-URL: nodejs#17781 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Jon Moss <me@jonathanmoss.me> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

common.getTTYfd() is used in one test only. Move it's definition to that test and out of the common module. PR-URL: nodejs#17781 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Jon Moss <me@jonathanmoss.me> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

common.firstInvalidFD() is used in only one test. Move it out of the common module and into the one test that uses it. PR-URL: nodejs#17781 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Jon Moss <me@jonathanmoss.me> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

PR-URL: nodejs#17781 Ref: nodejs#17781 (comment) Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Jon Moss <me@jonathanmoss.me> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

MylesBorins · 2018-01-09T23:35:34Z

This does not cleanly land on v9.x, would someone be willing to manually backport?

Trott · 2018-01-11T04:23:59Z

@MylesBorins This will land cleanly once #17401 is backported.

Trott · 2018-01-11T04:54:16Z

Once #18096 (backport of #17401) lands, these commits will land cleanly and won't need to be backported.

common.projectDir is used in one test, so it's not so common. Remove from common module to the one test file that needs it. PR-URL: #17781 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Jon Moss <me@jonathanmoss.me> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

common.getTTYfd() is used in one test only. Move it's definition to that test and out of the common module. PR-URL: #17781 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Jon Moss <me@jonathanmoss.me> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

common.firstInvalidFD() is used in only one test. Move it out of the common module and into the one test that uses it. PR-URL: #17781 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Jon Moss <me@jonathanmoss.me> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

PR-URL: #17781 Ref: #17781 (comment) Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Jon Moss <me@jonathanmoss.me> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

Trott added the test Issues and PRs related to the tests. label Dec 20, 2017

nodejs-github-bot added doc Issues and PRs related to the documentations. test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory. labels Dec 20, 2017

TimothyGu reviewed Dec 20, 2017

View reviewed changes

targos approved these changes Dec 20, 2017

View reviewed changes

gireeshpunathil approved these changes Dec 20, 2017

View reviewed changes

benjamingr approved these changes Dec 20, 2017

View reviewed changes

cjihrig approved these changes Dec 20, 2017

View reviewed changes

Trott added 4 commits December 20, 2017 06:16

test: remove common.projectDir

69eb221

common.projectDir is used in one test, so it's not so common. Remove from common module to the one test file that needs it.

test: remove getTTYfd() from common module

19bd327

common.getTTYfd() is used in one test only. Move it's definition to that test and out of the common module.

test: move firstInvalidFD() out of common module

cfa5ae0

common.firstInvalidFD() is used in only one test. Move it out of the common module and into the one test that uses it.

test: fix buggy getTTYfd() implementation

8be1259

Ref: nodejs#17781 (comment)

Trott force-pushed the uncommon-fn branch from ebf77f8 to 8be1259 Compare December 20, 2017 14:20

TimothyGu approved these changes Dec 20, 2017

View reviewed changes

maclover7 approved these changes Dec 20, 2017

View reviewed changes

benjamingr reviewed Dec 20, 2017

View reviewed changes

lpinca reviewed Dec 20, 2017

View reviewed changes

lpinca approved these changes Dec 20, 2017

View reviewed changes

jasnell approved these changes Dec 20, 2017

View reviewed changes

BridgeAR approved these changes Dec 21, 2017

View reviewed changes

BridgeAR added fast-track PRs that do not need to wait for 48 hours to land. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Dec 21, 2017

Trott removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 21, 2017

Trott closed this Dec 21, 2017

MylesBorins added the backport-requested-v9.x label Jan 9, 2018

targos removed the backport-requested-v9.x label Mar 24, 2018

BridgeAR added the backported-to-v9.x label Apr 30, 2018

MylesBorins added the dont-land-on-v8.x label Jul 31, 2018

Trott deleted the uncommon-fn branch January 13, 2022 22:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove uncommon functions from common module #17781

Remove uncommon functions from common module #17781

Trott commented Dec 20, 2017

Trott commented Dec 20, 2017

TimothyGu Dec 20, 2017 •

edited

Loading

benjamingr Dec 20, 2017

Trott Dec 20, 2017 •

edited

Loading

Trott Dec 20, 2017

Trott commented Dec 20, 2017

benjamingr Dec 20, 2017

lpinca Dec 20, 2017

lpinca Dec 20, 2017 •

edited

Loading

Trott commented Dec 20, 2017

Trott commented Dec 21, 2017

MylesBorins commented Jan 9, 2018

Trott commented Jan 11, 2018

Trott commented Jan 11, 2018

Remove uncommon functions from common module #17781

Remove uncommon functions from common module #17781

Conversation

Trott commented Dec 20, 2017

Checklist

Affected core subsystem(s)

Trott commented Dec 20, 2017

TimothyGu Dec 20, 2017 • edited Loading

Choose a reason for hiding this comment

benjamingr Dec 20, 2017

Choose a reason for hiding this comment

Trott Dec 20, 2017 • edited Loading

Choose a reason for hiding this comment

Trott Dec 20, 2017

Choose a reason for hiding this comment

Trott commented Dec 20, 2017

benjamingr Dec 20, 2017

Choose a reason for hiding this comment

lpinca Dec 20, 2017

Choose a reason for hiding this comment

lpinca Dec 20, 2017 • edited Loading

Choose a reason for hiding this comment

Trott commented Dec 20, 2017

Trott commented Dec 21, 2017

MylesBorins commented Jan 9, 2018

Trott commented Jan 11, 2018

Trott commented Jan 11, 2018

TimothyGu Dec 20, 2017 •

edited

Loading

Trott Dec 20, 2017 •

edited

Loading

lpinca Dec 20, 2017 •

edited

Loading