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: get rid of done in test-dns and use common.mustCall #10776

Closed
wants to merge 1 commit into from

Conversation

mauriyouth
Copy link

@mauriyouth mauriyouth commented Jan 12, 2017

Hi, this is my first pull request to node, i refactored aync test in test/internet/test-dns.js, to use
common.mustCall and get rid of done cb.
I also added

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

test

@nodejs-github-bot nodejs-github-bot added test Issues and PRs related to the tests. dont-land-on-v7.x labels Jan 12, 2017
@Trott
Copy link
Member

Trott commented Jan 12, 2017

@nodejs/testing

@mscdex mscdex added dns Issues and PRs related to the dns subsystem. and removed dont-land-on-v7.x labels Jan 12, 2017
@Trott
Copy link
Member

Trott commented Jan 12, 2017

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.

Lots of lint errors. (Are you sure you ran make test like the checkbox says?)

test/internet/test-dns.js
   55:1  error  Line 55 exceeds the maximum line length of 80   max-len
   75:1  error  Line 75 exceeds the maximum line length of 80   max-len
  117:1  error  Line 117 exceeds the maximum line length of 80  max-len
  129:1  error  Line 129 exceeds the maximum line length of 80  max-len
  145:1  error  Line 145 exceeds the maximum line length of 80  max-len
  156:1  error  Line 156 exceeds the maximum line length of 80  max-len
  178:1  error  Line 178 exceeds the maximum line length of 80  max-len
  189:1  error  Line 189 exceeds the maximum line length of 80  max-len
  205:1  error  Line 205 exceeds the maximum line length of 80  max-len
  217:1  error  Line 217 exceeds the maximum line length of 80  max-len
  240:1  error  Line 240 exceeds the maximum line length of 80  max-len
  252:1  error  Line 252 exceeds the maximum line length of 80  max-len
  284:1  error  Line 284 exceeds the maximum line length of 80  max-len
  296:1  error  Line 296 exceeds the maximum line length of 80  max-len
  312:1  error  Line 312 exceeds the maximum line length of 80  max-len
  325:1  error  Line 325 exceeds the maximum line length of 80  max-len
  337:1  error  Line 337 exceeds the maximum line length of 80  max-len
  350:1  error  Line 350 exceeds the maximum line length of 80  max-len
  378:1  error  Line 378 exceeds the maximum line length of 80  max-len
  392:1  error  Line 392 exceeds the maximum line length of 80  max-len
  405:1  error  Line 405 exceeds the maximum line length of 80  max-len
  426:1  error  Line 426 exceeds the maximum line length of 80  max-len
  485:1  error  More than 2 blank lines not allowed             no-multiple-empty-lines
✖ 23 problems (23 errors, 0 warnings)

@Trott
Copy link
Member

Trott commented Jan 12, 2017

(By the way, if you need to run the JS linter quickly without running all the tests: make jslint except on Windows where I think it's vcbuild nosign jslint instead.)

@Trott
Copy link
Member

Trott commented Jan 12, 2017

(Changes look good to me once lint issues are resolved.)

@mauriyouth
Copy link
Author

thanks, i'm fixing the lint stuff now, i just figured out i can see them with make lint

@Trott
Copy link
Member

Trott commented Jan 12, 2017

I think there may be another issue here. The test as originally written is designed to wait for one test to finish (that is, for the asynchronous callback to run) before running the next test. I think as written now, it may just be launching all the tests all at once.

@mauriyouth
Copy link
Author

@Trott fixed, the linting, but if i run the test one after the other, not in parallel, i don't see a way with doing it at the same time getting rid of the done cb, or i'm i missing something?

@Trott
Copy link
Member

Trott commented Jan 15, 2017

, i don't see a way with doing it at the same time getting rid of the done cb, or i'm i missing something?

That seems right. You probably need to keep the done() callback. (The current PR doesn't get rid of the done() function either.)

@jasnell
Copy link
Member

jasnell commented Mar 24, 2017

Updates on this one?

@jasnell jasnell added the stalled Issues and PRs that are stalled. label Mar 24, 2017
@fhinkel
Copy link
Member

fhinkel commented May 26, 2017

There hasn't been any activity here. I'm closing this. Feel free to reopen (or ping a collaborator) if I closed this in error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dns Issues and PRs related to the dns subsystem. stalled Issues and PRs that are stalled. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants