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

tools: add node-pty as a dependency #47793

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

Closed
MoLow wants to merge 2 commits into from
Closed

tools: add node-pty as a dependency #47793

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7f92599
tools: add node-pty as a dependency
MoLow Apr 29, 2023
5ddce6e
test: migrate pseudo_tty tests to use assertSnapshot
MoLow Apr 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions .github/workflows/tools.yml
View file
Open in desktop
9 changes: 8 additions & 1 deletion Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -478,6 +478,13 @@ benchmark/napi/.buildstamp: $(ADDONS_PREREQS) \
$(BENCHMARK_NAPI_BINDING_GYPS) $(BENCHMARK_NAPI_BINDING_SOURCES)
@$(call run_build_addons,"$$PWD/benchmark/napi",$@)

tools/node_modules/node-pty/.buildstamp:
@$(call run_build_addons,"$$PWD/tools/node_modules/",$@)

.PHONY: build-node-pty
build-node-pty: | $(NODE_EXE) tools/node_modules/node-pty/.buildstamp


.PHONY: clear-stalled
clear-stalled:
$(info Clean up any leftover processes but don't error if found.)
Expand Down Expand Up @@ -545,7 +552,7 @@ test-ci-js: | clear-stalled
.PHONY: test-ci
# Related CI jobs: most CI tests, excluding node-test-commit-arm-fanned
test-ci: LOGLEVEL := info
test-ci: | clear-stalled bench-addons-build build-addons build-js-native-api-tests build-node-api-tests doc-only
test-ci: | clear-stalled bench-addons-build build-addons build-js-native-api-tests build-node-api-tests build-node-pty doc-only
out/Release/cctest --gtest_output=xml:out/junit/cctest.xml
$(PYTHON) tools/test.py $(PARALLEL_ARGS) -p tap --logfile test.tap \
--mode=$(BUILDTYPE_LOWER) --flaky-tests=$(FLAKY_TESTS) \
Expand Down
37 changes: 34 additions & 3 deletions test/common/assertSnapshot.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,11 @@ const common = require('.');
const path = require('node:path');
const fs = require('node:fs/promises');
const assert = require('node:assert/strict');
const pty = require('../../tools/node_modules/node-pty');


const stackFramesRegexp = /(\s+)((.+?)\s+\()?(?:\(?(.+?):(\d+)(?::(\d+))?)\)?(\s+\{)?(\n|$)/g;
// eslint-disable-next-line no-control-regex
const stackFramesRegexp = /(\s+)((.+?)\s+\()?(?:\(?(.+?):(\d+)(?::(\d+))?)\)?(\s+\{)?(\u001b\[\d+m)?(\n|$)/g;
const windowNewlineRegexp = /\r/g;

function replaceStackTrace(str, replacement = '$1*$7\n') {
Expand Down Expand Up @@ -39,6 +41,33 @@ async function assertSnapshot(actual, filename = process.argv[1]) {
}
}

function spawnPTYPromisifyied(...args) {
const stderr = '';
let stdout = '';

const child = pty.spawn(...args);
child.onData((data) => { stdout += data; });

return new Promise((resolve, reject) => {
child.on('close', (code, signal) => {
Comment thread
MoLow marked this conversation as resolved.
resolve({
code,
signal,
stderr,
stdout,
});
});
child.on('error', (code, signal) => {
Comment thread
MoLow marked this conversation as resolved.
reject({
code,
signal,
stderr,
stdout,
});
});
});
}

/**
* Spawn a process and assert its output against a snapshot.
* if you want to automatically update the snapshot, run tests with NODE_REGENERATE_SNAPSHOTS=1
Expand All @@ -53,9 +82,11 @@ async function assertSnapshot(actual, filename = process.argv[1]) {
* @param {function(string): string} [transform]
* @returns {Promise<void>}

@tniessen tniessen May 1, 2023

Copy link
Copy Markdown
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.

@MoLow MoLow May 1, 2023

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

@tniessen tniessen May 1, 2023

Copy link
Copy Markdown
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?

*/
async function spawnAndAssert(filename, transform = (x) => x) {
async function spawnAndAssert(filename, transform = (x) => x, options = {}) {
const flags = common.parseTestFlags(filename);
const { stdout, stderr } = await common.spawnPromisified(process.execPath, [...flags, filename]);
const { stdout, stderr } = await (options.tty ?
spawnPTYPromisifyied(process.execPath, [...flags, filename]) :
Comment thread
MoLow marked this conversation as resolved.
common.spawnPromisified(process.execPath, [...flags, filename]));
await assertSnapshot(transform(`${stdout}${stderr}`), filename);
}

Expand Down
2 changes: 1 addition & 1 deletion ...seudo-tty/test_runner_default_reporter.js → ...ures/test-runner/output/default_output.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ process.env.FORCE_COLOR = '1';
delete process.env.NODE_DISABLE_COLORS;
delete process.env.NO_COLOR;

require('../common');
require('../../../common');
const test = require('node:test');

test('should pass', () => {});
Expand Down
57 changes: 57 additions & 0 deletions test/fixtures/test-runner/output/default_output.snapshot
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
[32m✔ should pass [90m(*ms)[39m[39m
[31m✖ should fail [90m(*ms)[39m[39m
Error: fail
*
*
*
*
*
*
*

[90m﹣ should skip [90m(*ms)[39m # SKIP[39m
▶ parent
[31m✖ should fail [90m(*ms)[39m[39m
Error: fail
*
*
*
*
*

[31m✖ should pass but parent fail [90m(*ms)[39m[39m
[32m'test did not finish before its parent and was cancelled'[39m

[31m▶ [39mparent [90m(*ms)[39m

[34mℹ tests 6[39m
[34mℹ suites 0[39m
[34mℹ pass 1[39m
[34mℹ fail 3[39m
[34mℹ cancelled 1[39m
[34mℹ skipped 1[39m
[34mℹ todo 0[39m
[34mℹ duration_ms *[39m

[31m✖ failing tests:[39m

[31m✖ should fail [90m(*ms)[39m[39m
Error: fail
*
*
*
*
*
*
*

[31m✖ should fail [90m(*ms)[39m[39m
Error: fail
*
*
*
*
*

[31m✖ should pass but parent fail [90m(*ms)[39m[39m
[32m'test did not finish before its parent and was cancelled'[39m
10 changes: 6 additions & 4 deletions test/parallel/test-runner-output.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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');

@anonrig anonrig Apr 30, 2023

Copy link
Copy Markdown
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?

}
const defaultTransform = snapshot
.transform(snapshot.replaceWindowsLineEndings, snapshot.replaceStackTrace, replaceTestDuration);
const specTransform = snapshot
.transform(snapshot.replaceWindowsLineEndings, snapshot.replaceStackTrace, replaceSpecDuration);
.transform(replaceSpecDuration, snapshot.replaceWindowsLineEndings, snapshot.replaceStackTrace);

@anonrig anonrig Apr 30, 2023

Copy link
Copy Markdown
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?



const tests = [
Expand All @@ -40,10 +41,11 @@ const tests = [
{ name: 'test-runner/output/name_pattern.js' },
{ name: 'test-runner/output/name_pattern_with_only.js' },
{ name: 'test-runner/output/unresolved_promise.js' },
].map(({ name, transform }) => ({
{ name: 'test-runner/output/default_output.js', transform: specTransform, tty: true },
].map(({ name, tty, transform }) => ({
name,
fn: common.mustCall(async () => {
await snapshot.spawnAndAssert(fixtures.path(name), transform ?? defaultTransform);
await snapshot.spawnAndAssert(fixtures.path(name), transform ?? defaultTransform, { tty });
}),
}));

Expand Down
57 changes: 0 additions & 57 deletions test/pseudo-tty/test_runner_default_reporter.out
View file
Open in desktop

This file was deleted.

43 changes: 43 additions & 0 deletions tools/dep_updaters/update-node-pty.sh
View file
Open in desktop
2 changes: 2 additions & 0 deletions tools/node_modules/node-pty/.gitignore
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

69 changes: 69 additions & 0 deletions tools/node_modules/node-pty/LICENSE
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading