repl: fix/improve persistent repl history by Fishrock123 · Pull Request #2356 · nodejs/node · GitHub

Fix the history limiting to take the most recent. History additions are unshifted in readline.

Now tests the saved repl history size limiting.

Cleans up the test a bunch in the process, and also adds some debug so you can figure out which test case failed if it fails. Let me know if there is a better way to do that.

Still failing for me.

Failed test # 8

/Users/evan/dev/code/forks/io.js/test/sequential/test-repl-persistent-history.js:197
          throw err;
          ^
AssertionError: '> \'=^.^=\'' === '> \'42\''
    at Writable.write [as _write] (/Users/evan/dev/code/forks/io.js/test/sequential/test-repl-persistent-history.js:194:18)
    at doWrite (_stream_writable.js:292:12)
    at writeOrBuffer (_stream_writable.js:278:5)
    at Writable.write (_stream_writable.js:207:11)
    at REPLServer._writeToOutput (readline.js:219:17)
    at REPLServer.Interface._refreshLine (readline.js:259:8)
    at REPLServer.Interface._historyPrev (readline.js:572:10)
    at REPLServer.Interface._ttyWrite (readline.js:858:14)
    at ActionStream.onkeypress (readline.js:105:10)
    at emitTwo (events.js:87:13)

Yep, that fixes it for me. We should probably run CI though to be safe.

CI: https://jenkins-iojs.nodesource.com/job/node-test-pull-request/85/

LGTM if the CI is happy

Hmm, windows isn't liking the cleanup, maybe I'll just have to refresh the tmpdir.

not ok 875 - test-repl-persistent-history.js
# c:\workspace\node-test-commit-windows\nodes\win2012r2\test\sequential\test-repl-persistent-history.js:213
# if (err) throw err;
# ^
# 
# Error: EPERM: operation not permitted, open 'c:\workspace\node-test-commit-windows\nodes\win2012r2\test\tmp.0\.node_repl_history'
# at Error (native)

New CI: https://jenkins-iojs.nodesource.com/job/node-test-pull-request/86/

Uh, ok.. Hrmmm. New windows (and ARM) failure from common.refreshTmpDir()...

not ok 875 - test-repl-persistent-history.js
# c:\workspace\node-test-commit-windows\nodes\win2012r2\test\common.js:34
# throw e;
# ^
# 
# Error: ENOTEMPTY: directory not empty, rmdir 'c:\workspace\node-test-commit-windows\nodes\win2012r2\test\tmp.0'
# at Error (native)
# at Object.fs.rmdirSync (fs.js:763:18)
# at rmdirSync (c:\workspace\node-test-commit-windows\nodes\win2012r2\test\common.js:49:10)
# at rimrafSync (c:\workspace\node-test-commit-windows\nodes\win2012r2\test\common.js:25:7)
# at Object.exports.refreshTmpDir (c:\workspace\node-test-commit-windows\nodes\win2012r2\test\common.js:55:3)
# at REPLServer.<anonymous> (c:\workspace\node-test-commit-windows\nodes\win2012r2\test\sequential\test-repl-persistent-history.js:211:16)
# at emitNone (events.js:72:20)
# at REPLServer.emit (events.js:166:7)
# at REPLServer.Interface.close (readline.js:285:8)
# at REPLServer.replClose [as close] (repl.js:486:29)

I think it needed to wait for I/O to close. Fingers crossed...

CI again: https://jenkins-iojs.nodesource.com/job/node-test-pull-request/88/

CI still not happy on Windows :[

not ok 875 - test-repl-persistent-history.js
# c:\workspace\node-test-commit-windows\nodes\win2008r2\test\common.js:34
# throw e;
# ^
# 
# Error: ENOTEMPTY: directory not empty, rmdir 'c:\workspace\node-test-commit-windows\nodes\win2008r2\test\tmp.0'
# at Error (native)
# at Object.fs.rmdirSync (fs.js:763:18)
# at rmdirSync (c:\workspace\node-test-commit-windows\nodes\win2008r2\test\common.js:49:10)
# at rimrafSync (c:\workspace\node-test-commit-windows\nodes\win2008r2\test\common.js:25:7)
# at Object.exports.refreshTmpDir (c:\workspace\node-test-commit-windows\nodes\win2008r2\test\common.js:55:3)
# at Immediate.setupTest (c:\workspace\node-test-commit-windows\nodes\win2008r2\test\sequential\test-repl-persistent-history.js:163:23)
# at Immediate.immediate._onImmediate (timers.js:423:18)
# at processImmediate [as _immediateCallback] (timers.js:371:17)

I'm not sure what's up with that.

Somehow this is failing: https://github.com/nodejs/node/blob/master/test/common.js#L15-L57

Specifically: https://github.com/nodejs/node/blob/master/test/common.js#L45-L48

I thought it might have been if the history file was written to after it starts clearing the files but I don't think that is possible...

Might be a Windows quirk: isaacs/rimraf#72

I think it must be intermittent, but we should still attempt an workaround later. Maybe give the CI a few more runs?

@Fishrock123 any update on this?

@evanlucas on the back burner right now. I think It can be fixed by replacing the refreshTmpDir with a function that checks if the file exists with a stat, and then unlinks if it does.

I think this can be worked around by running the rmDirSync a few times. I dealt with these Windows issues before and I think running 10-20 times usually does the job. It's an ugly hack indeed.

Hopefully this last patch fixed those stupid windows issues. https://jenkins-iojs.nodesource.com/job/node-test-pull-request/182/

Hmm, the rebase failed on that. here's a manual run: https://jenkins-iojs.nodesource.com/job/node-test-commit/339/

Ugh. EPERM. Whatever that means in this context.

Might as well install a full rimraf and be done with it, I know that it can handle EPERM at least.

https://jenkins-iojs.nodesource.com/job/node-test-commit/350/

Ok this EPERM isn't coming from the unlinkSync though. I think it's coming from fs.open in internal/repl...?

This test is already being investigated, but until a solution is found it should be marked flaky. Ref: nodejs#2319 Ref: nodejs#2356

This test is already being investigated, but until a solution is found it should be marked flaky. Ref: #2319 Ref: #2356 PR-URL: #2659 Reviewed-By: orangemocha - Alexis Campailla <orangemocha@nodejs.org>

This test is already being investigated, but until a solution is found it should be marked flaky. Ref: nodejs#2319 Ref: nodejs#2356 PR-URL: nodejs#2659 Reviewed-By: orangemocha - Alexis Campailla <orangemocha@nodejs.org>

cc @nodejs/collaborators PTAL at the last commit

@silverwind @evanlucas can I get a last LGTM on this? this works without any even minor breaking changes now.

Ok, just pulled down and tested. I've run the tests hundreds of times now and haven't had them fail.

LGTM

- Now cleans up the history file unless told otherwise. - Now also logs which test case failed. - Waits for flush after repl close if necessary. Fixes: nodejs#2319 PR-URL: nodejs#2356 Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed By: Evan Lucas <evanlucas@me.com>

Previously the wrong end of the history was limited on load. PR-URL: nodejs#2356 Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed By: Evan Lucas <evanlucas@me.com>

Thanks, landed in d8db757...73b7e05

@nodejs/lts this is something we want in v4.x.

- Now cleans up the history file unless told otherwise. - Now also logs which test case failed. - Waits for flush after repl close if necessary. Fixes: #2319 PR-URL: #2356 Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed By: Evan Lucas <evanlucas@me.com>

Previously the wrong end of the history was limited on load. PR-URL: #2356 Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed By: Evan Lucas <evanlucas@me.com>

Emitting 'close' before the history has flushed is somewhat incorrect and rather confusing. This also makes the 'close' event always asynchronous for consistency. Refs: nodejs#2356 PR-URL: nodejs#3435 Reviewed By: Evan Lucas <evanlucas@me.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com>

Emitting 'close' before the history has flushed is somewhat incorrect and rather confusing. This also makes the 'close' event always asynchronous for consistency. Refs: #2356 PR-URL: #3435 Reviewed By: Evan Lucas <evanlucas@me.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com>

- Now cleans up the history file unless told otherwise. - Now also logs which test case failed. - Waits for flush after repl close if necessary. Fixes: #2319 PR-URL: #2356 Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed By: Evan Lucas <evanlucas@me.com>

Previously the wrong end of the history was limited on load. PR-URL: #2356 Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed By: Evan Lucas <evanlucas@me.com>

Landed in v4.x-staging in 9fa8c32 and 594e8e7

- Now cleans up the history file unless told otherwise. - Now also logs which test case failed. - Waits for flush after repl close if necessary. Fixes: #2319 PR-URL: #2356 Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed By: Evan Lucas <evanlucas@me.com>

Previously the wrong end of the history was limited on load. PR-URL: #2356 Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed By: Evan Lucas <evanlucas@me.com>

Fishrock123 added the repl Issues and PRs related to the REPL subsystem. label Aug 12, 2015

Fishrock123 mentioned this pull request Aug 13, 2015

test: fix race condition in repl history test #2358

Closed

Fishrock123 mentioned this pull request Aug 18, 2015

Windows support (ENOTEMPTY issues) isaacs/rimraf#72

Closed

joaocgreis added a commit to JaneaSystems/node that referenced this pull request Sep 2, 2015

test: mark test-repl-persistent-history as flaky

dfbf0cd

This test is already being investigated, but until a solution is found it should be marked flaky. Ref: nodejs#2319 Ref: nodejs#2356

joaocgreis mentioned this pull request Sep 2, 2015

test: mark test-repl-persistent-history as flaky #2659

Closed

Fishrock123 mentioned this pull request Sep 2, 2015

sequential/test-repl-persistent-history is failing on OS X #2319

Closed

Fishrock123 mentioned this pull request Oct 19, 2015

repl: event ordering: delay 'close' until 'flushHistory' #3435

Merged

Fishrock123 force-pushed the improve-repl-history branch from 17ad616 to d4d9460 Compare October 19, 2015 15:34

Fishrock123 reviewed Oct 19, 2015
View reviewed changes

Fishrock123 force-pushed the improve-repl-history branch from d4d9460 to d4dccd1 Compare October 19, 2015 15:53

Fishrock123 added 2 commits October 19, 2015 11:53

repl: limit persistent history correctly on load

73b7e05

Previously the wrong end of the history was limited on load. PR-URL: nodejs#2356 Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed By: Evan Lucas <evanlucas@me.com>

Fishrock123 force-pushed the improve-repl-history branch from d4dccd1 to 73b7e05 Compare October 19, 2015 15:54

Fishrock123 merged commit 73b7e05 into nodejs:master Oct 19, 2015

Fishrock123 added the land-on-v4.x label Oct 19, 2015

Fishrock123 added a commit that referenced this pull request Oct 21, 2015

repl: limit persistent history correctly on load

4c80c02

Previously the wrong end of the history was limited on load. PR-URL: #2356 Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed By: Evan Lucas <evanlucas@me.com>

rvagg mentioned this pull request Oct 21, 2015

Release proposal: v5.0.0 #3466

Merged

Fishrock123 added a commit that referenced this pull request Oct 26, 2015

repl: limit persistent history correctly on load

594e8e7

Previously the wrong end of the history was limited on load. PR-URL: #2356 Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed By: Evan Lucas <evanlucas@me.com>

jasnell mentioned this pull request Oct 26, 2015

test: repl-persistent-history is no longer flaky #3437

Merged

rvagg mentioned this pull request Oct 28, 2015

Preparing v4.2.2 LTS Maintenance nodejs/Release#50

Closed

Fishrock123 added a commit that referenced this pull request Oct 29, 2015

repl: limit persistent history correctly on load

a04408a

Previously the wrong end of the history was limited on load. PR-URL: #2356 Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed By: Evan Lucas <evanlucas@me.com>

jasnell mentioned this pull request Oct 29, 2015

Release Proposal: v4.2.2 "Argon" (LTS) #3588

Closed

jasnell removed the land-on-v4.x label Nov 3, 2015

elvisbegovic mentioned this pull request Oct 17, 2018

EPERM: operation not permitted GoogleChromeLabs/ndb#131

Closed

repl: fix/improve persistent repl history by Fishrock123 · Pull Request #2356 · nodejs/node · GitHub

Uh oh!

Uh oh!

repl: fix/improve persistent repl history - #2356

repl: fix/improve persistent repl history#2356Fishrock123 merged 2 commits intonodejs:masternodejs/node:masterfrom Fishrock123:improve-repl-historyFishrock123/node:improve-repl-historyCopy head branch name to clipboard

Conversation

Fishrock123 commented Aug 12, 2015

Uh oh!

evanlucas commented Aug 12, 2015

Uh oh!

evanlucas commented Aug 14, 2015

Uh oh!

Fishrock123 commented Aug 14, 2015

Uh oh!

evanlucas commented Aug 14, 2015

Uh oh!

Fishrock123 commented Aug 14, 2015

Uh oh!

Fishrock123 commented Aug 14, 2015

Uh oh!

Fishrock123 commented Aug 14, 2015

Uh oh!

Fishrock123 commented Aug 14, 2015

Uh oh!

evanlucas commented Aug 17, 2015

Uh oh!

Fishrock123 commented Aug 17, 2015

Uh oh!

silverwind commented Aug 17, 2015

Uh oh!

evanlucas commented Aug 26, 2015

Uh oh!

Fishrock123 commented Aug 26, 2015

Uh oh!

silverwind commented Aug 26, 2015

Uh oh!

Fishrock123 commented Aug 27, 2015

Uh oh!

Fishrock123 commented Aug 27, 2015

Uh oh!

Fishrock123 commented Aug 27, 2015

Uh oh!

silverwind commented Aug 27, 2015

Uh oh!

Fishrock123 commented Aug 27, 2015

Uh oh!

Fishrock123 commented Aug 27, 2015

Uh oh!

Fishrock123 commented Oct 8, 2015

Uh oh!

Fishrock123 Oct 19, 2015

Choose a reason for hiding this comment

Uh oh!

evanlucas commented Oct 19, 2015

Uh oh!

Fishrock123 commented Oct 19, 2015

Uh oh!

jasnell commented Oct 26, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

repl: fix/improve persistent repl history#2356
Fishrock123 merged 2 commits into
nodejs:masterfrom
Fishrock123:improve-repl-history