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

test: replace localhost with os.hostname in fs-readfilesync #23101

Closed

Conversation

lundibundi
Copy link
Member

Fixes: #21501

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes (waiting for CI)
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@targos could you please try this on the setup mentioned in the issue if possible?

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Sep 26, 2018
@lundibundi lundibundi added the needs-ci PRs that need a full CI run. label Sep 26, 2018
@Trott
Copy link
Member

Trott commented Sep 26, 2018

Can you confirm that the test still works with this change if the host is disconnected from the network?

@Trott
Copy link
Member

Trott commented Sep 26, 2018

(Actually, if you could confirm that it works in its current form on the master branch when disconnected from the network, that would be helpful too!)

@lundibundi
Copy link
Member Author

lundibundi commented Sep 29, 2018

@Trott, sorry for the delay. I have run this test with/without network successfully both on master and on this branch.

Edit: I ran them on Windows 10 (10.0.17134 Build 17134), just to clarify.

@lundibundi
Copy link
Member Author

@Trott
Copy link
Member

Trott commented Sep 29, 2018

This is a Windows-only test, I believe, so... @nodejs/platform-windows Can one or two folks review? @nodejs/testing too...

@addaleax addaleax added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed needs-ci PRs that need a full CI run. labels Sep 30, 2018
@addaleax
Copy link
Member

Copy link
Member

@jdalton jdalton left a comment

Choose a reason for hiding this comment

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

Neat issue and workaround.

@lundibundi
Copy link
Member Author

@danbev
Copy link
Contributor

danbev commented Oct 1, 2018

Landed in 1be804d.

@danbev danbev closed this Oct 1, 2018
danbev pushed a commit that referenced this pull request Oct 1, 2018
PR-URL: #23101
Fixes: #21501
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: John-David Dalton <[email protected]>
@lundibundi lundibundi deleted the fix-localhost-readfilesync branch October 1, 2018 09:20
targos pushed a commit that referenced this pull request Oct 1, 2018
PR-URL: #23101
Fixes: #21501
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: John-David Dalton <[email protected]>
targos pushed a commit that referenced this pull request Oct 3, 2018
PR-URL: #23101
Fixes: #21501
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: John-David Dalton <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

test-fs-readfilesync-enoent failing locally with unknown error
6 participants