Skip to content

test: Improved tests in test-os - #8606

Closed
delvedor wants to merge 1 commit into
nodejs:masterfrom
delvedor:test-os
Closed

test: Improved tests in test-os#8606
delvedor wants to merge 1 commit into
nodejs:masterfrom
delvedor:test-os

Conversation

@delvedor

@delvedordelvedor commented Sep 17, 2016•
edited
Loading

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

test

Affected test

test-os

Description of change
  • Moved all variables declaration from var to const
  • Moved from .equal to .strictEqual
  • Added more checks about the type of the returned values.

Part of code & learn.
Cc: @mcollina

@nodejs-github-botnodejs-github-bot added the test Issues and PRs related to the tests. label Sep 17, 2016
@mcollina

mcollina commented Sep 17, 2016

Copy link
Copy Markdown
Member

Can you please shorten the commit message and include the name of the test you are changing?

@mcollina

mcollina commented Sep 17, 2016

Copy link
Copy Markdown
Member

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

@delvedor

delvedor commented Sep 17, 2016

Copy link
Copy Markdown
MemberAuthor

Done!

@delvedordelvedor changed the title test: Moved from var to const, equal to strictEqual and added typeof teststest: Improved tests in test-osSep 17, 2016
@mcollina

mcollina commented Sep 17, 2016

Copy link
Copy Markdown
Member

https://ci.nodejs.org/job/node-test-commit-arm/5000/nodes=ubuntu1604-arm64/ failed. Seems highly unrelated.

cc @addaleax@jasnell

@mscdexmscdex added the os Issues and PRs related to the os subsystem. label Sep 17, 2016
@addaleax

addaleax commented Sep 17, 2016

Copy link
Copy Markdown
Member

That failure looks more like a build problem that might be fixed by just cleaning up on the build machine? /cc @nodejs/build

cjihrig
cjihrig reviewed Sep 17, 2016
View reviewed changes
Comment threadtest/parallel/test-os.js Outdated

@cjihrigcjihrigSep 17, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could you remove this.

Comment threadtest/parallel/test-os.js Outdated

@cjihrigcjihrigSep 17, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

These could be, for example, assert.strictEqual(typeof value, 'string');.

Comment threadtest/parallel/test-os.js Outdated

@cjihrigcjihrigSep 17, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This should also check for null.

@delvedordelvedorSep 17, 2016

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Sure, originally I wrote the not null test here, but since typeof null === 'object' I thought the is.object with this check was a nonsense.
Anyhow, now I added it again, let me know if something else is missing :)

Fishrock123
Fishrock123 suggested changes Sep 17, 2016
View reviewed changes

@Fishrock123Fishrock123 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

See @cjihrig's comments

@jbergstroem

jbergstroem commented Sep 17, 2016

Copy link
Copy Markdown
Member

Cleaning up the host now.

cjihrig
cjihrig approved these changes Sep 17, 2016
View reviewed changes

@cjihrigcjihrig left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

mcollina
mcollina approved these changes Sep 18, 2016
View reviewed changes

@mcollinamcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, can you please squash the commits?

@delvedor
test: Improved tests in test-os
5e5dbf2
Moved from var to const.
Moved from .equal to .strictEqual.
Added more checks about the type of the returned values.
@imyllerimyller self-assigned this Sep 20, 2016
@imyller

imyller commented Sep 20, 2016

Copy link
Copy Markdown
Member

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

@imyller

imyller commented Sep 20, 2016

Copy link
Copy Markdown
Member

@Fishrock123 Could you revise your review. I think the issues mentioned have been addressed.

jasnell
jasnell approved these changes Sep 20, 2016
View reviewed changes

@jasnelljasnell left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

targos
targos approved these changes Sep 20, 2016
View reviewed changes

@targostargos left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@imyller

imyller commented Sep 21, 2016

Copy link
Copy Markdown
Member

Any objections to start landing this?

I see that there are four LGTMs for Reviewed-By and the requested code changes have been made.

@jasnell

jasnell commented Sep 21, 2016

Copy link
Copy Markdown
Member

Go for it!

@imyller

imyller commented Sep 21, 2016•
edited
Loading

Copy link
Copy Markdown
Member

I'll start landing this:

  • Five LGTMs
  • No objections
  • Requested code changes have been completed
  • CI tests passed (only usual CI failures)

imyller pushed a commit to imyller/node that referenced this pull request Sep 21, 2016
@delvedor@imyller
test: cleanup test-os.js
66df5d1
Moved from var to const.
Moved from .equal to .strictEqual.
Added more checks about the type of the returned values.
PR-URL: nodejs#8606
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
@imyller

imyller commented Sep 21, 2016

Copy link
Copy Markdown
Member

landed in 66df5d1

Thank you for your contribution, @delvedor

@imyllerimyller closed this Sep 21, 2016
@imyllerimyller removed their assignment Sep 21, 2016
@delvedor
delvedor deleted the test-os branch September 21, 2016 08:23
Fishrock123 pushed a commit that referenced this pull request Oct 11, 2016
@delvedor@Fishrock123
test: cleanup test-os.js
edf9242
Moved from var to const.
Moved from .equal to .strictEqual.
Added more checks about the type of the returned values.
PR-URL: #8606
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollinamcollinamcollina approved these changes
@jasnelljasnelljasnell approved these changes
@targostargostargos approved these changes
@cjihrigcjihrigcjihrig approved these changes
+1 more reviewer
@Fishrock123Fishrock123Fishrock123 requested changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

osIssues and PRs related to the os subsystem.testIssues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

12 participants

@delvedor@mcollina@addaleax@jbergstroem@imyller@jasnell@Fishrock123@targos@cjihrig@mscdex@MylesBorins@nodejs-github-bot