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.

Sign up for GitHub

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

Jump to bottom

test: removes FIXME parallel/test-repl-persistent-history #8756

Closed
wants to merge 1 commit into from
Closed

test: removes FIXME parallel/test-repl-persistent-history #8756

wants to merge 1 commit into from

Conversation

ALJCepeda
Copy link
Contributor

Checklist
  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)
Description of change

When NODE_REPL_HISTORY isn't defined repl defaults to temporary file
This prevents the temporary file from being cleared and removes check on fixture
Refs: #4640

/cc @Fishrock123

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Sep 24, 2016
@ALJCepeda
test: removes FIXME parallel/test-repl-persistent-history
66c3fe1 
When NODE_REPL_HISTORY isn't defined `repl` defaults to temporary file
This prevents the temporary file from being cleared and removes check on fixture
Refs: nodejs#4640
@ALJCepeda ALJCepeda changed the title t: removes FIXME parallel/test-repl-persistent-history test: removes FIXME parallel/test-repl-persistent-history Sep 24, 2016
@mscdex mscdex added the repl Issues and PRs related to the REPL subsystem. label Sep 24, 2016
@jasnell
Copy link
Member

jasnell commented Sep 26, 2016

@Trott @Fishrock123

@evanlucas
Copy link
Contributor

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

@Fishrock123
Copy link
Contributor

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

Windows error-d on the build, pretty strange. Changes LGTM, awaiting a clean CI.

@ALJCepeda
Copy link
Contributor Author

ALJCepeda commented Sep 26, 2016 •
edited
Loading

It seems that most of the builds are failing https://ci.nodejs.org/job/node-test-pull-request/ maybe infrastructure issues still?

@imyller
Copy link
Member

imyller commented Sep 26, 2016

CI fail on Windows seems unrelated:

not ok 296 inspector/test-inspector
# Error: read ECONNRESET
# at exports._errnoException (util.js:1026:11)
# at TCP.onread (net.js:572:26)
# [err] Debugger listening on port 9229.
# [err] Warning: This is an experimental feature and could change at any time.
# [err] To start debugging, open the following URL in Chrome:
# [err]     chrome-devtools://devtools/remote/serve_file/@60cd6e859b9f557d2312f5bf532f6aec5f284980/inspector.html?experiments=true&v8only=true&ws=localhost:9229/node
# [err] 
# [test] Verifying debugger stops on start (--debug-brk option)
# [test] Setting a breakpoint and verifying it is hit
# [out] A message 5
# [out] 
# [test] Verify we can read current application state
# [test] Verify node waits for the frontend to disconnect
# [out] Outputed message #1
# [out] 
# [err] Debugger attached.
# [err] Waiting for the debugger to disconnect...
# [err] 
# [test] Connection terminated

imyller
imyller approved these changes Sep 26, 2016
View reviewed changes
Copy link
Member

@imyller imyller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Fishrock123
Copy link
Contributor

LGTM -- Landing as is with amendments to the commit message.

@ALJCepeda in the future could you please make sure the first commit message line is not more than 50 characters long, and the following lines are no longer than 72? Thanks!

@Fishrock123
Copy link
Contributor

Landed in 8e81e91 -- thanks for taking care of this! 🎉

Fishrock123 pushed a commit that referenced this pull request Sep 27, 2016
@ALJCepeda @Fishrock123
test: fixed FIXME in test-repl-persistent-history
8e81e91 
When NODE_REPL_HISTORY isn't defined `repl` defaults to temporary file
This prevents the temporary file from being cleared and removes check
on fixture

Refs: #4640
PR-URL: #8756
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
@Fishrock123
Copy link
Contributor

(Oh one other thing: please use full URLs to refer to issues. I caught that for ya here.)

@ALJCepeda
Copy link
Contributor Author

@Fishrock123 Copy on the commit message but I don't understand what you mean by using full URLS for issues?

@ALJCepeda ALJCepeda deleted the 4640-8 branch September 27, 2016 14:30
@Fishrock123
Copy link
Contributor

e.g. https://github.com/nodejs/node/pull/8756 vs #8756

jasnell pushed a commit that referenced this pull request Sep 29, 2016
@ALJCepeda @jasnell
test: fixed FIXME in test-repl-persistent-history
b796f7f 
When NODE_REPL_HISTORY isn't defined `repl` defaults to temporary file
This prevents the temporary file from being cleared and removes check
on fixture

Refs: #4640
PR-URL: #8756
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
@MylesBorins
Copy link
Contributor

does this make sense to backport?

@Fishrock123
Copy link
Contributor

i mean if you want to otherwise why bother

Fishrock123 pushed a commit that referenced this pull request Oct 11, 2016
@ALJCepeda @Fishrock123
test: fixed FIXME in test-repl-persistent-history
2e95b0e 
When NODE_REPL_HISTORY isn't defined `repl` defaults to temporary file
This prevents the temporary file from being cleared and removes check
on fixture

Refs: #4640
PR-URL: #8756
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@imyller imyller imyller approved these changes

Assignees
No one assigned
Labels
repl Issues and PRs related to the REPL subsystem. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@ALJCepeda @jasnell @evanlucas @Fishrock123 @imyller @MylesBorins @mscdex @nodejs-github-bot