Implement multi insert test case for pkg agent handler #1612

kevindiu · 2022-04-07T08:27:57Z

Signed-off-by: kevindiu [email protected]

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:

Go Version: 1.18
Docker Version: 20.10.8
Kubernetes Version: 1.22.0
NGT Version: 1.14.3

Types of changes:

Bug fix [type/bug]
New feature [type/feature]
Add tests [type/test]
Security related changes [type/security]
Add documents [type/documentation]
Refactoring [type/refactoring]
Update dependencies [type/dependency]
Update benchmarks and performances [type/bench]
Update CI [type/ci]

Changes to Core Features:

Have you added an explanation of what your changes do and why you'd like us to include them?
Have you written new tests for your core changes, as applicable?
Have you successfully ran tests with your changes locally?

Checklist:

I have read the CONTRIBUTING document.
I have checked open Pull Requests for the similar feature or fixes?
I have added tests and benchmarks to cover my changes.
I have ensured all new and existing tests passed.
I have commented my code, particularly in hard-to-understand areas
I have updated the documentation accordingly.

vdaas-ci · 2022-04-07T08:28:27Z

[CHATOPS:HELP] ChatOps commands.

🙆‍♀️ /approve - approve
💌 /changelog - replace the PR body by changelog details
🍱 /format - format codes and add licenses
✅ /gen-test - generate test codes
🏷️ /label - add labels
⏪ /rebase - rebase master
🔚 2️⃣ 🔚 /label actions/e2e-deploy - run E2E deploy & integration test

codecov · 2022-04-07T08:46:44Z

Codecov Report

Merging #1612 (e17cc98) into master (e6d9fe5) will increase coverage by 0.20%.
The diff coverage is n/a.

❗ Current head e17cc98 differs from pull request most recent head 1232db0. Consider uploading reports for the commit 1232db0 to get more accurate results

@@            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

Impacted Files	Coverage Δ
internal/worker/queue.go	`100.00% <0.00%> (+1.26%)`	⬆️
pkg/agent/core/ngt/handler/grpc/handler.go	`16.99% <0.00%> (+3.11%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e6d9fe5...1232db0. Read the comment docs.

pkg/agent/core/ngt/handler/grpc/handler_test.go

vankichi

Please check your grammar before asking review?
success/fail is better to set at the start of the test case for readable

pkg/agent/core/ngt/handler/grpc/handler_test.go

vankichi · 2022-05-24T01:13:10Z

pkg/agent/core/ngt/handler/grpc/handler_test.go

+	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
+	}


Additionally, it should be a global function.

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?

Yes.
We can use it for other tests, like MultiUpdate, Upsert for preparing conditions.

vankichi · 2022-05-24T01:17:47Z

pkg/agent/core/ngt/handler/grpc/handler_test.go

+				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),


Suggested change

reqs: genSameVecMultiInsertReq(insertNum, []float32{0, 0, 0}, nil),

reqs: genSameVecMultiInsertReq(insertNum, [intVecDim]float32, nil),

It would be better to show the reason set 3 variables in a slice.

we cannot define a slice like your suggestion.
I will refactor it to

Suggested change

reqs: genSameVecMultiInsertReq(insertNum, []float32{0, 0, 0}, nil),

reqs: genSameVecMultiInsertReq(insertNum, make([]float32, f32VecDim), nil),

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 }

If this function gives us any good merit, it would be better.
Either is fine..

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.

pkg/agent/core/ngt/handler/grpc/handler_test.go

Signed-off-by: kevindiu <[email protected]>

Co-authored-by: Kosuke Morimoto <[email protected]>

* impl multiinsert Equivalence Class Testing Signed-off-by: kevindiu <[email protected]>

* impl multiinsert boundary value test (part1) Signed-off-by: kevindiu <[email protected]> * impl remaining tests Signed-off-by: kevindiu <[email protected]> * change max dim Signed-off-by: kevindiu <[email protected]> * change max dim to reference value Signed-off-by: kevindiu <[email protected]> * change max dim to reference value Signed-off-by: kevindiu <[email protected]> * fix comments Signed-off-by: kevindiu <[email protected]> * rename help function name Signed-off-by: kevindiu <[email protected]> * combine genXXXInsertReq function Signed-off-by: kevindiu <[email protected]> * rename helper function name Signed-off-by: kevindiu <[email protected]>

* impl multiinsert test part 3 Signed-off-by: kevindiu <[email protected]> * fix comment Signed-off-by: kevindiu <[email protected]> * fix comment Signed-off-by: kevindiu <[email protected]>

* implement tests Signed-off-by: kevindiu <[email protected]> * implement remaining tests Signed-off-by: kevindiu <[email protected]> * 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 <[email protected]> * Update pkg/agent/core/ngt/handler/grpc/handler_test.go Co-authored-by: Kiichiro YUKAWA <[email protected]> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Kiichiro YUKAWA <[email protected]>

Co-authored-by: Kiichiro YUKAWA <[email protected]>

Signed-off-by: kevindiu <[email protected]>

github-actions bot added area/agent/core team/set SET team labels Apr 7, 2022

kevindiu changed the title ~~[Draftdraft part of test case for multi insert~~ [Draft] draft test case for multi insert Apr 7, 2022

github-actions bot added size/M type/test Adding tests priority/low labels Apr 7, 2022

kevindiu marked this pull request as draft April 7, 2022 08:29

kevindiu changed the title ~~[Draft] draft test case for multi insert~~ [Draft] Implement multi insert test case for pkg agent handler Apr 7, 2022

kmrmt reviewed Apr 11, 2022

View reviewed changes

pkg/agent/core/ngt/handler/grpc/handler_test.go Show resolved Hide resolved

vankichi reviewed Apr 12, 2022

View reviewed changes

pkg/agent/core/ngt/handler/grpc/handler_test.go Outdated Show resolved Hide resolved

pkg/agent/core/ngt/handler/grpc/handler_test.go Outdated Show resolved Hide resolved

pkg/agent/core/ngt/handler/grpc/handler_test.go Outdated Show resolved Hide resolved

github-actions bot added size/L and removed size/M labels Apr 13, 2022

kevindiu requested review from kmrmt and vankichi April 13, 2022 06:21

github-actions bot added size/M and removed size/M labels Apr 13, 2022

kevindiu force-pushed the test/pkg/add-pkg-agent-handler-multi-insert-test branch from b52126b to 218b308 Compare April 13, 2022 08:38

github-actions bot added the size/M label Apr 13, 2022

kevindiu force-pushed the test/pkg/add-pkg-agent-handler-multi-insert-test branch from 218b308 to cdc3881 Compare April 13, 2022 08:39

github-actions bot added size/M and removed size/M labels Apr 13, 2022

kevindiu force-pushed the test/pkg/add-pkg-agent-handler-multi-insert-test branch from 183aade to d698417 Compare April 13, 2022 09:30

github-actions bot added the size/M label Apr 13, 2022

kevindiu force-pushed the test/pkg/add-pkg-agent-handler-multi-insert-test branch from d698417 to ad2516b Compare April 13, 2022 09:31

github-actions bot removed the size/M label Apr 13, 2022

vankichi mentioned this pull request May 20, 2022

implement update handler test cases #1657

Merged

18 tasks

kevindiu force-pushed the test/pkg/add-pkg-agent-handler-multi-insert-test branch from 196527e to 4e32995 Compare May 20, 2022 04:48

kmrmt previously approved these changes May 20, 2022

View reviewed changes

vankichi reviewed May 24, 2022

View reviewed changes

kevindiu dismissed kmrmt’s stale review via e17cc98 May 24, 2022 02:45

kevindiu and others added 16 commits May 24, 2022 11:46

draft part of test case for multi insert

230223c

Signed-off-by: kevindiu <[email protected]>

fix comments and add decision table test cases

6fcef66

Signed-off-by: kevindiu <[email protected]>

fix decision table test case name

3110147

Signed-off-by: kevindiu <[email protected]>

update test case name

f2eab05

Signed-off-by: kevindiu <[email protected]>

Update pkg/agent/core/ngt/handler/grpc/handler_test.go

45c7c0b

Co-authored-by: Kosuke Morimoto <[email protected]>

Implement multi insert test case for pkg agent handler - part 1 (#1630)

3bbdacd

* impl multiinsert Equivalence Class Testing Signed-off-by: kevindiu <[email protected]>

Implement multi insert test case for pkg agent handler - part 3 (#1646)

51e8c1b

* impl multiinsert test part 3 Signed-off-by: kevindiu <[email protected]> * fix comment Signed-off-by: kevindiu <[email protected]> * fix comment Signed-off-by: kevindiu <[email protected]>

Apply suggestions from code review

e375bd2

Co-authored-by: Kiichiro YUKAWA <[email protected]>

fix comment

3061ec9

Signed-off-by: kevindiu <[email protected]>

fix comment

e2ee73f

Signed-off-by: kevindiu <[email protected]>

fix comment

aeecaa2

Signed-off-by: kevindiu <[email protected]>

fix comment

d122eee

Signed-off-by: kevindiu <[email protected]>

fix comment

a594ffa

Signed-off-by: kevindiu <[email protected]>

fix comment

1232db0

Signed-off-by: kevindiu <[email protected]>

kevindiu force-pushed the test/pkg/add-pkg-agent-handler-multi-insert-test branch from e17cc98 to 1232db0 Compare May 24, 2022 02:46

kevindiu requested review from vankichi and kmrmt May 24, 2022 02:46

vankichi approved these changes May 24, 2022

View reviewed changes

kmrmt approved these changes May 24, 2022

View reviewed changes

kpango merged commit 25d453f into master May 24, 2022

kpango deleted the test/pkg/add-pkg-agent-handler-multi-insert-test branch May 24, 2022 04:40

This was referenced May 24, 2022

[patch] release v1.5.3 #1671

Closed

[patch] release v1.5.3 #1672

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement multi insert test case for pkg agent handler #1612

Implement multi insert test case for pkg agent handler #1612

kevindiu commented Apr 7, 2022 •

edited

Loading

vdaas-ci commented Apr 7, 2022

codecov bot commented Apr 7, 2022 •

edited

Loading

vankichi left a comment

vankichi May 24, 2022

kevindiu May 24, 2022

vankichi May 24, 2022

kevindiu May 24, 2022

vankichi May 24, 2022

vankichi May 24, 2022

kevindiu May 24, 2022 •

edited

Loading

kevindiu May 24, 2022 •

edited

Loading

vankichi May 24, 2022

kevindiu May 24, 2022

kevindiu May 24, 2022

	reqs: genSameVecMultiInsertReq(insertNum, []float32{0, 0, 0}, nil),
	reqs: genSameVecMultiInsertReq(insertNum, [intVecDim]float32, nil),

Implement multi insert test case for pkg agent handler #1612

Implement multi insert test case for pkg agent handler #1612

Conversation

kevindiu commented Apr 7, 2022 • edited Loading

Description:

Related Issue:

How Has This Been Tested?:

Environment:

Types of changes:

Changes to Core Features:

Checklist:

vdaas-ci commented Apr 7, 2022

codecov bot commented Apr 7, 2022 • edited Loading

Codecov Report

vankichi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevindiu May 24, 2022 • edited Loading

Choose a reason for hiding this comment

kevindiu May 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevindiu commented Apr 7, 2022 •

edited

Loading

codecov bot commented Apr 7, 2022 •

edited

Loading

kevindiu May 24, 2022 •

edited

Loading

kevindiu May 24, 2022 •

edited

Loading