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

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

Closed
evanlucas opened this issue Aug 7, 2015 · 15 comments
Closed

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

evanlucas opened this issue Aug 7, 2015 · 15 comments
Assignees
Labels
macos Issues and PRs related to the macOS platform / OSX. repl Issues and PRs related to the REPL subsystem. test Issues and PRs related to the tests.

Comments

@evanlucas
Copy link
Contributor

Path: sequential/test-repl-persistent-history
assert.js:89
  throw new assert.AssertionError({
  ^
AssertionError: '> \'=^.^=\'' === '> \'42\''
    at Writable.write [as _write] (/Users/evan/dev/code/forks/io.js/test/sequential/test-repl-persistent-history.js:173:16)
    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)

I haven't tested it on any other OS for now.

@mscdex mscdex added test Issues and PRs related to the tests. repl Issues and PRs related to the REPL subsystem. labels Aug 7, 2015
@brendanashworth brendanashworth added the macos Issues and PRs related to the macOS platform / OSX. label Aug 7, 2015
@Fishrock123
Copy link
Contributor

On the CI? If so, can you point me to a CI run?

I really need to update that test so it shows which set of tests actually failed the assertion...

@Fishrock123 Fishrock123 self-assigned this Aug 7, 2015
@evanlucas
Copy link
Contributor Author

Weird, it seems fine on the CI. I can't get it to pass locally though on OS X 10.10.4

@evanlucas
Copy link
Contributor Author

It seems to pass locally for me about every 1 in 15 times

@Fishrock123
Copy link
Contributor

@evanlucas what's your OS X version? Also, is this on master?

@evanlucas
Copy link
Contributor Author

Yes. 10.10.4

@targos
Copy link
Member

targos commented Aug 7, 2015

I just ran the test 5 times on the same OS X and got no error

@evanlucas
Copy link
Contributor Author

very strange, I can hardly get it to pass

@thefourtheye
Copy link
Contributor

Can you comment out that particular case and try?

@Fishrock123
Copy link
Contributor

Can you comment out that particular case and try?

Ok so, the (other) problem here is that figuring out which case is having problems isn't necessarily easy. (I guess I should log some more info in the tests...?) :/

@evanlucas I think the problem is that the test isn't writing to the history file correctly?

Can you run it just as ./iojs --expose-internals test/sequential/test-repl-persistent-history.js and then post what is in test/tmp/.node_repl_history?

@jbergstroem
Copy link
Member

Have a look at your permissions/ownership; perhaps you've previously run tests as another user? Does the sudo sandwich work? (sudo make test)

@evanlucas
Copy link
Contributor Author

It will be this afternoon before I can test again on that machine. It works fine on my laptop. :/

@evanlucas
Copy link
Contributor Author

It looks like it is a race condition. The history file does not appear to be completely written.

The contents of that file:

'=^.^='
'hello world'

The patch below seems to work reliably for me. It's unfortunate that it takes using setTimeout though.

diff --git a/test/sequential/test-repl-persistent-history.js b/test/sequential/test-repl-persistent-history.js
index 8d550f6..9298269 100644
--- a/test/sequential/test-repl-persistent-history.js
+++ b/test/sequential/test-repl-persistent-history.js
@@ -185,7 +185,7 @@ function runTest() {
     repl.on('close', function() {
       // Ensure everything that we expected was output
       assert.strictEqual(expected.length, 0);
-      setImmediate(runTest);
+      setTimeout(runTest, 5);
     });

     repl.inputStream.run(test);

@Fishrock123
Copy link
Contributor

The patch below seems to work reliably for me. It's unfortunate that it takes using setTimeout though.

Can you try uncommenting this: https://github.com/nodejs/node/blob/master/test/sequential/test-repl-persistent-history.js#L109-L113 (I forgot to remove the comment oops, fixed in #2358)

joaocgreis added a commit to JaneaSystems/node that referenced this issue Sep 2, 2015
This test is already being investigated, but until a solution is found
it should be marked flaky.

Ref: nodejs#2319
Ref: nodejs#2356
@joaocgreis
Copy link
Member

As it is in the current master, this test fails every time on my local Ubuntu 14.04 and I got it to fail after 336 runs on Windows 2012r2.

@Fishrock123
Copy link
Contributor

#2356 Fixes this but I haven't been able to resolve windows EPERM errors.

joaocgreis added a commit that referenced this issue Sep 2, 2015
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>
joaocgreis added a commit that referenced this issue Sep 3, 2015
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>
Fishrock123 pushed a commit to Fishrock123/node that referenced this issue Sep 3, 2015
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>
Fishrock123 added a commit that referenced this issue Oct 21, 2015
- 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>
Fishrock123 added a commit that referenced this issue Oct 26, 2015
- 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>
Fishrock123 added a commit that referenced this issue Oct 29, 2015
- 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
macos Issues and PRs related to the macOS platform / OSX. repl Issues and PRs related to the REPL subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

No branches or pull requests

8 participants