test: refactor test-repl by addaleax · Pull Request #17926 · nodejs/node

addaleax · 2017-12-31T21:10:03Z

Switch over to async tracking through promises/async fns
Remove an unused temp dir refresh
Inline the multiline/npm text prompts into expectations
Unify handling prompts/stripping prompts out
Make sure no unexpected data is received by requireing
all lines to be matched, rather than chunks received from
the REPL. This made the test too loose in terms of
matched lines and too strict in terms of stream chunking
requirements.
Some general cleanup

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

test/repl

- Switch over to async tracking through promises/async fns - Remove an unused temp dir refresh - Inline the multiline/npm text prompts into expectations - Unify handling prompts/stripping prompts out - Make sure no unexpected data is received by requireing all *lines* to be matched, rather than chunks received from the REPL. This made the test too loose in terms of matched lines and too strict in terms of stream chunking requirements. - Some general cleanup

addaleax · 2018-01-02T15:05:35Z

CI: https://ci.nodejs.org/job/node-test-commit/15176/

Trott

LGTM. Is this refactoring related to the issues @bmeck was running into? If so, awesome. If not, it might be good to have him give it a review.

bmeck · 2018-01-02T22:24:57Z

@Trott related in some ways, but a decent number are not related. LGTM

addaleax · 2018-01-02T22:25:42Z

@Trott The motivation here was the write chunking issue hinted at in the PR description. If this helps with something else, yay, even though that was not my original goal. :)

maclover7

This is a really nice revamp -- awesome work!

Left one comment about another possible source of refactoring, feel free to land without, is nonblocking.

addaleax · 2018-01-02T22:33:30Z

@maclover7 Sorry – I’m not seeing any comment here?

maclover7 · 2018-01-02T22:37:34Z

test/parallel/test-repl.js

+  }
+})();
+
+function startTCPRepl() {


@addaleax not sure why this didn't go through the first time, hopefully works this time!

Was just suggesting that maybe a factory method could be pulled out of startTCPRepl and startUnixRepl, it seems like there is some duplicated code there.

@maclover7 Yeah, I agree :) It’s not 100 % trivial but it would sure be nice to have that! I might do it when I have the time but I think this is okay for now :)

maclover7 · 2018-01-06T13:48:47Z

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

addaleax · 2018-01-06T21:22:17Z

Landed in d1ad4a9

- Switch over to async tracking through promises/async fns - Remove an unused temp dir refresh - Inline the multiline/npm text prompts into expectations - Unify handling prompts/stripping prompts out - Make sure no unexpected data is received by requireing all *lines* to be matched, rather than chunks received from the REPL. This made the test too loose in terms of matched lines and too strict in terms of stream chunking requirements. - Some general cleanup PR-URL: #17926 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Bradley Farias <bradley.meck@gmail.com> Reviewed-By: Jon Moss <me@jonathanmoss.me> Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins · 2018-01-09T23:48:29Z

This does not land cleanly on v9.x, could it be backported?

- Switch over to async tracking through promises/async fns - Remove an unused temp dir refresh - Inline the multiline/npm text prompts into expectations - Unify handling prompts/stripping prompts out - Make sure no unexpected data is received by requireing all *lines* to be matched, rather than chunks received from the REPL. This made the test too loose in terms of matched lines and too strict in terms of stream chunking requirements. - Some general cleanup PR-URL: nodejs#17926

- Switch over to async tracking through promises/async fns - Remove an unused temp dir refresh - Inline the multiline/npm text prompts into expectations - Unify handling prompts/stripping prompts out - Make sure no unexpected data is received by requireing all *lines* to be matched, rather than chunks received from the REPL. This made the test too loose in terms of matched lines and too strict in terms of stream chunking requirements. - Some general cleanup Backport-PR-URL: #18082 PR-URL: #17926 Reviewed-By: Evan Lucas <evanlucas@me.com>

- Switch over to async tracking through promises/async fns - Remove an unused temp dir refresh - Inline the multiline/npm text prompts into expectations - Unify handling prompts/stripping prompts out - Make sure no unexpected data is received by requireing all *lines* to be matched, rather than chunks received from the REPL. This made the test too loose in terms of matched lines and too strict in terms of stream chunking requirements. - Some general cleanup PR-URL: #17926 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Bradley Farias <bradley.meck@gmail.com> Reviewed-By: Jon Moss <me@jonathanmoss.me> Reviewed-By: James M Snell <jasnell@gmail.com>

nodejs-github-bot added the test Issues and PRs related to the tests. label Dec 31, 2017

addaleax added repl Issues and PRs related to the REPL subsystem. dont-land-on-v6.x labels Dec 31, 2017

addaleax requested a review from Trott January 2, 2018 15:04

cjihrig approved these changes Jan 2, 2018

View reviewed changes

Trott approved these changes Jan 2, 2018

View reviewed changes

bmeck approved these changes Jan 2, 2018

View reviewed changes

maclover7 approved these changes Jan 2, 2018

View reviewed changes

maclover7 reviewed Jan 2, 2018

View reviewed changes

maclover7 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 6, 2018

jasnell approved these changes Jan 6, 2018

View reviewed changes

addaleax closed this Jan 6, 2018

addaleax deleted the refactor-test-repl branch January 6, 2018 21:24

addaleax mentioned this pull request Jan 6, 2018

src: refactor HasWriteQueue() away #18019

Closed

2 tasks

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

addaleax added backported-to-v9.x and removed backport-requested-v9.x author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Jan 10, 2018

addaleax mentioned this pull request Jan 10, 2018

[v9.x] test: refactor test-repl #18082

Closed

4 tasks

evanlucas mentioned this pull request Jan 30, 2018

v9.5.0 release proposal #18464

Merged

MylesBorins added the backported-to-v8.x label May 22, 2018

MylesBorins mentioned this pull request Jul 9, 2018

v8.12.0 proposal #21593

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

test: refactor test-repl#17926

test: refactor test-repl#17926
addaleax wants to merge 1 commit intonodejs:masterfrom
addaleax:refactor-test-repl

addaleax commented Dec 31, 2017

Uh oh!

addaleax commented Jan 2, 2018

Uh oh!

Trott left a comment

Uh oh!

bmeck commented Jan 2, 2018

Uh oh!

addaleax commented Jan 2, 2018

Uh oh!

maclover7 left a comment

Uh oh!

addaleax commented Jan 2, 2018

Uh oh!

maclover7 Jan 2, 2018

Uh oh!

addaleax Jan 2, 2018

Uh oh!

maclover7 commented Jan 6, 2018

Uh oh!

addaleax commented Jan 6, 2018

Uh oh!

MylesBorins commented Jan 9, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

Uh oh!

Conversation

addaleax commented Dec 31, 2017

Checklist

Affected core subsystem(s)

Uh oh!

addaleax commented Jan 2, 2018

Uh oh!

Trott left a comment

Choose a reason for hiding this comment

Uh oh!

bmeck commented Jan 2, 2018

Uh oh!

addaleax commented Jan 2, 2018

Uh oh!

maclover7 left a comment

Choose a reason for hiding this comment

Uh oh!

addaleax commented Jan 2, 2018

Uh oh!

maclover7 Jan 2, 2018

Choose a reason for hiding this comment

Uh oh!

addaleax Jan 2, 2018

Choose a reason for hiding this comment

Uh oh!

maclover7 commented Jan 6, 2018

Uh oh!

addaleax commented Jan 6, 2018

Uh oh!

MylesBorins commented Jan 9, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants