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: add os setPriority, getPriority test coverage #38771

Merged
merged 3 commits into from
Sep 27, 2023

Conversation

waelsy123
Copy link
Contributor

@waelsy123 waelsy123 commented May 22, 2021

adding more os.setPriority & os.getPriority test coverage

@github-actions github-actions bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels May 22, 2021
Copy link
Contributor

@RaisinTen RaisinTen left a comment

Choose a reason for hiding this comment

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

@RaisinTen
Copy link
Contributor

The only line in the mentioned functions that has no coverage: https://coverage.nodejs.org/coverage-7afa7b9ab34b4f7c/lib/os.js.html#L322

const DUMMY_PRIORITY = 10
os.setPriority(DUMMY_PRIORITY)
const proiority = os.getPriority()
is.number(proiority)
Copy link
Member

Choose a reason for hiding this comment

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

replace proiority with priority

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks I missed that

@waelsy123
Copy link
Contributor Author

@RaisinTen indeed it has already test in another dedicated file. however I wanted to have at least one unit test within os-test

test/parallel/test-os.js Outdated Show resolved Hide resolved
@nodejs-github-bot
Copy link
Collaborator

@aduh95 aduh95 added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 20, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 20, 2023
@nodejs-github-bot
Copy link
Collaborator

Copy link
Contributor

@RaisinTen RaisinTen left a 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

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@RaisinTen RaisinTen added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Sep 27, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 27, 2023
@nodejs-github-bot nodejs-github-bot merged commit c19b2a7 into nodejs:main Sep 27, 2023
30 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in c19b2a7

@waelsy123
Copy link
Contributor Author

glad to get that merged thanks!

@waelsy123 waelsy123 deleted the improve-os-tests branch September 27, 2023 16:29
ruyadorno pushed a commit that referenced this pull request Sep 28, 2023
PR-URL: #38771
Reviewed-By: Darshan Sen <raisinten@gmail.com>
@ruyadorno ruyadorno mentioned this pull request Sep 28, 2023
ruyadorno pushed a commit that referenced this pull request Sep 28, 2023
PR-URL: #38771
Reviewed-By: Darshan Sen <raisinten@gmail.com>
@ruyadorno ruyadorno mentioned this pull request Sep 28, 2023
targos pushed a commit that referenced this pull request Oct 28, 2023
PR-URL: #38771
Reviewed-By: Darshan Sen <raisinten@gmail.com>
alexfernandez pushed a commit to alexfernandez/node that referenced this pull request Nov 1, 2023
PR-URL: nodejs#38771
Reviewed-By: Darshan Sen <raisinten@gmail.com>
debadree25 pushed a commit to debadree25/node that referenced this pull request Apr 15, 2024
PR-URL: nodejs#38771
Reviewed-By: Darshan Sen <raisinten@gmail.com>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
PR-URL: nodejs/node#38771
Reviewed-By: Darshan Sen <raisinten@gmail.com>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
PR-URL: nodejs/node#38771
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. needs-ci PRs that need a full CI run. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants