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: move test that depends on dns query to internet #22516

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

joyeecheung
Copy link
Member

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

@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

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

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

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

@trivikr trivikr trivikr approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@richardlau richardlau richardlau approved these changes

@cjihrig cjihrig cjihrig approved these changes

@lpinca lpinca lpinca approved these changes

@Trott Trott Trott approved these changes

@refack refack refack approved these changes

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