Create internal/net/http/client option test #831

kevindiu · 2020-11-12T03:01:06Z

Description:

This PR implements http client option test.
This PR also includes the following changes:

[Test related] add the comparator implementation of transport to the client_test.go
[Refactor] Refactor the error returned from the option and handle it in New()

Related Issue:

How Has This Been Tested?:

Environment:

Go Version: 1.15.2
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.

vdaas-ci · 2020-11-12T03:01:26Z

[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

kevindiu · 2020-11-12T03:02:28Z

/rebase

vdaas-ci · 2020-11-12T03:03:06Z

[REBASE] Rebase triggered by kevindiu for branch: test/net/http/client

vdaas-ci · 2020-11-12T03:03:14Z

[REBASE] Failed to rebase.

internal/net/http/client/client_test.go

github-actions · 2020-11-12T03:06:56Z

internal/net/http/client/option_test.go

+			name: "set header success",
+			args: args{
+				header: http.Header(
+					map[string][]string{"dummy": []string{"val"}},


[golangci] _{reported by reviewdog 🐶}
File is not gofmt-ed with -s (gofmt)

internal/net/http/client/client_test.go

codecov · 2020-11-12T03:16:37Z

Codecov Report

Merging #831 (56d099b) into master (a89ddb6) will increase coverage by 0.39%.
The diff coverage is 77.41%.

@@            Coverage Diff             @@
##           master     #831      +/-   ##
==========================================
+ Coverage   18.15%   18.55%   +0.39%     
==========================================
  Files         422      422              
  Lines       19554    19572      +18     
==========================================
+ Hits         3550     3631      +81     
+ Misses      15793    15733      -60     
+ Partials      211      208       -3

Impacted Files	Coverage Δ
internal/db/nosql/cassandra/cassandra_mock.go	`100.00% <ø> (ø)`
internal/net/http/client/client.go	`0.00% <0.00%> (ø)`
internal/net/http/client/option.go	`100.00% <100.00%> (+100.00%)`	⬆️
internal/worker/queue.go	`95.00% <0.00%> (-3.34%)`	⬇️
internal/file/watch/watch.go	`90.00% <0.00%> (-1.00%)`	⬇️
internal/worker/worker.go	`83.33% <0.00%> (+1.04%)`	⬆️
internal/errgroup/group.go	`93.93% <0.00%> (+1.51%)`	⬆️

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 2e09b7e...229806e. Read the comment docs.

kevindiu · 2020-11-12T05:09:44Z

/format

vdaas-ci · 2020-11-12T05:11:01Z

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

hlts2

LGTM

vankichi · 2020-11-16T02:23:13Z

@kevindiu
why did you change client_test.go?
I think it is not needed in this PR 🤔

kevindiu · 2020-11-16T02:24:57Z

@vankichi I think it is better to write transport comparator in client_test not in option_test, so I changed client_test implementation

internal/net/http/client/option_test.go

vankichi · 2020-11-16T02:46:54Z

internal/net/http/client/option_test.go

+		{
+			name: "set timeout failed with empty value",
+			args: args{
+				dur: "",


I think it is not needed.

@vankichi Do you mean this line dur: "" is not need?
I think it is more clear to write even if the value is the default value of the type, what do you think?

but, as you know, the default value of string is "".
I do not care but it is not necessary, also your opinion is right.
It is the decision problem, any test rule in coding guideline?

There are no such rule in the coding guideline.
I am not sure if I am correct, as all we know the default value of string is "" in golang, we can decide to write this definition or not by ourself (not standardize for everyone, but preference for each developer), what do you think?

Even if we decide to use this pattern, I think this rule only applies to test code, so I think it is better not to write it in coding guideline.

I think it is clear creating test case with no input test case like as int pattern.
And, I think we consider about the unit test practice and create new document for unit testing.
How about it?

I think it is clear creating test case with no input test case like as int pattern.

I think users need to input the value in option (for example WithXXX(""), so I think writing "" is more realistic and easier to understand.

I think we consider about the unit test practice and create new document for unit testing.

I think both ok for me. If you think the code quality of test code is important then I think maybe it is better to create it

well, so how about write the default value expressly in each test?

OK. Let's try it, but Not now

vankichi · 2020-11-16T02:47:37Z

internal/net/http/client/option_test.go

+		{
+			name: "set timeout failed with empty value",
+			args: args{
+				dur: "",


I think it is not needed.

vankichi · 2020-11-16T02:48:32Z

internal/net/http/client/option_test.go

+		{
+			name: "set timeout failed with empty value",
+			args: args{
+				dur: "",


I think it is not needed.

internal/net/http/client/option_test.go

vankichi · 2020-11-16T02:56:59Z

@vankichi I think it is better to write transport comparator in client_test not in option_test, so I changed client_test implementation

@kevindiu
umm...so, you have to show above implementation in your PR description at least and you had to communicate me and @hlts2 before implementation at first. But, it is not a critical problem.

internal/net/http/client/option_test.go

vankichi

LGTM

vankichi · 2020-11-18T02:18:58Z

/rebase
/format
/approve

vdaas-ci · 2020-11-18T02:19:35Z

[REBASE] Rebase triggered by vankichi for branch: test/net/http/client

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

vdaas-ci · 2020-11-18T02:20:25Z

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

vdaas-ci

[APPROVED] This PR is approved by vankichi.

github-actions bot added area/internal team/set SET team size/XXL type/test Adding tests priority/low labels Nov 12, 2020

github-actions bot reviewed Nov 12, 2020

View reviewed changes

kevindiu requested a review from hlts2 November 12, 2020 03:32

hlts2 previously approved these changes Nov 16, 2020

View reviewed changes

kevindiu requested a review from vankichi November 16, 2020 02:05

vankichi reviewed Nov 16, 2020

View reviewed changes

kevindiu dismissed hlts2’s stale review via 95e9e63 November 16, 2020 04:51

github-actions bot added team/sre SRE team type/ci labels Nov 16, 2020

github-actions bot reviewed Nov 16, 2020

View reviewed changes

internal/net/http/client/option_test.go Show resolved Hide resolved

kevindiu requested a review from hlts2 November 16, 2020 05:04

hlts2 previously approved these changes Nov 18, 2020

View reviewed changes

vankichi previously approved these changes Nov 18, 2020

View reviewed changes

fix

52dfbd7

kevindiu and others added 6 commits November 18, 2020 02:19

add option test

bc38fa9

revert client test

91509c9

fix

f8594cd

🤖 Update license headers / Format go codes and yaml files

4c04b55

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

add 0 value test

ebc08e9

remove default value definition in test

229806e

github-actions bot dismissed stale reviews from vankichi and hlts2 via 229806e November 18, 2020 02:19

github-actions bot force-pushed the test/net/http/client branch from 56d099b to 229806e Compare November 18, 2020 02:19

vdaas-ci approved these changes Nov 18, 2020

View reviewed changes

vankichi merged commit b045e86 into master Nov 18, 2020

vankichi deleted the test/net/http/client branch November 18, 2020 08:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create internal/net/http/client option test #831

Create internal/net/http/client option test #831

kevindiu commented Nov 12, 2020 •

edited

Loading

vdaas-ci commented Nov 12, 2020

kevindiu commented Nov 12, 2020

vdaas-ci commented Nov 12, 2020

vdaas-ci commented Nov 12, 2020

github-actions bot Nov 12, 2020

codecov bot commented Nov 12, 2020 •

edited

Loading

kevindiu commented Nov 12, 2020

vdaas-ci commented Nov 12, 2020

hlts2 left a comment

vankichi commented Nov 16, 2020

kevindiu commented Nov 16, 2020

vankichi Nov 16, 2020

kevindiu Nov 16, 2020

vankichi Nov 16, 2020

kevindiu Nov 16, 2020

vankichi Nov 16, 2020

kevindiu Nov 16, 2020 •

edited

Loading

vankichi Nov 17, 2020

vankichi Nov 16, 2020

vankichi Nov 16, 2020

vankichi commented Nov 16, 2020

vankichi left a comment

vankichi commented Nov 18, 2020

vdaas-ci commented Nov 18, 2020

vdaas-ci commented Nov 18, 2020

vdaas-ci left a comment

Create internal/net/http/client option test #831

Create internal/net/http/client option test #831

Conversation

kevindiu commented Nov 12, 2020 • edited Loading

Description:

Related Issue:

How Has This Been Tested?:

Environment:

Types of changes:

Changes to Core Features:

Checklist:

vdaas-ci commented Nov 12, 2020

kevindiu commented Nov 12, 2020

vdaas-ci commented Nov 12, 2020

vdaas-ci commented Nov 12, 2020

github-actions bot Nov 12, 2020

Choose a reason for hiding this comment

codecov bot commented Nov 12, 2020 • edited Loading

Codecov Report

kevindiu commented Nov 12, 2020

vdaas-ci commented Nov 12, 2020

hlts2 left a comment

Choose a reason for hiding this comment

vankichi commented Nov 16, 2020

kevindiu commented Nov 16, 2020

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 Nov 16, 2020 • 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

vankichi commented Nov 16, 2020

vankichi left a comment

Choose a reason for hiding this comment

vankichi commented Nov 18, 2020

vdaas-ci commented Nov 18, 2020

vdaas-ci commented Nov 18, 2020

vdaas-ci left a comment

Choose a reason for hiding this comment

kevindiu commented Nov 12, 2020 •

edited

Loading

codecov bot commented Nov 12, 2020 •

edited

Loading

kevindiu Nov 16, 2020 •

edited

Loading