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: refactor assert in internet test-dns.js #8980

Closed
wants to merge 1 commit into from
Closed

test: refactor assert in internet test-dns.js #8980

wants to merge 1 commit into from

Conversation

jun-oka
Copy link
Contributor

@jun-oka jun-oka commented Oct 8, 2016

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

test internet test-dns

Description of change
  • assert.equal() -> assert.strictEqual()
  • assert.ok(typeof example === ‘type’) -> assert.strictEqual()

@jun-oka
test: refactor assert in internet test-dns.js
acd3f29 
change assert.equal() to assert.strictEqual()
and use assert.strictEqual() for type validation
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Oct 8, 2016
@mscdex mscdex added the dns Issues and PRs related to the dns subsystem. label Oct 8, 2016
lpinca
lpinca approved these changes Oct 8, 2016
View reviewed changes
Copy link
Member

@lpinca lpinca left a comment

Choose a reason for hiding this comment

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

Not sure about the typeof assertions that already use ===. I find assert.ok(typeof foo === 'bar'); easier to grok. Anyway LGTM.

jasnell
jasnell approved these changes Oct 8, 2016
View reviewed changes
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

LGTM

@jasnell
Copy link
Member

jasnell commented Oct 8, 2016

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

imyller
imyller approved these changes Oct 8, 2016
View reviewed changes
Copy link
Member

@imyller imyller left a comment

Choose a reason for hiding this comment

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

LGTM

@gibfahn
Copy link
Member

gibfahn commented Oct 8, 2016

The linter passed from the CI (we don't run the internet test suite in CI so the linter is the important test).

I also ran this test locally (macOS) and it passed.

@jun-oka
Copy link
Contributor Author

jun-oka commented Oct 9, 2016

Oh. We got error.

@lpinca OK! Thank you for that info.
Without undefined assertion, typeof assertions already used ===.
I just preferred to use strictEqual() but I will take care of how it os easy to grok for another files.

@gibfahn
Copy link
Member

gibfahn commented Oct 9, 2016

@jun-oka I actually find the strictEqual easier to read (I think your changes make it more readable), but of course it's just personal preference.

You don't need to worry about the errors as the test you changed isn't run in the CI. The linter passed though, which is good.

@jun-oka
Copy link
Contributor Author

jun-oka commented Oct 9, 2016

@gibfahn Yes! I am aware of CI :) Thank you for letting mo know!

jasnell pushed a commit that referenced this pull request Oct 10, 2016
@jun-oka @jasnell
test: refactor assert in internet test-dns.js
528c844 
change assert.equal() to assert.strictEqual()
and use assert.strictEqual() for type validation

PR-URL: #8980
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@jasnell
Copy link
Member

jasnell commented Oct 10, 2016

Landed in 528c844. Thank you!

@jasnell jasnell closed this Oct 10, 2016
jasnell pushed a commit that referenced this pull request Oct 10, 2016
@jun-oka @jasnell
test: refactor assert in internet test-dns.js
1efd3df 
change assert.equal() to assert.strictEqual()
and use assert.strictEqual() for type validation

PR-URL: #8980
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Fishrock123 pushed a commit that referenced this pull request Oct 11, 2016
@jun-oka @Fishrock123
test: refactor assert in internet test-dns.js
05f7412 
change assert.equal() to assert.strictEqual()
and use assert.strictEqual() for type validation

PR-URL: #8980
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@MylesBorins
Copy link
Member

This does not land cleanly in LTS. Added dont-land label. Please feel free to manually backport

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell approved these changes

@lpinca lpinca lpinca approved these changes

@imyller imyller imyller approved these changes

@cjihrig cjihrig cjihrig approved these changes

@gibfahn gibfahn gibfahn 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.

None yet
9 participants
@jun-oka @jasnell @gibfahn @MylesBorins @lpinca @imyller @cjihrig @mscdex @nodejs-github-bot