fix MySQL panic test #996

vankichi · 2021-02-09T10:07:42Z

Fix the panic error of MySQL unit test.

Description:

I researched the panic error during the MySQL unit test.
This error may be caused by nil pointer of the session which is one of the MySQL client struct values.
My change is following:

added the nil check for each function that uses session.
added new error type for MySQL: ErrMySQLSessionNil
fix the code for using session mock.

I need the authors' review.

Related Issue:

How Has This Been Tested?:

Environment:

Go Version: 1.15.7
Docker Version: 19.03.8
Kubernetes Version: 1.18.2
NGT Version: 1.13.1

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 · 2021-02-09T10:08:06Z

[CHATOPS:HELP] ChatOps commands.

🙆‍♀️ /approve - approve
💌 /changelog - add changelog comment
🍱 /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

kpango · 2021-02-09T10:13:14Z

internal/errors/mysql.go

@@ -50,6 +50,11 @@ var (
 	ErrRequiredMemberNotFilled = func(member string) error {
 		return Wrapf(NewErrMySQLInvalidArgumentIdentity(), "error required member not filled (member: %s)", member)
 	}
+
+	// ErrMySQLSessionNil represents a function to generate an error that the MySQL session is nil.
+	ErrMySQLSessionNil = func() error {


I think this case you shouldn't use functional type for error, but if you add additional information such as below it's better

db string host string port int user string pass string name string charset string

@kpango so, for example, should I implement the below code instead of this?

if m.session == nil { return errors.Errorf("session is nil, { db: %s, host: %s, port: %d, user: %s, pass: %s, name: %s, charset: %s", m.db, m.host, m.port, m.user, m.pass, m.name, m.charset) }

Suggested change

ErrMySQLSessionNil = func() error {

ErrMySQLSessionNil = func(db, host, user, name, charset string, port int) error {

return errors.Errorf("session is nil, { db: %s, host: %s, port: %d, user: %s, name: %s, charset: %s", db, host, port, user, name, charset)

or you can print them on debug log.

I think debug log is easy.

@kpango thx for the review, I updated the code 🙏

internal/db/rdb/mysql/mysql_test.go

codecov · 2021-02-09T10:16:01Z

Codecov Report

Merging #996 (fdf9a3a) into master (4bf65f8) will increase coverage by 0.12%.
The diff coverage is 95.34%.

@@            Coverage Diff             @@
##           master     #996      +/-   ##
==========================================
+ Coverage   14.25%   14.37%   +0.12%     
==========================================
  Files         494      494              
  Lines       28294    28336      +42     
==========================================
+ Hits         4033     4073      +40     
- Misses      24012    24013       +1     
- Partials      249      250       +1

Impacted Files	Coverage Δ
internal/errors/mysql.go	`100.00% <ø> (ø)`
internal/db/rdb/mysql/mysql.go	`94.46% <95.34%> (+0.15%)`	⬆️
internal/worker/worker.go	`82.29% <0.00%> (-1.05%)`	⬇️
internal/worker/queue.go	`100.00% <0.00%> (+1.66%)`	⬆️

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 0351002...6473f03. Read the comment docs.

vankichi · 2021-02-15T09:52:06Z

/rebase
/format

vdaas-ci · 2021-02-15T09:52:44Z

[REBASE] Rebase triggered by vankichi for branch: test/internal/fix-mysql-paniced-test

vdaas-ci · 2021-02-15T09:54:03Z

[FORMAT] Updating license headers and formatting go codes triggered by vankichi.

internal/db/rdb/mysql/mysql.go

kevindiu · 2021-02-16T07:06:13Z

missing session validation in SetVector().
https://github.com/vdaas/vald/pull/996/files#diff-3b634a30e03f37629a7475da00b816954f658f6ee80a63c56f74a93de34a2795R288

kevindiu · 2021-02-16T07:07:36Z

please also add author review required to the PR description :)

internal/db/rdb/mysql/mysql.go

kevindiu · 2021-02-19T05:02:05Z

/rebase
/format

vdaas-ci · 2021-02-19T05:02:41Z

[REBASE] Rebase triggered by kevindiu for branch: test/internal/fix-mysql-paniced-test

vdaas-ci · 2021-02-19T05:04:39Z

[FORMAT] Updating license headers and formatting go codes triggered by kevindiu.

vankichi · 2021-02-19T06:03:45Z

/rebase

vdaas-ci · 2021-02-19T06:04:22Z

[REBASE] Rebase triggered by vankichi for branch: test/internal/fix-mysql-paniced-test

internal/db/rdb/mysql/mysql.go

vankichi · 2021-02-19T07:31:35Z

/rebase
/format

vdaas-ci · 2021-02-19T07:32:12Z

[REBASE] Rebase triggered by vankichi for branch: test/internal/fix-mysql-paniced-test

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

vdaas-ci · 2021-02-19T07:33:14Z

[FORMAT] Updating license headers and formatting go codes triggered by vankichi.

kevindiu

LGTM

hlts2

thank you 🙇
LGTM

kpango

LGTM

github-actions bot added area/internal team/set SET team size/L type/test Adding tests priority/low labels Feb 9, 2021

kpango reviewed Feb 9, 2021

View reviewed changes

github-actions bot reviewed Feb 9, 2021

View reviewed changes

internal/db/rdb/mysql/mysql_test.go Outdated Show resolved Hide resolved

internal/db/rdb/mysql/mysql_test.go Outdated Show resolved Hide resolved

vankichi changed the title ~~[WIP] fix MySQL panic test~~ fix MySQL panic test Feb 15, 2021

vankichi marked this pull request as ready for review February 15, 2021 09:51

vdaas-ci force-pushed the test/internal/fix-mysql-paniced-test branch from 2550884 to ac6ee4e Compare February 15, 2021 09:52

vankichi requested review from a team, kmrmt and hlts2 and removed request for a team February 15, 2021 09:53

vankichi requested review from kevindiu and removed request for kmrmt February 15, 2021 09:54

vankichi self-assigned this Feb 15, 2021

kevindiu suggested changes Feb 16, 2021

View reviewed changes

hlts2 reviewed Feb 19, 2021

View reviewed changes

internal/db/rdb/mysql/mysql.go Outdated Show resolved Hide resolved

vdaas-ci force-pushed the test/internal/fix-mysql-paniced-test branch from 1fe0da9 to 2e13626 Compare February 19, 2021 05:02

kevindiu reviewed Feb 19, 2021

View reviewed changes

internal/db/rdb/mysql/mysql.go Outdated Show resolved Hide resolved

vankichi added 8 commits February 19, 2021 07:32

✅ ♻️ add new defined errror for MySQL

4247b37

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

✅ ♻️ 🐛 fix MySQL_Client_Open test

fa2f238

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

✅ ♻️ remove unnecessary code and comment out

058d6dc

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

✅ ♻️ fix ErrMySQLSessionNil

b067c75

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

✅ ♻️ add test

c28b48f

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

✅ ♻️ add test

874db7f

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

✅ fix code from review

fc8cdf7

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

✅ add outputLog for mysql error

6473f03

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

vdaas-ci force-pushed the test/internal/fix-mysql-paniced-test branch from a84417f to 6473f03 Compare February 19, 2021 07:32

kevindiu approved these changes Feb 19, 2021

View reviewed changes

hlts2 approved these changes Feb 19, 2021

View reviewed changes

vankichi added the status/author-review label Feb 19, 2021

kevindiu requested a review from kpango February 19, 2021 07:47

kpango approved these changes Feb 19, 2021

View reviewed changes

kpango merged commit 0ce53b6 into master Feb 19, 2021

kpango deleted the test/internal/fix-mysql-paniced-test branch February 19, 2021 08:04

vdaas-ci mentioned this pull request Feb 19, 2021

[patch] v1.0.3 release #1026

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix MySQL panic test #996

fix MySQL panic test #996

vankichi commented Feb 9, 2021 •

edited

Loading

vdaas-ci commented Feb 9, 2021

kpango Feb 9, 2021

vankichi Feb 15, 2021

kpango Feb 15, 2021

kpango Feb 15, 2021

kpango Feb 15, 2021

vankichi Feb 15, 2021

codecov bot commented Feb 9, 2021 •

edited

Loading

vankichi commented Feb 15, 2021

vdaas-ci commented Feb 15, 2021

vdaas-ci commented Feb 15, 2021

kevindiu commented Feb 16, 2021

kevindiu commented Feb 16, 2021

kevindiu commented Feb 19, 2021

vdaas-ci commented Feb 19, 2021

vdaas-ci commented Feb 19, 2021

vankichi commented Feb 19, 2021

vdaas-ci commented Feb 19, 2021

vankichi commented Feb 19, 2021

vdaas-ci commented Feb 19, 2021

vdaas-ci commented Feb 19, 2021

kevindiu left a comment

hlts2 left a comment

kpango left a comment

	ErrMySQLSessionNil = func() error {
	ErrMySQLSessionNil = func(db, host, user, name, charset string, port int) error {
	return errors.Errorf("session is nil, { db: %s, host: %s, port: %d, user: %s, name: %s, charset: %s", db, host, port, user, name, charset)

fix MySQL panic test #996

fix MySQL panic test #996

Conversation

vankichi commented Feb 9, 2021 • edited Loading

Description:

Related Issue:

How Has This Been Tested?:

Environment:

Types of changes:

Changes to Core Features:

Checklist:

vdaas-ci commented Feb 9, 2021

kpango Feb 9, 2021

Choose a reason for hiding this comment

vankichi Feb 15, 2021

Choose a reason for hiding this comment

kpango Feb 15, 2021

Choose a reason for hiding this comment

kpango Feb 15, 2021

Choose a reason for hiding this comment

kpango Feb 15, 2021

Choose a reason for hiding this comment

vankichi Feb 15, 2021

Choose a reason for hiding this comment

codecov bot commented Feb 9, 2021 • edited Loading

Codecov Report

vankichi commented Feb 15, 2021

vdaas-ci commented Feb 15, 2021

vdaas-ci commented Feb 15, 2021

kevindiu commented Feb 16, 2021

kevindiu commented Feb 16, 2021

kevindiu commented Feb 19, 2021

vdaas-ci commented Feb 19, 2021

vdaas-ci commented Feb 19, 2021

vankichi commented Feb 19, 2021

vdaas-ci commented Feb 19, 2021

vankichi commented Feb 19, 2021

vdaas-ci commented Feb 19, 2021

vdaas-ci commented Feb 19, 2021

kevindiu left a comment

Choose a reason for hiding this comment

hlts2 left a comment

Choose a reason for hiding this comment

kpango left a comment

Choose a reason for hiding this comment

vankichi commented Feb 9, 2021 •

edited

Loading

codecov bot commented Feb 9, 2021 •

edited

Loading