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: make sure O_NOATIME is present only in Linux #6614

Closed
wants to merge 2 commits into from

Conversation

thefourtheye
Copy link
Contributor

@thefourtheye thefourtheye commented May 6, 2016

Checklist
  • tests and code linting passes
  • a test and/or benchmark is included
  • the commit message follows commit guidelines
Affected core subsystem(s)

test

Description of change

As it is, the test checks if the return value is undefined in other
platforms. But it should also make sure that the O_NOATIME should be
found only in Linux.

Ref: #6492

cc @Trott @bnoordhuis @jasnell

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

As it is, the test checks if the return value is `undefined` in other
platforms. But it should also make sure that the `O_NOATIME` should be
found only in Linux.
@thefourtheye thefourtheye added the test Issues and PRs related to the tests. label May 6, 2016
@mscdex mscdex added the process Issues and PRs related to the process subsystem. label May 6, 2016
assert(constants.hasOwnProperty('O_NOATIME'));
assert.strictEqual(constants.O_NOATIME, 0x40000);
} else {
assert(false === constants.hasOwnProperty('O_NOATIME'));
Copy link
Member

Choose a reason for hiding this comment

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

If you use !('O_NOATIME' in constants), it will check the prototype as well (for the very unlikely case that we decide to move things to the prototype in the future.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@bnoordhuis
Copy link
Member

LGTM with a suggestion.

@bnoordhuis
Copy link
Member

LGTM

@cjihrig
Copy link
Contributor

cjihrig commented May 6, 2016

LGTM. CI is green.

@addaleax
Copy link
Member

LGTM

@thefourtheye
Copy link
Contributor Author

Landed in 4803d11.

@thefourtheye thefourtheye deleted the improve-noatime-test branch May 12, 2016 14:55
thefourtheye added a commit that referenced this pull request May 12, 2016
As it is, the test checks if the return value is `undefined` in other
platforms. But it should also make sure that the `O_NOATIME` should be
found only in Linux.

PR-URL: #6614
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
evanlucas pushed a commit that referenced this pull request May 17, 2016
As it is, the test checks if the return value is `undefined` in other
platforms. But it should also make sure that the `O_NOATIME` should be
found only in Linux.

PR-URL: #6614
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@MylesBorins
Copy link
Contributor

@thefourtheye lts?

@addaleax
Copy link
Member

addaleax commented Jun 2, 2016

@thealphanerd This depends on #6492 which is a semver-minor one, so probably no for now.

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

Successfully merging this pull request may close these issues.

6 participants