-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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: add os setPriority, getPriority test coverage #38771
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this already covered in https://github.com/nodejs/node/blob/HEAD/test/parallel/test-os-process-priority.js?
The only line in the mentioned functions that has no coverage: https://coverage.nodejs.org/coverage-7afa7b9ab34b4f7c/lib/os.js.html#L322 |
test/parallel/test-os.js
Outdated
const DUMMY_PRIORITY = 10 | ||
os.setPriority(DUMMY_PRIORITY) | ||
const proiority = os.getPriority() | ||
is.number(proiority) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
replace proiority
with priority
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks I missed that
@RaisinTen indeed it has already test in another dedicated file. however I wanted to have at least one unit test within os-test |
Co-authored-by: Darshan Sen <raisinten@gmail.com>
56d8ca3
to
80c621a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/nodejs/node/blob/HEAD/test/parallel/test-os-process-priority.js handles a number of special cases for the os.set/getPriority
APIs. If the newly added test is not a source of more flakiness, I'm okay with adding this. Otherwise, I think that the other test is more robust and it should be okay to keep that one and not add more tests that are repetitions of existing tests.
Edit: No related flakes, so this should be fine
Landed in c19b2a7 |
glad to get that merged thanks! |
PR-URL: #38771 Reviewed-By: Darshan Sen <raisinten@gmail.com>
PR-URL: #38771 Reviewed-By: Darshan Sen <raisinten@gmail.com>
PR-URL: #38771 Reviewed-By: Darshan Sen <raisinten@gmail.com>
PR-URL: nodejs#38771 Reviewed-By: Darshan Sen <raisinten@gmail.com>
PR-URL: nodejs#38771 Reviewed-By: Darshan Sen <raisinten@gmail.com>
PR-URL: nodejs/node#38771 Reviewed-By: Darshan Sen <raisinten@gmail.com>
PR-URL: nodejs/node#38771 Reviewed-By: Darshan Sen <raisinten@gmail.com>
adding more
os.setPriority
&os.getPriority
test coverage