chia7712 commented on code in PR #915:
URL: https://github.com/apache/yunikorn-core/pull/915#discussion_r1677216548
##########
pkg/scheduler/objects/allocation_test.go:
##########
@@ -232,12 +232,25 @@ func TestNewAllocFromSI(t *testing.T) {
var nilAlloc *Allocation
alloc := NewAllocationFromSI(allocSI)
assert.Equal(t, alloc, nilAlloc, "placeholder allocation created
without a TaskGroupName")
- allocSI.TaskGroupName = "testgroup"
Review Comment:
Maybe we can simplify replace the string by another one. For example:
`allocSI.TaskGroupName = "TestNewAllocFromSI"`
##########
pkg/common/configs/configvalidator_test.go:
##########
@@ -2194,7 +2196,6 @@ func TestCheckQueuesStructure(t *testing.T) {
if tc.validateFunc != nil {
tc.validateFunc(t, tc.partition)
}
-
Review Comment:
please revert this unrelated chagne.
##########
pkg/scheduler/placement/placement_test.go:
##########
@@ -260,7 +260,8 @@ partitions:
// user rule existing queue, acl allowed
err = man.PlaceApplication(app)
queueName := app.GetQueuePath()
- if err != nil || queueName != "root.testparent.testchild" {
+ const TestQueueName = "root.testparent.testchild"
Review Comment:
Could you please rewrite it?
```go
assert.NilError(t, err)
assert.Equal(t, "root.testparent.testchild", queueName)
```
##########
pkg/scheduler/objects/allocation_ask_test.go:
##########
@@ -135,11 +136,18 @@ func TestPlaceHolder(t *testing.T) {
ask = NewAllocationAskFromSI(siAsk)
var nilAsk *AllocationAsk
assert.Equal(t, ask, nilAsk, "placeholder ask created without a
TaskGroupName")
- siAsk.TaskGroupName = "testgroup"
+
+ siAsk = &si.AllocationAsk{
Review Comment:
ditto: `siAsk.TaskGroupName = "TestPlaceHolder"`
##########
pkg/scheduler/placement/provided_rule_test.go:
##########
@@ -119,7 +119,8 @@ partitions:
// unqualified queue with parent rule that exists directly in hierarchy
appInfo = newApplication("app1", "default", "testchild", user, tags,
nil, "")
queue, err = pr.placeApplication(appInfo, queueFunc)
- if queue != "root.testparent.testchild" || err != nil {
Review Comment:
Could you please rewrite it by assert? For example:
```go
assert.NilError(t, err)
assert.Equal(t, "root.testparent.testchild", queue)
```
##########
pkg/common/configs/configvalidator_test.go:
##########
@@ -2007,8 +2007,10 @@ func TestCheckLimits(t *testing.T) { //nolint:funlen
}
for _, testCase := range testCases {
+ config := testCase.config
+
Review Comment:
please remove this line
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
