-
Notifications
You must be signed in to change notification settings - Fork 75
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.
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
Implement multi insert test case for pkg agent handler #1612
Implement multi insert test case for pkg agent handler #1612
Conversation
[CHATOPS:HELP] ChatOps commands.
|
Codecov Report
@@ Coverage Diff @@
## master #1612 +/- ##
==========================================
+ Coverage 30.71% 30.92% +0.20%
==========================================
Files 372 372
Lines 32502 32502
==========================================
+ Hits 9984 10052 +68
+ Misses 22144 22076 -68
Partials 374 374
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check your grammar before asking review?
success/fail
is better to set at the start of the test case for readable
b52126b
to
218b308
Compare
218b308
to
cdc3881
Compare
183aade
to
d698417
Compare
d698417
to
ad2516b
Compare
196527e
to
4e32995
Compare
genMultiInsertReq := func(t objectType, dist vector.Distribution, num int, dim int, cfg *payload.Insert_Config) *payload.Insert_MultiRequest { | ||
var vecs [][]float32 | ||
switch t { | ||
case Float: | ||
vecs = genF32Vec(dist, num, dim) | ||
case Uint8: | ||
vecs = genIntVec(dist, num, dim) | ||
} | ||
|
||
req := &payload.Insert_MultiRequest{ | ||
Requests: make([]*payload.Insert_Request, num), | ||
} | ||
for i, vec := range vecs { | ||
req.Requests[i] = &payload.Insert_Request{ | ||
Vector: &payload.Object_Vector{ | ||
Id: "uuid-" + strconv.Itoa(i+1), | ||
Vector: vec, | ||
}, | ||
Config: cfg, | ||
} | ||
} | ||
|
||
return req | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additionally, it should be a global function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why I define it in a function is because it is the function to generate MultiInsert request which can be only used in this function. Do you think I still need to define it as a global function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
We can use it for other tests, like MultiUpdate
, Upsert
for preparing conditions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed :)
name: "Boundary Value Testing case 1.1: Success to MultiInsert with 0 value vector (vector type is uint8)", | ||
args: args{ | ||
ctx: ctx, | ||
reqs: genSameVecMultiInsertReq(insertNum, []float32{0, 0, 0}, nil), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reqs: genSameVecMultiInsertReq(insertNum, []float32{0, 0, 0}, nil), | |
reqs: genSameVecMultiInsertReq(insertNum, [intVecDim]float32, nil), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be better to show the reason set 3 variables in a slice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we cannot define a slice like your suggestion.
I will refactor it to
reqs: genSameVecMultiInsertReq(insertNum, []float32{0, 0, 0}, nil), | |
reqs: genSameVecMultiInsertReq(insertNum, make([]float32, f32VecDim), nil), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or should I create a helper function to fix it?
genSameValueVec := func(size int, val float32) []float32 {
v := make([]float32, size)
for i := 0; i < size; i++ {
v[i] = val
}
return v
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this function gives us any good merit, it would be better.
Either is fine..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, I think we can also use the helper function for different value tests (like #1612 (comment)).
I will fix this comment by defining the helper function for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed :)
Signed-off-by: kevindiu <kevindiujp@gmail.com>
Signed-off-by: kevindiu <kevindiujp@gmail.com>
Signed-off-by: kevindiu <kevindiujp@gmail.com>
Signed-off-by: kevindiu <kevindiujp@gmail.com>
Co-authored-by: Kosuke Morimoto <ksk@vdaas.org>
* impl multiinsert Equivalence Class Testing Signed-off-by: kevindiu <kevindiujp@gmail.com>
* impl multiinsert boundary value test (part1) Signed-off-by: kevindiu <kevindiujp@gmail.com> * impl remaining tests Signed-off-by: kevindiu <kevindiujp@gmail.com> * change max dim Signed-off-by: kevindiu <kevindiujp@gmail.com> * change max dim to reference value Signed-off-by: kevindiu <kevindiujp@gmail.com> * change max dim to reference value Signed-off-by: kevindiu <kevindiujp@gmail.com> * fix comments Signed-off-by: kevindiu <kevindiujp@gmail.com> * rename help function name Signed-off-by: kevindiu <kevindiujp@gmail.com> * combine genXXXInsertReq function Signed-off-by: kevindiu <kevindiujp@gmail.com> * rename helper function name Signed-off-by: kevindiu <kevindiujp@gmail.com>
* impl multiinsert test part 3 Signed-off-by: kevindiu <kevindiujp@gmail.com> * fix comment Signed-off-by: kevindiu <kevindiujp@gmail.com> * fix comment Signed-off-by: kevindiu <kevindiujp@gmail.com>
* implement tests Signed-off-by: kevindiu <kevindiujp@gmail.com> * implement remaining tests Signed-off-by: kevindiu <kevindiujp@gmail.com> * Update pkg/agent/core/ngt/handler/grpc/handler_test.go Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * fix comment Signed-off-by: kevindiu <kevindiujp@gmail.com> * Update pkg/agent/core/ngt/handler/grpc/handler_test.go Co-authored-by: Kiichiro YUKAWA <kyukawa315@gmail.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Kiichiro YUKAWA <kyukawa315@gmail.com>
Co-authored-by: Kiichiro YUKAWA <kyukawa315@gmail.com>
Signed-off-by: kevindiu <kevindiujp@gmail.com>
Signed-off-by: kevindiu <kevindiujp@gmail.com>
Signed-off-by: kevindiu <kevindiujp@gmail.com>
Signed-off-by: kevindiu <kevindiujp@gmail.com>
Signed-off-by: kevindiu <kevindiujp@gmail.com>
Signed-off-by: kevindiu <kevindiujp@gmail.com>
e17cc98
to
1232db0
Compare
Signed-off-by: kevindiu kevindiujp@gmail.com
Description:
This PR implements MultiInsert test for pkg agent handler.
Part 1: #1630 merged
Part 2:#1645 merged
Part 3: #1646 merged
Part 4: #1651 merged
Related Issue:
How Has This Been Tested?:
Environment:
Types of changes:
Changes to Core Features:
Checklist: