Skip to content
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

test: add test cases about CRI image #2030

Merged

Conversation

starnop
Copy link
Contributor

@starnop starnop commented Aug 1, 2018

Signed-off-by: Starnop [email protected]

Ⅰ. Describe what this PR did

Ⅱ. Does this pull request fix one issue?

fixes part of #1756

Ⅲ. Describe how you did it

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

@allencloud
Copy link
Collaborator

CI fails:

gometalinter
gometalinter --config .gometalinter.json ./...
cri/v1alpha1/cri_utils_test.go:1::warning: file is not gofmted with -s (gofmt)
cri/v1alpha1/cri_utils_test.go:1::warning: file is not goimported (goimports)
Makefile:122: recipe for target 'gometalinter' failed
make: *** [gometalinter] Error 1

@starnop

@codecov-io
Copy link

codecov-io commented Aug 2, 2018

Codecov Report

Merging #2030 into master will increase coverage by 5.14%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2030      +/-   ##
==========================================
+ Coverage   59.63%   64.77%   +5.14%     
==========================================
  Files         208      208              
  Lines       16482    16482              
==========================================
+ Hits         9829    10677     +848     
+ Misses       5453     4474     -979     
- Partials     1200     1331     +131
Flag Coverage Δ
#criv1alpha1test 32.99% <ø> (-0.14%) ⬇️
#criv1alpha2test 33.74% <ø> (?)
#integrationtest 39.48% <ø> (-0.04%) ⬇️
#unittest 24.09% <ø> (+0.19%) ⬆️
Impacted Files Coverage Δ
cri/v1alpha1/cri.go 63.44% <0%> (-0.35%) ⬇️
daemon/mgr/container.go 55.66% <0%> (-0.21%) ⬇️
daemon/logger/jsonfile/utils.go 73.17% <0%> (+1.62%) ⬆️
apis/server/utils.go 66.66% <0%> (+4.76%) ⬆️
cri/stream/runtime.go 68.23% <0%> (+5.88%) ⬆️
cri/stream/remotecommand/httpstream.go 44.04% <0%> (+9.32%) ⬆️
cri/criservice.go 64.7% <0%> (+24.99%) ⬆️
cri/v1alpha2/cri_utils.go 82.33% <0%> (+27.18%) ⬆️
cri/v1alpha2/cri_wrapper.go 53.87% <0%> (+53.87%) ⬆️
cri/v1alpha2/cri.go 64.63% <0%> (+64.63%) ⬆️
... and 4 more

@allencloud
Copy link
Collaborator

any update and review? @YaoZengzeng

Username: "foo",
Volumes: parseVolumesFromPouch(map[string]interface{}{"foo": "foo"}),
},
wantErr: nil,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The type of wantErr is bool, we could not assign nil to it.

CI has found this mistake but not failed.

imageUser: "",
},
want: nil,
want1: "",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use a better name, other than want1 :)

args: args{
imageUser: "",
},
want: nil,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should check whether the value is equal other then the pointer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your comment. All above mentioned has been done.

@starnop starnop force-pushed the cri-test-ImageUser branch 3 times, most recently from b598225 to f195bae Compare August 20, 2018 11:37
@allencloud
Copy link
Collaborator

CRI unit test fails, @starnop :

=== RUN   Test_imageToCriImage/nil_test
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x168 pc=0x1b74885]

@starnop starnop force-pushed the cri-test-ImageUser branch 4 times, most recently from 36be0d5 to efd1dbd Compare August 21, 2018 06:13
@starnop starnop closed this Aug 21, 2018
@starnop starnop reopened this Aug 21, 2018
@starnop starnop force-pushed the cri-test-ImageUser branch from efd1dbd to 796d025 Compare August 21, 2018 07:33
@YaoZengzeng
Copy link
Contributor

LGTM

@YaoZengzeng YaoZengzeng merged commit 1f7183e into AliyunContainerService:master Aug 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants