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.

Sign up for GitHub

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

Jump to bottom

add ut and refactor for pkg/scheduler/plugins/util/k8s package #3412

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

add ut and refactor for pkg/scheduler/plugins/util/k8s package #3412

wants to merge 2 commits into from

Conversation

googs1025
Copy link
Contributor

@googs1025 googs1025 commented Apr 14, 2024 •
edited

What type of PR is this?

unit test

What this PR does / why we need it:

  • refactor nodelock
  • add some unit test for pkg/scheduler/plugins/util/k8s/snapshot.go
  • fix IsPVCUsedByPods method

Which issue(s) this PR fixes:

See more detail #3053 (comment)

Special notes for your reviewer:

Does this PR introduce a user-facing change?

None

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

None

@volcano-sh-bot volcano-sh-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 14, 2024
@googs1025 googs1025 force-pushed the snapshot_ut branch 2 times, most recently from fd6cd8f to 671b661 Compare April 14, 2024 13:36
@googs1025
Copy link
Contributor Author

@lowang-bh /PTAL thank a lot!

@googs1025
add ut for pkg/scheduler/plugins/util/k8s package
224c8d4 
Signed-off-by: googs1025 <googs1025@gmail.com>
lowang-bh
lowang-bh reviewed Apr 17, 2024
View reviewed changes
Copy link
Member

@lowang-bh lowang-bh left a comment

Choose a reason for hiding this comment

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

/lgtm

By the way, what's ut coverage percentage?

@volcano-sh-bot volcano-sh-bot added the lgtm Indicates that a PR is ready to be merged. label Apr 17, 2024
@googs1025
Copy link
Contributor Author

googs1025 commented Apr 17, 2024 •
edited

@lowang-bh
image
image
Additionally, I noticed some code duplication in this section. I intend to submit a refactoring pull request to improve it here. like those two
https://github.com/volcano-sh/volcano/blob/master/pkg/scheduler/plugins/util/nodelock/nodelock.go#L79
https://github.com/volcano-sh/volcano/blob/master/pkg/scheduler/plugins/util/nodelock/nodelock.go#L112

WDYT

@lowang-bh
Copy link
Member

Additionally, I noticed some code duplication in this section. I intend to submit a refactoring pull request to improve it here. like those two https://github.com/volcano-sh/volcano/blob/master/pkg/scheduler/plugins/util/nodelock/nodelock.go#L79 https://github.com/volcano-sh/volcano/blob/master/pkg/scheduler/plugins/util/nodelock/nodelock.go#L112

WDYT

Welcom！

@volcano-sh-bot volcano-sh-bot removed the lgtm Indicates that a PR is ready to be merged. label Apr 18, 2024
@volcano-sh-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign lowang-bh
You can assign the PR to them by writing /assign @lowang-bh in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@googs1025
Copy link
Contributor Author

In addition to adding unit tests, I also tried to refactor nodelock. Because it involves changes to the original logic, please reviewers to help me check it carefully.
/PTAL @lowang-bh @william-wang @shinytang6

@googs1025
Copy link
Contributor Author

/assign @william-wang @shinytang6
Thanks for the review

@googs1025 googs1025 changed the title add ut for pkg/scheduler/plugins/util/k8s package add ut and refactor for pkg/scheduler/plugins/util/k8s package Apr 20, 2024
@volcano-sh-bot volcano-sh-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 20, 2024
@volcano-sh-bot volcano-sh-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 20, 2024
@googs1025
Copy link
Contributor Author

done

@googs1025
Copy link
Contributor Author

/PTAL @william-wang @shinytang6 thanks a lot

hwdef
hwdef reviewed Apr 28, 2024
View reviewed changes
Copy link
Member

@hwdef hwdef left a comment

Choose a reason for hiding this comment

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

/lgtm

@volcano-sh-bot volcano-sh-bot added the lgtm Indicates that a PR is ready to be merged. label Apr 28, 2024
@googs1025 googs1025 requested a review from hwdef May 7, 2024 02:04
@volcano-sh-bot volcano-sh-bot removed the lgtm Indicates that a PR is ready to be merged. label May 7, 2024
@volcano-sh-bot
Copy link
Contributor

New changes are detected. LGTM label has been removed.

@googs1025 googs1025 requested a review from lowang-bh May 8, 2024 00:43
@googs1025 googs1025 requested a review from lowang-bh May 8, 2024 14:32
@googs1025
refecotr: nodelock
3187e1c 
Signed-off-by: googs1025 <googs1025@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shinytang6 shinytang6 Awaiting requested review from shinytang6

@william-wang william-wang Awaiting requested review from william-wang

@hwdef hwdef Awaiting requested review from hwdef

@lowang-bh lowang-bh Awaiting requested review from lowang-bh

At least 1 approving review is required to merge this pull request.

Assignees

@william-wang william-wang

@hwdef hwdef

@lowang-bh lowang-bh

@shinytang6 shinytang6

Labels
size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@googs1025 @lowang-bh @volcano-sh-bot @hwdef @william-wang @shinytang6