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: mock dns lookup function in parallel tests #17296

Closed
wants to merge 2 commits into from
Closed

test: mock dns lookup function in parallel tests #17296

wants to merge 2 commits into from

Conversation

joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Nov 25, 2017 •
edited
Loading

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

test

These tests should not make any DNS calls. The lookup would fail when the DNS requests are hijacked and time out instead of erroring out.

Refs: nodejs/help#687
Refs: #14781 (comment)

@joyeecheung
test: add common.dns.errorLookupMock
452c4b8
@joyeecheung
test: mock the lookup function in parallel tests
a5403ec 
These tests should not make any DNS calls. The lookup would fail
when the DNS requests are hijacked and time out instead of erroring
out.
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Nov 25, 2017
@joyeecheung
Copy link
Member Author

joyeecheung commented Nov 25, 2017 •
edited
Loading

These failures happened to me again when I was on the hotel wifi. We should just mock the requests.

cc @refack @Trott

@joyeecheung
Copy link
Member Author

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

@refack refack added the dns Issues and PRs related to the dns subsystem. label Nov 25, 2017
@refack
Copy link
Contributor

refack commented Nov 25, 2017

These tests should not make any DNS calls.

I totally agree with that.
The proposed implementation, I need to think about... (D.R.Y. is a non-goal in tests IMO)

@Trott
Copy link
Member

Trott commented Nov 25, 2017

The proposed implementation, I need to think about... (D.R.Y. is a non-goal in tests IMO)

Generally agree, but I'm 👍 on mocks/test doubles being D.R.Y.

Trott
Trott approved these changes Nov 25, 2017
View reviewed changes
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

LGTM. If it's easy/quick enough to do, it might be worth comparing a coverage run with and without these changes to see if this loses anything in terms of coverage of the lib/dns.js code.

@joyeecheung
Copy link
Member Author

@Trott I think if the coverage decreases we would need to write other tests for that (or it would be ideal if we can run the internet tests on the coverage machine & a testing CI job).

@joyeecheung
Copy link
Member Author

node-test-commit-linux-linked failed but didn't look related...trying a new one: https://ci.nodejs.org/job/node-test-commit-linux-linked/374/

@joyeecheung joyeecheung added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 26, 2017
joyeecheung added a commit that referenced this pull request Nov 28, 2017
@joyeecheung
test: add common.dns.errorLookupMock
59ed40a 
PR-URL: #17296
Refs: nodejs/help#687
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
joyeecheung added a commit that referenced this pull request Nov 28, 2017
@joyeecheung
test: mock the lookup function in parallel tests
0fb1e07 
These tests should not make any DNS calls. The lookup would fail
when the DNS requests are hijacked and time out instead of erroring
out.

PR-URL: #17296
Refs: nodejs/help#687
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@joyeecheung
Copy link
Member Author

Landed in 59ed40a and 0fb1e07, thanks!

@TimothyGu TimothyGu removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 29, 2017
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
@joyeecheung @MylesBorins
test: add common.dns.errorLookupMock
7144544 
PR-URL: #17296
Refs: nodejs/help#687
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
@joyeecheung @MylesBorins
test: mock the lookup function in parallel tests
28a96ab 
These tests should not make any DNS calls. The lookup would fail
when the DNS requests are hijacked and time out instead of erroring
out.

PR-URL: #17296
Refs: nodejs/help#687
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
@joyeecheung @MylesBorins
test: add common.dns.errorLookupMock
b7578bf 
PR-URL: #17296
Refs: nodejs/help#687
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
@joyeecheung @MylesBorins
test: mock the lookup function in parallel tests
e2ae99f 
These tests should not make any DNS calls. The lookup would fail
when the DNS requests are hijacked and time out instead of erroring
out.

PR-URL: #17296
Refs: nodejs/help#687
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
@joyeecheung @MylesBorins
test: add common.dns.errorLookupMock
4e789a3 
PR-URL: #17296
Refs: nodejs/help#687
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
@joyeecheung @MylesBorins
test: mock the lookup function in parallel tests
3d45a94 
These tests should not make any DNS calls. The lookup would fail
when the DNS requests are hijacked and time out instead of erroring
out.

PR-URL: #17296
Refs: nodejs/help#687
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Dec 12, 2017
Merged
@gibfahn
Copy link
Member

gibfahn commented Dec 19, 2017

Should this be backported to v6/v8.x-staging? If yes please follow the guide and raise a backport PR, if no let me know or add the dont-land-on label.

@gibfahn gibfahn mentioned this pull request Dec 20, 2017
Closed
2 tasks
@joyeecheung joyeecheung mentioned this pull request Mar 30, 2018
Closed
joyeecheung added a commit to joyeecheung/node that referenced this pull request Mar 30, 2018
@joyeecheung
test: add common.dns.errorLookupMock
fde95ff 
PR-URL: nodejs#17296
Refs: nodejs/help#687
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
joyeecheung added a commit to joyeecheung/node that referenced this pull request Mar 30, 2018
@joyeecheung
test: mock the lookup function in parallel tests
d989b20 
These tests should not make any DNS calls. The lookup would fail
when the DNS requests are hijacked and time out instead of erroring
out.

PR-URL: nodejs#17296
Refs: nodejs/help#687
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
gibfahn pushed a commit that referenced this pull request Apr 11, 2018
@joyeecheung @gibfahn
test: add common.dns.errorLookupMock
34af494 
PR-URL: #17296
Backport-PR-URL: #19706
Refs: nodejs/help#687
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
gibfahn pushed a commit that referenced this pull request Apr 11, 2018
@joyeecheung @gibfahn
test: mock the lookup function in parallel tests
da16227 
These tests should not make any DNS calls. The lookup would fail
when the DNS requests are hijacked and time out instead of erroring
out.

PR-URL: #17296
Backport-PR-URL: #19706
Refs: nodejs/help#687
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request May 2, 2018
Merged
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

@jasnell jasnell jasnell approved these changes

@refack refack refack approved these changes

Assignees
No one assigned
Labels
dns Issues and PRs related to the dns 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.
7 participants
@joyeecheung @refack @Trott @gibfahn @jasnell @TimothyGu @nodejs-github-bot