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

doc, win: improve os.setPriority documentation #22817

Closed
wants to merge 1 commit into from

Conversation

bzoz
Copy link
Contributor

@bzoz bzoz commented Sep 12, 2018

Also fixes the test: #22799

See https://support.microsoft.com/en-us/help/110853/prb-can-t-increase-process-priority

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

@addaleax addaleax added doc Issues and PRs related to the documentations. windows Issues and PRs related to the Windows platform. os Issues and PRs related to the os subsystem. labels Sep 12, 2018
if (expected < PRIORITY_HIGH)
assert.strictEqual(priority, PRIORITY_HIGHEST);
assert.ok(priority === PRIORITY_HIGHEST || priority === PRIORITY_HIGH);
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of doing this for all cases, can we make an explicit check for Windows and possibly for elevated user status before making this kind of assertion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is for Windows only (see line 114)

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, can we check for elevated status then? Perhaps using this method or something similar (without 3rd party utilities)?

@danbev
Copy link
Contributor

danbev commented Sep 18, 2018

cjihrig pushed a commit to libuv/libuv that referenced this pull request Sep 18, 2018
Refs: nodejs/node#22817
PR-URL: #1985
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
@danbev
Copy link
Contributor

danbev commented Sep 19, 2018

Landed in 6e746f1.

@danbev danbev closed this Sep 19, 2018
danbev pushed a commit that referenced this pull request Sep 19, 2018
PR-URL: #22817
Fixes: #22799
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
targos pushed a commit that referenced this pull request Sep 19, 2018
PR-URL: #22817
Fixes: #22799
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
targos pushed a commit that referenced this pull request Sep 20, 2018
PR-URL: #22817
Fixes: #22799
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
cjihrig added a commit to cjihrig/libuv that referenced this pull request Sep 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. os Issues and PRs related to the os subsystem. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants