Add test for `s3/writer` and Refactor. #672

hlts2 · 2020-09-03T08:05:33Z

Signed-off-by: hlts2 hiroto.funakoshi.hiroto@gmail.com

Description:

I refactored and added the tests for s3/writer.
And when I rebase PR, I found a build error. So I fixed it.

Refactor

WHY
I can not write the tests for this method because the object is created internally and really need to connect to s3.
https://github.com/vdaas/vald/blob/master/internal/db/storage/blob/s3/writer/writer.go#L99-L122
WHAT
so I want to rewrite to be able to mock.
For example. I’m thinking of making an s3manager wrapper like add test of internal/db/rdb/mysql #659 and Refactoring and Add test code for compress #622 .

Related issue

Autor Review Required

Review Step

Related Issue:

How Has This Been Tested?:

Environment:

Go Version: 1.15.1
Docker Version: 19.03.8
Kubernetes Version: 1.18.2
NGT Version: 1.12.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.

pull-assistant · 2020-09-03T08:05:35Z

Best reviewed: commit by commit

Optimal code review plan (2 warnings)

feat: wrapper for s3 manager

fix: refactor

...orage/blob/s3/writer/writer.go 50% changes removed in fix: s3 type

fix: s3 type

fix typo

.../sdk/s3/s3manager/s3manager.go 44% changes removed in fix: apply suggestio...

...orage/blob/s3/writer/writer.go 67% changes removed in fix: apply suggestio...

fix: apply suggestion

add test for writer

delete unncessary file

delete unncessary code

fix: apply feedback

fix: add comment

fix: fatal error

fix: apply suggestion

🤖 Update license headers / Format go codes and yaml files

Powered by Pull Assistant. Last update bc33027 ... 5fc1dc0. Read the comment docs.

vdaas-ci · 2020-09-03T08:05:47Z

