tools: add node-pty as a dependency by MoLow · Pull Request #47793 · nodejs/node

MoLow · 2023-04-30T18:43:28Z

this is another followup for #47498 enabling running pseudo-tty snapshot tests using https://github.com/microsoft/node-pty
see #47793 (comment) for the reasoning behind this change

nodejs-github-bot · 2023-04-30T18:43:32Z

Review requested:

@nodejs/actions
@nodejs/gyp
@nodejs/test_runner

anonrig · 2023-04-30T21:35:01Z

@MoLow Can you add an eslint rule to inhibit requiring/using node-pty inside lib folder?

anonrig

License file is missing

test/common/assertSnapshot.js

anonrig · 2023-04-30T21:38:03Z

test/parallel/test-runner-output.mjs

    .replaceAll(/[0-9.]+ms/g, '*ms')
-    .replaceAll(/duration_ms [0-9.]+/g, 'duration_ms *');
+    .replaceAll(/duration_ms [0-9.]+/g, 'duration_ms *')
+    .replace(new RegExp(`(\\u001b\\[\\d+m)\\(${process.cwd()}/?(\\u001b\\[\\d+m)(.*)(\\u001b\\[\\d+m)\\)`, 'g'), '$3');


Can you add a comment to this line and explain what it does?

anonrig · 2023-04-30T21:38:29Z

test/parallel/test-runner-output.mjs

  .transform(snapshot.replaceWindowsLineEndings, snapshot.replaceStackTrace, replaceTestDuration);
 const specTransform = snapshot
-  .transform(snapshot.replaceWindowsLineEndings, snapshot.replaceStackTrace, replaceSpecDuration);
+  .transform(replaceSpecDuration, snapshot.replaceWindowsLineEndings, snapshot.replaceStackTrace);


Why did we have to change the ordering of this transform? Would it be helpful to document this with a comment?

test/common/assertSnapshot.js

mscdex · 2023-05-01T02:47:55Z

To avoid confusion with actual deps/ directory changes, I think tools: would be a more appropriate commit message prefix.

MoLow · 2023-05-01T14:17:48Z

License file is missing

I think that is relevant for deps, not for tools, see #47793 (comment)
(the license file itself exists but is not exposed, which is ok)

tniessen

Could you explain what problem node-pty solves here, and how it worked before?

tniessen · 2023-05-01T14:31:03Z

test/common/assertSnapshot.js

@@ -53,9 +82,11 @@ async function assertSnapshot(actual, filename = process.argv[1]) {
 * @param {function(string): string} [transform]
 * @returns {Promise<void>}


Should probably be updated.

why? it still returns Promise<void>

Huh, either GitHub or myself messed up the reference. I meant to select the entire doc comment, not just the @returns line. Specifically, shouldn't a @param be added?

MoLow · 2023-05-01T14:47:12Z

Could you explain what problem node-pty solves here, and how it worked before?

Sure. the current pseudo-tty test suite uses an equivilent implementation for this from python:

node/test/pseudo-tty/pty_helper.py

Line 3 in d225d95

import pty

as part of the effort of migrating the message tests to use common.assertSnapshot this will allow running as any other test under tests/parallel.

common.assertSnapshot has a few advantages over the current python snapshot tests (it is faster, the producing snapshots is not doen maually so it is both easier and deterministic)

MoLow · 2023-05-01T16:10:50Z

@nodejs/node-api any idea why the build is failing on mac? https://github.com/nodejs/node/actions/runs/4846084398/jobs/8635397424

benjamingr · 2023-05-01T16:31:55Z

                                      ^
../src/unix/pty.cc:669:10: error: use of undeclared identifier 'openpty'
  return openpty(amaster, aslave, name, (termios *)termp, (winsize *)winp);
         ^
../src/unix/pty.cc:717:10: error: use of undeclared identifier 'forkpty'
  return forkpty(amaster, name, (termios *)termp, (winsize *)winp);
         ^

MoLow · 2023-05-01T18:02:07Z

closing in favor of #47803, wich is much simpler

nodejs-github-bot added build Issues and PRs related to build files or the CI. meta Issues and PRs related to the general management of the project. needs-ci PRs that need a full CI run. tools Issues and PRs related to the tools directory. labels Apr 30, 2023

MoLow force-pushed the pseudo-tty-tests branch 2 times, most recently from 8c96ecf to dc38eb4 Compare April 30, 2023 19:44

anonrig reviewed Apr 30, 2023

View reviewed changes

tniessen reviewed May 1, 2023

View reviewed changes

MoLow added 2 commits May 1, 2023 18:05

tools: add node-pty as a dependency

7f92599

test: migrate pseudo_tty tests to use assertSnapshot

5ddce6e

MoLow force-pushed the pseudo-tty-tests branch from dc38eb4 to 5ddce6e Compare May 1, 2023 16:10

mscdex changed the title ~~deps: add node-pty as a dependency~~ tools: add node-pty as a dependency May 1, 2023

MoLow closed this May 1, 2023

MoLow deleted the pseudo-tty-tests branch May 1, 2023 18:02

		@@ -53,9 +82,11 @@ async function assertSnapshot(actual, filename = process.argv[1]) {
		* @param {function(string): string} [transform]
		* @returns {Promise<void>}

Uh oh!

Conversation

MoLow commented Apr 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Apr 30, 2023

Uh oh!

anonrig commented Apr 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

anonrig left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

anonrig Apr 30, 2023

Choose a reason for hiding this comment

Uh oh!

anonrig Apr 30, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mscdex commented May 1, 2023

Uh oh!

MoLow commented May 1, 2023

Uh oh!

tniessen left a comment

Choose a reason for hiding this comment

Uh oh!

tniessen May 1, 2023

Choose a reason for hiding this comment

Uh oh!

MoLow May 1, 2023

Choose a reason for hiding this comment

Uh oh!

tniessen May 1, 2023

Choose a reason for hiding this comment

Uh oh!

MoLow commented May 1, 2023

Uh oh!

MoLow commented May 1, 2023

Uh oh!

benjamingr commented May 1, 2023

Uh oh!

MoLow commented May 1, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

MoLow commented Apr 30, 2023 •

edited

Loading

anonrig commented Apr 30, 2023 •

edited

Loading