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.

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

tools: add node-pty as a dependency #47793

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ on:
- nghttp2
- nghttp3
- ngtcp2
- node-pty
- postject
- root-certificates
- simdutf
Expand Down Expand Up @@ -252,6 +253,14 @@ jobs:
cat temp-output
tail -n1 temp-output | grep "NEW_VERSION=" >> "$GITHUB_ENV" || true
rm temp-output
- id: node-pty
subsystem: tools
label: test
run: |
./tools/dep_updaters/update-node-pty.sh > temp-output
cat temp-output
tail -n1 temp-output | grep "NEW_VERSION=" >> "$GITHUB_ENV" || true
rm temp-output
steps:
- uses: actions/checkout@ac593985615ec2ede58e132d2e21d2b1cbd6127c # v3.3.0
if: github.event_name == 'schedule' || inputs.id == 'all' || inputs.id == matrix.id
Expand Down
9 changes: 8 additions & 1 deletion Makefile
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
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) => {
resolve({
code,
signal,
stderr,
stdout,
});
});
child.on('error', (code, signal) => {
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>}
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?

*/
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]) :
common.spawnPromisified(process.execPath, [...flags, filename]));
await assertSnapshot(transform(`${stdout}${stderr}`), filename);
}

Expand Down
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
✔ should pass (*ms)
✖ should fail (*ms)
Error: fail
*
*
*
*
*
*
*

﹣ should skip (*ms) # SKIP
▶ parent
✖ should fail (*ms)
Error: fail
*
*
*
*
*

✖ should pass but parent fail (*ms)
'test did not finish before its parent and was cancelled'

▶ parent (*ms)

ℹ tests 6
ℹ suites 0
ℹ pass 1
ℹ fail 3
ℹ cancelled 1
ℹ skipped 1
ℹ todo 0
ℹ duration_ms *

✖ failing tests:

✖ should fail (*ms)
Error: fail
*
*
*
*
*
*
*

✖ should fail (*ms)
Error: fail
*
*
*
*
*

✖ should pass but parent fail (*ms)
'test did not finish before its parent and was cancelled'
10 changes: 6 additions & 4 deletions test/parallel/test-runner-output.mjs
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');
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?

}
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?



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

This file was deleted.

43 changes: 43 additions & 0 deletions tools/dep_updaters/update-node-pty.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
#!/bin/sh

# Shell script to update node-pty in the source tree to the latest release.

# This script must be in the tools directory when it runs because it uses the
# script source file path to determine directories to work in.

set -ex

ROOT=$(cd "$(dirname "$0")/../.." && pwd)

[ -z "$NODE" ] && NODE="$ROOT/out/Release/node"
[ -x "$NODE" ] || NODE=$(command -v node)
NPM="$ROOT/deps/npm/bin/npm-cli.js"

NEW_VERSION=$("$NODE" "$NPM" view node-pty latest)
CURRENT_VERSION=$("$NODE" -p "require('./tools/node_modules/node-pty/package.json').version")

if [ "$NEW_VERSION" = "$CURRENT_VERSION" ]; then
echo "Skipped because node-pty is on the latest version."
exit 0
fi

cd "$( dirname "$0" )" || exit
rm -rf ../node_modules/node-pty
(
rm -rf node-pty-tmp
mkdir node-pty-tmp
cd node-pty-tmp || exit

"$NODE" "$NPM" init --yes
"$NODE" "$NPM" install --global-style --no-bin-links --ignore-scripts node-pty
)

mv node-pty-tmp/node_modules/node-pty ../node_modules/node-pty
rm -rf node-pty-tmp/

echo ".buildstamp
build/" >> ../node_modules/node-pty/.gitignore

# The last line of the script should always print the new version,
# as we need to add it to $GITHUB_ENV variable.
echo "NEW_VERSION=$NEW_VERSION"
2 changes: 2 additions & 0 deletions tools/node_modules/node-pty/.gitignore

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

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

Loading