-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
test: adding history regression test case #24843
Conversation
@antsmartian sadly an error occured when I tried to trigger a build :( |
912aa88
to
078dfa8
Compare
Rebuild CI Lite : https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/1862/ |
@BridgeAR ping... |
@nodejs/repl @BridgeAR Gentle remainder 🔉 |
}, | ||
{ | ||
env: {}, | ||
test: [UP, UP, UP, DOWN, DOWN, DOWN], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest to add one more UP
and one more DOWN
so we reach the end of the list in both cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, taken care of this. Guess should be good now.. cc @BridgeAR
078dfa8
to
9d7ce13
Compare
@BridgeAR Can you please just have a look now, have addressed your comments and I wanted to take your review once before I land this to master. |
Still LG but please run a regular CI |
Yeah sure, thanks much @BridgeAR. |
Landed in 0bf7d41 |
PR-URL: nodejs#24843 Refs: nodejs#24385 Reviewed-By: Ruben Bridgewater <[email protected]>
Thanks @Trott |
PR-URL: #24843 Refs: #24385 Reviewed-By: Ruben Bridgewater <[email protected]>
PR-URL: nodejs#24843 Refs: nodejs#24385 Reviewed-By: Ruben Bridgewater <[email protected]>
PR-URL: nodejs#24843 Refs: nodejs#24385 Reviewed-By: Ruben Bridgewater <[email protected]>
PR-URL: #24843 Refs: #24385 Reviewed-By: Ruben Bridgewater <[email protected]>
PR-URL: #24843 Refs: #24385 Reviewed-By: Ruben Bridgewater <[email protected]>
PR-URL: #24843 Refs: #24385 Reviewed-By: Ruben Bridgewater <[email protected]>
Recently added mulitline caused a issue in history navigation ref: #24385
Fundamentally, we don't had a test case for navigation till now, the one I see here: https://github.com/nodejs/node/blob/master/test/parallel/test-repl-persistent-history.js#L97, just tests one level navigation.
cc @nodejs/repl @BridgeAR
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes