Skip to content

test: move test that depends on dns query to internet#22516

Closed
joyeecheung wants to merge 1 commit intonodejs:masterfrom
joyeecheung:mov-inet
Closed

test: move test that depends on dns query to internet#22516
joyeecheung wants to merge 1 commit intonodejs:masterfrom
joyeecheung:mov-inet

Conversation

@joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Aug 25, 2018

These test cases in test/parallel/test-dns-lookup.js send
dns requests and depend on the results, which could fail
if the DNS service for invalid hosts is hijacked by the ISP.

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

@joyeecheung
test: move test that depends on dns query to internet
0e8de28 
These test cases in `test/parallel/test-dns-lookup.js` send
dns requests and depend on the results, which could fail
if the DNS service for invalid hosts is hijacked by the ISP.
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Aug 25, 2018

@joyeecheung build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/668/pipeline

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Aug 25, 2018
@joyeecheung joyeecheung mentioned this pull request Aug 25, 2018
Closed
2 tasks
@joyeecheung
Copy link
Member Author

joyeecheung commented Aug 25, 2018

BTW: my DNS is hijacked. This test fails if I don't turn my VPN on.

@joyeecheung
Copy link
Member Author

joyeecheung commented Aug 25, 2018 •
edited
Loading

node-test-commit-custom-suites with internet as CI_JS_SUITES : https://ci.nodejs.org/job/node-test-commit-custom-suites/661/ (I hope I have started it correctly...)

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

@Trott
Copy link
Member

Trott commented Aug 25, 2018

Related: I wonder if make test-coverage should be modified to include the internet tests.

@richardlau
Copy link
Member

richardlau commented Aug 25, 2018

Related: I wonder if make test-coverage should be modified to include the internet tests.

Related discussion: nodejs/build#1470

refack
refack approved these changes Aug 25, 2018
View reviewed changes
Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

RSLGTM

@refack
Copy link
Contributor

refack commented Aug 25, 2018

node-test-commit-custom-suites with internet as CI_JS_SUITES: ci.nodejs.org/job/node-test-commit-custom-suites/661 (I hope I have started it correctly...)

Job was set up correctly and passed - https://ci.nodejs.org/job/node-test-commit-custom-suites/661/default/testReport/(root)/test/

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 31, 2018
@addaleax
Copy link
Member

addaleax commented Aug 31, 2018

Landed in a58b8dd

@addaleax addaleax closed this Aug 31, 2018
addaleax pushed a commit that referenced this pull request Aug 31, 2018
@joyeecheung @addaleax
test: move test that depends on dns query to internet
a58b8dd 
These test cases in `test/parallel/test-dns-lookup.js` send
dns requests and depend on the results, which could fail
if the DNS service for invalid hosts is hijacked by the ISP.

PR-URL: #22516
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
addaleax pushed a commit that referenced this pull request Aug 31, 2018
@joyeecheung @addaleax
test: move test that depends on dns query to internet
c44fa97 
These test cases in `test/parallel/test-dns-lookup.js` send
dns requests and depend on the results, which could fail
if the DNS service for invalid hosts is hijacked by the ISP.

PR-URL: #22516
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
targos pushed a commit that referenced this pull request Sep 3, 2018
@joyeecheung @targos
test: move test that depends on dns query to internet
6737201 
These test cases in `test/parallel/test-dns-lookup.js` send
dns requests and depend on the results, which could fail
if the DNS service for invalid hosts is hijacked by the ISP.

PR-URL: #22516
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@targos targos mentioned this pull request Sep 5, 2018
Merged
targos pushed a commit that referenced this pull request Sep 6, 2018
@joyeecheung @targos
test: move test that depends on dns query to internet
2d6ca80 
These test cases in `test/parallel/test-dns-lookup.js` send
dns requests and depend on the results, which could fail
if the DNS service for invalid hosts is hijacked by the ISP.

PR-URL: #22516
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
This was referenced Sep 6, 2018
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Trott Trott Trott approved these changes

@lpinca lpinca lpinca approved these changes

@cjihrig cjihrig cjihrig approved these changes

@richardlau richardlau richardlau approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@trivikr trivikr trivikr approved these changes

+1 more reviewer

@refack refack refack approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

@joyeecheung @nodejs-github-bot @Trott @richardlau @refack @addaleax @lpinca @cjihrig @BridgeAR @trivikr