volcano icon indicating copy to clipboard operation
volcano copied to clipboard

[good first issue]add relative unit testcase for merged PRs

Open lowang-bh opened this issue 1 year ago • 7 comments

What would you like to be added:

Add UT to cover the pod's condition message in allocate/preempt/reclaim/backfill action, according to relative issues and PRs as following.

  • [ ] allocate
    • Issue:
      • #3049,
      • #3050
    • PR: #3051
  • [ ] preempt
    • Issue
      • #3068
      • #3079
    • PR #3070
  • [ ] reclaim
  • [ ] backfill

Why is this needed:

To prevent future changes cause new issue and breaking current fix. The origin idea is from issue https://github.com/volcano-sh/volcano/issues/3049

lowang-bh avatar Aug 27 '23 05:08 lowang-bh

/good-first-issue

lowang-bh avatar Apr 12 '24 13:04 lowang-bh

@lowang-bh: This request has been marked as suitable for new contributors.

Please ensure the request meets the requirements listed here.

If this request no longer meets these requirements, the label can be removed by commenting with the /remove-good-first-issue command.

In response to this:

/good-first-issue

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

volcano-sh-bot avatar Apr 12 '24 13:04 volcano-sh-bot

Hello, I am hoping to start with this good first issue on Volcano. Could it be assigned to me?

MondayCha avatar Apr 16 '24 12:04 MondayCha

/assign @MondayCha

lowang-bh avatar Apr 16 '24 13:04 lowang-bh

@MondayCha Hi, do you still work on this? Could you assign to me? I'm ready to add some UTs to start at volcano.

JesseStutler avatar Sep 09 '24 12:09 JesseStutler

/assign @JesseStutler

lowang-bh avatar Sep 09 '24 13:09 lowang-bh

I think the current testing framework's scalability is a bit poor...Like I want to add UT in reclaim action, I want to build a pod with preemptionPolicy with pod, I need to add a function based on BuildPod, but now the parameter list of BuildPod is a bit too long: https://github.com/volcano-sh/volcano/blob/0843c0d33fccdb85439dc7086dd7cea061070901/pkg/scheduler/util/test_utils.go#L57

I don’t know what the parameters in which position are used for. On the contrary, the way of writing in the k8s repository is clearer: https://github.com/kubernetes/kubernetes/blob/master/pkg/controller/job/job_controller_test.go#L7791-L7827

It is better to expand the fields in this way. Now it is difficult for BuildPod to expand the fields, parameter list will be longer and longer...

JesseStutler avatar Sep 23 '24 12:09 JesseStutler