[YUNIKORN-2824] Refactor preemption_test.go #985

SophieTech88 · 2024-10-16T07:16:35Z

What is this PR for?

Off late, lot of new tests has been added into preemption_test.go for several use cases. There is a room of improvement to simplify the whole tests especially by avoiding duplicates.
This PR focus on TestTryPreemptionOnNode , TestTryPreemption_NodeWithCapacityLesserThanAsk, and TestTryPreemptionOnQueue, those 3 tests can be merged together into single one TestTryPreemptionCombined and handle each cases through t.Run({}) construct.

What type of PR is it?

Todos

- Task

What is the Jira issue?

https://issues.apache.org/jira/browse/YUNIKORN-2824

How should this be tested?

It should pass all unit tests.

Screenshots (if appropriate)

Questions:

- The licenses files need update.
- There is breaking changes for older versions.
- It needs documentation.

chenyulin0719

I guess this PR is still in draft. I will convert it.
Leaving a few quick reviews.

chenyulin0719 · 2024-10-16T07:44:58Z

pkg/scheduler/objects/preemption_test.go

+		nodes            []*Node
+		iterator         func() NodeIterator


We can remove the iterator because nodes can be used to generate an iterator.

Yeah. And I restructure the test function again , and keep the node iterator in the body, not as parameters. I hope it will be better. Thank you!

chenyulin0719 · 2024-10-16T08:03:17Z

pkg/scheduler/objects/preemption_test.go

+		rootMaxRes       map[string]string
+		parentMaxRes     map[string]string
+		parentGuarRes    map[string]string
+		childQ1MaxRes    map[string]string
+		childQ1GuarRes   map[string]string
+		childQ2MaxRes    map[string]string
+		childQ2GuarRes   map[string]string


I'm not in favor of using 7 paramters to represent the queue configuration, especially when there is only one difference in childQ2GuarRes in TestTryPreemption_NodeWithCapacityLesserThanAsk

Thank you so much. I agree that there are too many parameters here. I jus reduced to 2 parameters: rootMaxRes and childQ2GuarRes.

SophieTech88 · 2024-10-16T19:22:18Z

Here I find there are 4 test functions:

TestTryPreemption: 1 node
TestTryPreemptionOnNode : 2 nodes
TestTryPreemption_NodeWithCapacityLesserThanAsk : 2 nodes
TestTryPreemptionOnQueue : 2 nodes

As a draft here, I made a func TestTryPreemptionCombined to deal with 3 tests, TestTryPreemptionOnNode , TestTryPreemption_NodeWithCapacityLesserThanAsk and TestTryPreemptionOnQueue, all of them have 2 nodes, they have similar process, and the test passed.

But when I want to merge test TestTryPreemption ,which has only one node, it failed. assert.Assert(t, result != nil, "no result") // assertion failed: result is nil : no result. So my guess, it failed to create a new preemptor. Welcome to any suggestions.

SophieTech88 · 2024-10-18T14:47:17Z

So at this moment, I only create a new test named TestTryPreemptionCombined, which includes 3 test cases:

TestTryPreemptionOnNode : 2 nodes
TestTryPreemption_NodeWithCapacityLesserThanAsk : 2 nodes
TestTryPreemptionOnQueue : 2 nodes

I skip the TestTryPreemption right now. And I want to check if I am on the right direction?

craigcondit

To be honest, I don't really see this as an improvement. It makes the tests themselves hard to follow.

SophieTech88 · 2024-10-19T05:25:00Z

To be honest, I don't really see this as an improvement. It makes the tests themselves hard to follow.
Hi Craig,

Thank you so much for your feedback! I completely understand your concerns regarding the readability of the combined tests.

I think this boils down to finding the right balance between readability and avoiding code duplication. In the original code, the logic is certainly clearer with separated test cases, but there is a lot of duplicate code due to the similar steps involved in the preemption process. I believe this is why @manirajv06 raised the issue in the first place—hoping to simplify the tests and remove redundancy, while still preserving clarity. Please correct me if my understanding is off!

As I reviewed the three test cases, I noticed that while their goals differ, the preemption process itself is quite similar across all of them, which is where the duplicated code comes from. Here's a breakdown of the common steps:

Setting up the two nodes and queues:
- The parameters for rootMaxRes, childQ2GuarRes, and app2Res are unique to each test case to suit the specific scenario being tested.
- One idea I considered was passing all parameters (e.g., MaxRes and GuaranteedRes for rootQ, parentQ, childQ1, childQ2) in a more structured way to simplify things, but I felt that might lead to passing too many parameters.
Setting up app1 with its allocations: This is a common step across all tests.
Setting up app2: Another similar step across tests.
Setting up headRoom, preemptor, and triggering preemption: This is where the main difference lies between the tests:
- TestTryPreemptionOnNode: Preemption happens due to node resource constraints.
- TestTryPreemption_NodeWithCapacityLesserThanAsk: No preemption happens because the node's capacity is smaller than the request.
- TestTryPreemptionOnQueue: Preemption happens due to queue resource constraints, which introduces an extra check for allocs.
Validating preemption results based on the test case.

While I tried to reduce the duplicated code, I realize there’s still a challenge in balancing this with maintaining readability and clarity. The goal was to make the code more concise without sacrificing logic transparency.

I've updated the code with a new approach that attempts to address these trade-offs. I'm not sure if it fully aligns with your expectations, but I'd really appreciate your thoughts on it. If you have any specific suggestions or ideas, I'd be more than happy to incorporate them. I’m open to any feedback to further improve it!

SophieTech88 · 2024-10-19T05:32:26Z

I also had another thought. Since the logic is the key focus in these tests, and the only common parts across the three test cases are setting up app1 with its allocations and setting up app2, we could extract just those common steps into a helper function.

By creating a function that handles setting up app1 and app2, we can reuse it across the three test cases. This way, the tests remain separated, keeping the logic for each scenario clear and focused, while still reducing some of the duplicate code.

craigcondit · 2024-10-22T20:57:48Z

I think keeping the tests separate and extracting common logic into helper functions makes the most sense. It allows the tests to test one thing (I'm not generally a fan of combining everything into the case-based flows), but avoids repetition.

SophieTech88 · 2024-10-22T21:19:16Z

I think keeping the tests separate and extracting common logic into helper functions makes the most sense. It allows the tests to test one thing (I'm not generally a fan of combining everything into the case-based flows), but avoids repetition.

Hi Craig. Thank you so much to review and comment, it is a great help for me to understand the next step. I love the idea of a helper function with separate tests. I will update the code, soon.

SophieTech88 · 2024-10-24T06:33:09Z

I try to do the work step by step as below :

return the code back to the original one (6d01a91)
Create 2 helper functions: createApp1() and createApp2() (641ad5a)
Update the createApp2() function, make it more flexible, and apply it to other test cases (c199547)
Update the createApp1() function, make it more flexible, and apply it to other test cases (5c25d60)

If anyone has any great suggestion, just let me know.

codecov · 2024-10-24T11:40:03Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.29%. Comparing base (44705ae) to head (5c25d60).
Report is 4 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #985      +/-   ##
==========================================
- Coverage   81.50%   81.29%   -0.21%     
==========================================
  Files          97       97              
  Lines       12625    15463    +2838     
==========================================
+ Hits        10290    12571    +2281     
- Misses       2052     2611     +559     
+ Partials      283      281       -2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

craigcondit

+1 LGTM.

SophieTech88 · 2024-10-24T20:53:42Z

+1 LGTM.

Thank you so much for your suggestion and quick response. ❤️

[YUNIKORN-2824]Refactor preemption_test.go(for debug purpose)

254ba10

manirajv06 assigned SophieTech88 Oct 16, 2024

manirajv06 requested review from manirajv06, craigcondit and pbacsko October 16, 2024 07:51

chenyulin0719 reviewed Oct 16, 2024

View reviewed changes

chenyulin0719 marked this pull request as draft October 16, 2024 08:16

SophieTech88 added 5 commits October 16, 2024 07:30

Update iterator()

21f79c3

test for TestTryPreemptionMerge2

07cf77d

update the TestTryPreemptionMerge func

938f4bd

Update the comments

9d434dc

Merge TestTryPreemptionOnQueue to the merge test, too

0d575da

Update the code for TestTryPreemptionCombined test

cf0b7e9

SophieTech88 marked this pull request as ready for review October 18, 2024 05:21

Update the format

3557486

craigcondit requested changes Oct 18, 2024

View reviewed changes

Update the code in a more readable way

7d93237

SophieTech88 added 4 commits October 23, 2024 00:12

Add 3 test case back

6d01a91

Create helper function to reduce the duplicate codes

641ad5a

Apply createApp2 helper function to other tests

c199547

Update createApp1 fucntion, and apply it to all other related tests

5c25d60

craigcondit approved these changes Oct 24, 2024

View reviewed changes

craigcondit changed the title ~~[YUNIKORN-2824]Refactor preemption_test.go(for debug purpose)~~ [YUNIKORN-2824] Refactor preemption_test.go Oct 24, 2024

craigcondit closed this in 597983c Oct 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[YUNIKORN-2824] Refactor preemption_test.go #985

[YUNIKORN-2824] Refactor preemption_test.go #985

SophieTech88 commented Oct 16, 2024 •

edited

Loading

chenyulin0719 left a comment

chenyulin0719 Oct 16, 2024 •

edited

Loading

SophieTech88 Oct 16, 2024

chenyulin0719 Oct 16, 2024

SophieTech88 Oct 16, 2024

SophieTech88 commented Oct 16, 2024 •

edited

Loading

SophieTech88 commented Oct 18, 2024

craigcondit left a comment

SophieTech88 commented Oct 19, 2024

SophieTech88 commented Oct 19, 2024

craigcondit commented Oct 22, 2024

SophieTech88 commented Oct 22, 2024

SophieTech88 commented Oct 24, 2024 •

edited

Loading

codecov bot commented Oct 24, 2024 •

edited

Loading

craigcondit left a comment

SophieTech88 commented Oct 24, 2024

[YUNIKORN-2824] Refactor preemption_test.go #985

[YUNIKORN-2824] Refactor preemption_test.go #985

Conversation

SophieTech88 commented Oct 16, 2024 • edited Loading

What is this PR for?

What type of PR is it?

Todos

What is the Jira issue?

How should this be tested?

Screenshots (if appropriate)

Questions:

chenyulin0719 left a comment

Choose a reason for hiding this comment

chenyulin0719 Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

SophieTech88 Oct 16, 2024

Choose a reason for hiding this comment

chenyulin0719 Oct 16, 2024

Choose a reason for hiding this comment

SophieTech88 Oct 16, 2024

Choose a reason for hiding this comment

SophieTech88 commented Oct 16, 2024 • edited Loading

SophieTech88 commented Oct 18, 2024

craigcondit left a comment

Choose a reason for hiding this comment

SophieTech88 commented Oct 19, 2024

SophieTech88 commented Oct 19, 2024

craigcondit commented Oct 22, 2024

SophieTech88 commented Oct 22, 2024

SophieTech88 commented Oct 24, 2024 • edited Loading

codecov bot commented Oct 24, 2024 • edited Loading

Codecov Report

craigcondit left a comment

Choose a reason for hiding this comment

SophieTech88 commented Oct 24, 2024

SophieTech88 commented Oct 16, 2024 •

edited

Loading

chenyulin0719 Oct 16, 2024 •

edited

Loading

SophieTech88 commented Oct 16, 2024 •

edited

Loading

SophieTech88 commented Oct 24, 2024 •

edited

Loading

codecov bot commented Oct 24, 2024 •

edited

Loading