Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

tools: add node-pty as a dependency #47793

Closed
wants to merge 2 commits into from
Closed

tools: add node-pty as a dependency #47793

wants to merge 2 commits into from

Conversation

MoLow
Copy link
Member

@MoLow MoLow commented Apr 30, 2023 •
edited
Loading

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
Copy link
Collaborator

Review requested:

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

@nodejs-github-bot 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 MoLow force-pushed the pseudo-tty-tests branch 2 times, most recently from 8c96ecf to dc38eb4 Compare April 30, 2023 19:44
@anonrig
Copy link
Member

anonrig commented Apr 30, 2023 •
edited
Loading

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

anonrig
anonrig reviewed Apr 30, 2023
View reviewed changes
Copy link
Member

@anonrig anonrig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

License file is missing

anonrig
anonrig reviewed Apr 30, 2023
View reviewed changes
test/common/assertSnapshot.js Show resolved Hide resolved
test/parallel/test-runner-output.mjs
@@ -14,12 +14,13 @@ function replaceSpecDuration(str) {
return str
.replaceAll(/\(0(\r?\n)ms\)/g, '(ZEROms)')
.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');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

test/parallel/test-runner-output.mjs
}
const defaultTransform = snapshot
.transform(snapshot.replaceWindowsLineEndings, snapshot.replaceStackTrace, replaceTestDuration);
const specTransform = snapshot
.transform(snapshot.replaceWindowsLineEndings, snapshot.replaceStackTrace, replaceSpecDuration);
.transform(replaceSpecDuration, snapshot.replaceWindowsLineEndings, snapshot.replaceStackTrace);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Show resolved Hide resolved
test/common/assertSnapshot.js Show resolved Hide resolved
@mscdex
Copy link
Contributor

mscdex commented May 1, 2023

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

@MoLow
Copy link
Member Author

MoLow commented May 1, 2023

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
tniessen reviewed May 1, 2023
View reviewed changes
Copy link
Member

@tniessen tniessen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should probably be updated.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why? it still returns Promise<void>

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member Author

MoLow commented May 1, 2023

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
Copy link
Member Author

MoLow commented May 1, 2023

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

@benjamingr
Copy link
Member 

                                      ^
../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);
         ^

@mscdex mscdex changed the title deps: add node-pty as a dependency tools: add node-pty as a dependency May 1, 2023
@MoLow
Copy link
Member Author

MoLow commented May 1, 2023

closing in favor of #47803, wich is much simpler

@MoLow MoLow closed this May 1, 2023
@MoLow MoLow deleted the pseudo-tty-tests branch May 1, 2023 18:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tniessen tniessen tniessen left review comments

@anonrig anonrig anonrig left review comments

Assignees
No one assigned
Labels
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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@MoLow @nodejs-github-bot @anonrig @mscdex @benjamingr @tniessen