Skip to content

test: Improved tests in test-os#8606

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

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

Conversation

@delvedor
Copy link
Member

@delvedor delvedor commented Sep 17, 2016 •
edited
Loading

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-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Sep 17, 2016
@mcollina
Copy link
Member

mcollina commented Sep 17, 2016

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

@mcollina
Copy link
Member

mcollina commented Sep 17, 2016

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

@delvedor
Copy link
Member Author

delvedor commented Sep 17, 2016

Done!

@delvedor delvedor changed the title test: Moved from var to const, equal to strictEqual and added typeof tests test: Improved tests in test-os Sep 17, 2016
@mcollina
Copy link
Member

mcollina commented Sep 17, 2016

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

cc @addaleax @jasnell

@mscdex mscdex added the os Issues and PRs related to the os subsystem. label Sep 17, 2016
@addaleax
Copy link
Member

addaleax commented Sep 17, 2016

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
test/parallel/test-os.js Outdated
Copy link
Contributor

@cjihrig cjihrig Sep 17, 2016

Choose a reason for hiding this comment

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

Could you remove this.

test/parallel/test-os.js Outdated
Copy link
Contributor

@cjihrig cjihrig Sep 17, 2016

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');.

test/parallel/test-os.js Outdated
Copy link
Contributor

@cjihrig cjihrig Sep 17, 2016

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.

Copy link
Member Author

@delvedor delvedor Sep 17, 2016

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
Copy link
Contributor

@Fishrock123 Fishrock123 left a comment

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
Copy link
Member

jbergstroem commented Sep 17, 2016

Cleaning up the host now.

cjihrig
cjihrig approved these changes Sep 17, 2016
View reviewed changes
Copy link
Contributor

@cjihrig cjihrig left a comment

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
Copy link
Member

@mcollina mcollina left a comment

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.
@imyller imyller self-assigned this Sep 20, 2016
@imyller
Copy link
Member

imyller commented Sep 20, 2016

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

@imyller
Copy link
Member

imyller commented Sep 20, 2016

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

targos
targos approved these changes Sep 20, 2016
View reviewed changes
Copy link
Member

@targos targos left a comment

Choose a reason for hiding this comment

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

LGTM

@imyller
Copy link
Member

imyller commented Sep 21, 2016

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
Copy link
Member

jasnell commented Sep 21, 2016

Go for it!

@imyller
Copy link
Member

imyller commented Sep 21, 2016 •
edited
Loading

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
Copy link
Member

imyller commented Sep 21, 2016

landed in 66df5d1

Thank you for your contribution, @delvedor

@imyller imyller closed this Sep 21, 2016
@imyller imyller 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 free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollina mcollina mcollina approved these changes

@jasnell jasnell jasnell approved these changes

@targos targos targos approved these changes

@cjihrig cjihrig cjihrig approved these changes

+1 more reviewer

@Fishrock123 Fishrock123 Fishrock123 requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

os Issues and PRs related to the os 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.