test: add os setPriority, getPriority test coverage #38771
Conversation
github-actions
bot
added
needs-ci
There was a problem hiding this comment.
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.
replace proiority with priority
There was a problem hiding this comment.
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>
aduh95
force-pushed
the
improve-os-tests
branch
from
56d8ca3
to
80c621a
Compare
github-actions
bot
removed
the
request-ci
There was a problem hiding this comment.
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
RaisinTen
added
commit-queue
nodejs-github-bot
removed
the
commit-queue
|
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.getPrioritytest coverage