Skip to content

Conversation

@santigimeno
Copy link
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

Description of change

Use the common.isWindows, common.isFreeBSD and common.isSunOS where possible. Add common.isOSX and common.isLinux.
Fix test-fs-read-file-sync-hostname as in its current form was not being run anywhere.

/cc @nodejs/testing

@nodejs-github-botnodejs-github-bot added the test Issues and PRs related to the tests. label Jul 22, 2016
Copy link
Contributor

Choose a reason for hiding this comment

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

This might be more readable as !(common.isFreeBSD || common.isOSX || common.isLinux)

@cjihrig
Copy link
Contributor

LGTM with a couple comments.

@santigimeno
Copy link
MemberAuthor

@santigimeno
Copy link
MemberAuthor

PR updated. Thanks

@cjihrig
Copy link
Contributor

CI came back all green. I wonder if the linux2 thing was essentially a no-op, and now a test is being skipped that doesn't need to be skipped.

@bnoordhuis
Copy link
Member

LGTM

I wonder if the linux2 thing was essentially a no-op, and now a test is being skipped that doesn't need to be skipped.

The way I read it, the test was simply always skipped on all platforms.

@jasnell
Copy link
Member

LGTM

@santigimeno
Copy link
MemberAuthor

The way I read it, the test was simply always skipped on all platforms.

@cjihrig, after Ben's explanation, does the PR LGTY?

@cjihrig
Copy link
Contributor

@santigimeno yes. I think we were saying the same thing... I just said it wrong.

@santigimeno
Copy link
MemberAuthor

Use the common.isWindows, common.isFreeBSD and common.isSunOS where possible. Add common.isOSX and common.isLinux. Fix `test-fs-read-file-sync-hostname` as in its current form was not being run anywhere. PR-URL: nodejs#7845 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>
@santigimeno
Copy link
MemberAuthor

All green except a known FreeBSD flaky test: #7650.
Squashing and merging.

@santigimenosantigimeno merged commit dee0e3a into nodejs:masterJul 27, 2016
@santigimeno
Copy link
MemberAuthor

Landed in dee0e3a

@cjihrigcjihrig mentioned this pull request Aug 8, 2016
cjihrig pushed a commit that referenced this pull request Aug 10, 2016
Use the common.isWindows, common.isFreeBSD and common.isSunOS where possible. Add common.isOSX and common.isLinux. Fix `test-fs-read-file-sync-hostname` as in its current form was not being run anywhere. PR-URL: #7845 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>
@cjihrigcjihrig mentioned this pull request Aug 11, 2016
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

testIssues and PRs related to the tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@santigimeno@cjihrig@bnoordhuis@jasnell@MylesBorins@nodejs-github-bot