[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

codecov · 2020-09-03T08:31:35Z

Codecov Report

Merging #672 into master will decrease coverage by 1.01%.
The diff coverage is 20.97%.

@@            Coverage Diff             @@
##           master     #672      +/-   ##
==========================================
- Coverage   15.35%   14.33%   -1.02%     
==========================================
  Files         412      416       +4     
  Lines       21629    19378    -2251     
==========================================
- Hits         3321     2778     -543     
+ Misses      18060    16366    -1694     
+ Partials      248      234      -14

Impacted Files	Coverage Δ
internal/compress/zstd.go	`100.00% <ø> (+69.35%)`	⬆️
internal/config/ngt.go	`100.00% <ø> (ø)`
internal/db/nosql/cassandra/cassandra.go	`0.00% <0.00%> (ø)`
internal/errors/http.go	`0.00% <0.00%> (ø)`
internal/errors/option.go	`0.00% <0.00%> (ø)`
...ternal/observability/metrics/agent/core/ngt/ngt.go	`0.00% <0.00%> (ø)`
pkg/agent/core/ngt/service/ngt.go	`0.00% <0.00%> (ø)`
pkg/agent/core/ngt/service/option.go	`0.00% <0.00%> (-12.77%)`	⬇️
pkg/agent/core/ngt/usecase/agentd.go	`0.00% <0.00%> (ø)`
...ger/replication/controller/handler/grpc/handler.go	`0.00% <ø> (ø)`
... and 406 more

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 7cf4b77...5fc1dc0. Read the comment docs.

hlts2 · 2020-09-04T06:20:54Z

/rebase

vdaas-ci · 2020-09-04T06:21:32Z

[REBASE] Rebase triggered by hlts2 for branch: test/internal/storage_blob_s3_writer

internal/db/storage/blob/s3/sdk/s3/s3manager/s3manager.go

internal/db/storage/blob/s3/sdk/s3/s3iface/s3iface.go

kevindiu

LGTM

internal/db/storage/blob/s3/writer/mock_test.go

internal/db/storage/blob/s3/writer/writer_test.go

internal/db/storage/blob/s3/writer/mock_test.go

internal/db/storage/blob/s3/writer/writer_test.go

hlts2 · 2020-09-14T02:37:17Z

/rebase

vdaas-ci · 2020-09-14T02:37:51Z

[REBASE] Rebase triggered by hlts2 for branch: test/internal/storage_blob_s3_writer

kevindiu · 2020-09-14T06:29:08Z

Can you update the test case to cover this logic?

vald/internal/db/storage/blob/s3/writer/writer.go

Line 93 in b075b0f

w.wg.Wait()

kevindiu · 2020-09-14T06:40:04Z

/rebase

Signed-off-by: hlts2 <hiroto.funakoshi.hiroto@gmail.com>

kevindiu

LGTM

internal/db/storage/blob/s3/writer/writer_test.go

Signed-off-by: hlts2 <hiroto.funakoshi.hiroto@gmail.com>

kevindiu

LGTM

vankichi · 2020-09-24T02:57:15Z

/rebase
/format
/approve

vdaas-ci · 2020-09-24T02:57:48Z

[REBASE] Rebase triggered by vankichi for branch: test/internal/storage_blob_s3_writer

vdaas-ci · 2020-09-24T02:58:33Z

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

Signed-off-by: vdaas-ci <ci@vdaas.org>

vdaas-ci

[APPROVED] This PR is approved by vankichi.

hlts2 self-assigned this Sep 3, 2020

github-actions bot added area/internal size/M type/test Adding tests priority/low labels Sep 3, 2020

vdaas-ci force-pushed the test/internal/storage_blob_s3_writer branch from 327af2e to 58a4145 Compare September 4, 2020 06:21

github-actions bot reviewed Sep 4, 2020

View reviewed changes

internal/db/storage/blob/s3/sdk/s3/s3manager/s3manager.go Show resolved Hide resolved

kpango reviewed Sep 7, 2020

View reviewed changes

internal/db/storage/blob/s3/sdk/s3/s3manager/s3manager.go Outdated Show resolved Hide resolved

github-actions bot reviewed Sep 7, 2020

View reviewed changes

internal/db/storage/blob/s3/sdk/s3/s3iface/s3iface.go Show resolved Hide resolved

kevindiu previously approved these changes Sep 7, 2020

View reviewed changes

hlts2 dismissed kevindiu’s stale review via 1eb1158 September 7, 2020 08:25

github-actions bot added the team/set SET team label Sep 7, 2020

hlts2 changed the title ~~[WIP] Add test for s3/writer and Refactor.~~ Add test for s3/writer and Refactor. Sep 7, 2020

github-actions bot added the size/XL label Sep 7, 2020

hlts2 marked this pull request as ready for review September 7, 2020 08:26

github-actions bot removed the size/M label Sep 7, 2020

hlts2 requested a review from kevindiu September 7, 2020 08:29

github-actions bot reviewed Sep 7, 2020

View reviewed changes

internal/db/storage/blob/s3/writer/mock_test.go Show resolved Hide resolved

kevindiu reviewed Sep 8, 2020

View reviewed changes

internal/db/storage/blob/s3/writer/writer_test.go Outdated Show resolved Hide resolved

internal/db/storage/blob/s3/writer/writer_test.go Show resolved Hide resolved

vdaas-ci force-pushed the test/internal/storage_blob_s3_writer branch from 73532eb to b075b0f Compare September 14, 2020 02:38

hlts2 added 6 commits September 23, 2020 02:39

fix typo

b2805c9

Signed-off-by: hlts2 <hiroto.funakoshi.hiroto@gmail.com>

fix: apply suggestion

f2eae1f

Signed-off-by: hlts2 <hiroto.funakoshi.hiroto@gmail.com>

add test for writer

fc75a65

Signed-off-by: hlts2 <hiroto.funakoshi.hiroto@gmail.com>

delete unncessary file

4ce808c

Signed-off-by: hlts2 <hiroto.funakoshi.hiroto@gmail.com>

delete unncessary code

83a7f2c

Signed-off-by: hlts2 <hiroto.funakoshi.hiroto@gmail.com>

fix: apply feedback

4ecae9b

Signed-off-by: hlts2 <hiroto.funakoshi.hiroto@gmail.com>

vdaas-ci dismissed kevindiu’s stale review via 4ecae9b September 23, 2020 02:39

vdaas-ci force-pushed the test/internal/storage_blob_s3_writer branch from f7bc2e3 to 4ecae9b Compare September 23, 2020 02:39

hlts2 added 2 commits September 23, 2020 11:44

fix: add comment

c8c99d6

Signed-off-by: hlts2 <hiroto.funakoshi.hiroto@gmail.com>

fix: fatal error

e08b21a

Signed-off-by: hlts2 <hiroto.funakoshi.hiroto@gmail.com>

github-actions bot added the area/manager/replication label Sep 23, 2020

hlts2 added 3 commits September 23, 2020 12:18

fix: fatal error

0ad5a6f

Signed-off-by: hlts2 <hiroto.funakoshi.hiroto@gmail.com>

fix: fatal error

1f35b86

Signed-off-by: hlts2 <hiroto.funakoshi.hiroto@gmail.com>

fix: fatal error

92f7fff

Signed-off-by: hlts2 <hiroto.funakoshi.hiroto@gmail.com>

kevindiu previously approved these changes Sep 23, 2020

View reviewed changes

vankichi reviewed Sep 23, 2020

View reviewed changes

internal/db/storage/blob/s3/writer/writer_test.go Outdated Show resolved Hide resolved

internal/db/storage/blob/s3/writer/writer_test.go Show resolved Hide resolved

fix: apply suggestion

8b6e211

Signed-off-by: hlts2 <hiroto.funakoshi.hiroto@gmail.com>

hlts2 dismissed kevindiu’s stale review via 8b6e211 September 24, 2020 01:43

fix: apply suggestion

6716040

Signed-off-by: hlts2 <hiroto.funakoshi.hiroto@gmail.com>

kevindiu previously approved these changes Sep 24, 2020

View reviewed changes

vankichi previously approved these changes Sep 24, 2020

View reviewed changes

🤖 Update license headers / Format go codes and yaml files

5fc1dc0

Signed-off-by: vdaas-ci <ci@vdaas.org>

vdaas-ci dismissed stale reviews from vankichi and kevindiu via 5fc1dc0 September 24, 2020 02:59

vdaas-ci approved these changes Sep 24, 2020

View reviewed changes

vankichi merged commit 34853a0 into master Sep 24, 2020

vankichi deleted the test/internal/storage_blob_s3_writer branch September 24, 2020 05:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add test for `s3/writer` and Refactor. #672

Add test for `s3/writer` and Refactor. #672

hlts2 commented Sep 3, 2020 •

edited

Loading

pull-assistant bot commented Sep 3, 2020 •

edited

Loading

vdaas-ci commented Sep 3, 2020

codecov bot commented Sep 3, 2020 •

edited

Loading

hlts2 commented Sep 4, 2020

vdaas-ci commented Sep 4, 2020

kevindiu left a comment

hlts2 commented Sep 14, 2020

vdaas-ci commented Sep 14, 2020

kevindiu commented Sep 14, 2020

kevindiu commented Sep 14, 2020

kevindiu left a comment

kevindiu left a comment

vankichi commented Sep 24, 2020

vdaas-ci commented Sep 24, 2020

vdaas-ci commented Sep 24, 2020

vdaas-ci left a comment

Add test for s3/writer and Refactor. #672

Add test for s3/writer and Refactor. #672

Conversation

hlts2 commented Sep 3, 2020 • edited Loading

Description:

Refactor

Related issue

Review Step

Related Issue:

How Has This Been Tested?:

Environment:

Types of changes:

Changes to Core Features:

Checklist:

pull-assistant bot commented Sep 3, 2020 • edited Loading

vdaas-ci commented Sep 3, 2020

codecov bot commented Sep 3, 2020 • edited Loading

Codecov Report

hlts2 commented Sep 4, 2020

vdaas-ci commented Sep 4, 2020

kevindiu left a comment

Choose a reason for hiding this comment

hlts2 commented Sep 14, 2020

vdaas-ci commented Sep 14, 2020

kevindiu commented Sep 14, 2020

kevindiu commented Sep 14, 2020

kevindiu left a comment

Choose a reason for hiding this comment

kevindiu left a comment

Choose a reason for hiding this comment

vankichi commented Sep 24, 2020

vdaas-ci commented Sep 24, 2020

vdaas-ci commented Sep 24, 2020

vdaas-ci left a comment

Choose a reason for hiding this comment

Add test for `s3/writer` and Refactor. #672

Add test for `s3/writer` and Refactor. #672

hlts2 commented Sep 3, 2020 •

edited

Loading

pull-assistant bot commented Sep 3, 2020 •

edited

Loading

codecov bot commented Sep 3, 2020 •

edited

Loading