FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

test: add os setPriority, getPriority test coverage by waelsy123 · Pull Request #38771 · nodejs/node · GitHub

/ node Public

test: add os setPriority, getPriority test coverage - #38771

Merged
nodejs-github-bot merged 3 commits into
nodejs:mainfrom
waelsy123:improve-os-tests
Sep 27, 2023
ConversationCommits (3)ChecksFiles changed
Merged

test: add os setPriority, getPriority test coverage#38771
nodejs-github-bot merged 3 commits into
nodejs:mainfrom
waelsy123:improve-os-tests

Conversation

waelsy123 commented May 22, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

adding more os.setPriority & os.getPriority test coverage

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
RaisinTen reviewed May 22, 2021
View reviewed changes

RaisinTen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Isn't this already covered in https://github.com/nodejs/node/blob/HEAD/test/parallel/test-os-process-priority.js?

RaisinTen commented May 22, 2021

Copy link
Copy Markdown
Member

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

jasnell reviewed May 23, 2021
View reviewed changes
Comment thread test/parallel/test-os.js Outdated

jasnell May 23, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

replace proiority with priority

waelsy123 May 23, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

thanks I missed that

waelsy123 commented May 23, 2021

Copy link
Copy Markdown
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

RaisinTen reviewed May 24, 2021
View reviewed changes
Comment thread test/parallel/test-os.js Outdated

nodejs-github-bot commented May 25, 2021

Copy link
Copy Markdown
Collaborator

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

aduh95 requested a review from RaisinTen September 20, 2023 07:55
aduh95 added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 20, 2023
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 commented Sep 20, 2023

Copy link
Copy Markdown
Collaborator

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

RaisinTen approved these changes Sep 20, 2023
View reviewed changes

RaisinTen left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

github-actions Bot mentioned this pull request Sep 21, 2023
Open
22 tasks

nodejs-github-bot commented Sep 21, 2023

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Sep 21, 2023

Copy link
Copy Markdown
Collaborator

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

This was referenced Sep 22, 2023
Open
Open
Open

nodejs-github-bot commented Sep 26, 2023

Copy link
Copy Markdown
Collaborator

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

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 removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 27, 2023
nodejs-github-bot merged commit c19b2a7 into nodejs:main Sep 27, 2023

nodejs-github-bot commented Sep 27, 2023

Copy link
Copy Markdown
Collaborator

Landed in c19b2a7

waelsy123 commented Sep 27, 2023

Copy link
Copy Markdown
Contributor Author

glad to get that merged thanks!

waelsy123 deleted the improve-os-tests branch September 27, 2023 16:29
ruyadorno pushed a commit that referenced this pull request Sep 28, 2023
test: add os setPriority, getPriority test coverage
1a18034 
PR-URL: #38771
Reviewed-By: Darshan Sen <raisinten@gmail.com>
ruyadorno mentioned this pull request Sep 28, 2023
Closed
ruyadorno pushed a commit that referenced this pull request Sep 28, 2023
test: add os setPriority, getPriority test coverage
e49a573 
PR-URL: #38771
Reviewed-By: Darshan Sen <raisinten@gmail.com>
ruyadorno mentioned this pull request Sep 28, 2023
Merged
targos pushed a commit that referenced this pull request Oct 28, 2023
test: add os setPriority, getPriority test coverage
088460d 
PR-URL: #38771
Reviewed-By: Darshan Sen <raisinten@gmail.com>
targos mentioned this pull request Nov 28, 2023
Merged
debadree25 pushed a commit to debadree25/node that referenced this pull request Apr 15, 2024
test: add os setPriority, getPriority test coverage
da89771 
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
test: add os setPriority, getPriority test coverage
c7c9683 
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
test: add os setPriority, getPriority test coverage
20457ac 
PR-URL: nodejs/node#38771
Reviewed-By: Darshan Sen <raisinten@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell left review comments
RaisinTen RaisinTen approved these changes

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL