-
Notifications
You must be signed in to change notification settings - Fork 29.9k
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
Comments
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... |
Weird, it seems fine on the CI. I can't get it to pass locally though on OS X 10.10.4 |
It seems to pass locally for me about every 1 in 15 times |
@evanlucas what's your OS X version? Also, is this on master? |
Yes. 10.10.4 |
I just ran the test 5 times on the same OS X and got no error |
very strange, I can hardly get it to pass |
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 |
Have a look at your permissions/ownership; perhaps you've previously run tests as another user? Does the sudo sandwich work? ( |
It will be this afternoon before I can test again on that machine. It works fine on my laptop. :/ |
It looks like it is a race condition. The history file does not appear to be completely written. The contents of that file:
The patch below seems to work reliably for me. It's unfortunate that it takes using
|
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) |
This test is already being investigated, but until a solution is found it should be marked flaky. Ref: nodejs#2319 Ref: nodejs#2356
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. |
#2356 Fixes this but I haven't been able to resolve windows |
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
- 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 <[email protected]> Reviewed By: Evan Lucas <[email protected]>
- 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 <[email protected]> Reviewed By: Evan Lucas <[email protected]>
- 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 <[email protected]> Reviewed By: Evan Lucas <[email protected]>
I haven't tested it on any other OS for now.
The text was updated successfully, but these errors were encountered: