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

ci: add deadcode checking in gometalinter #2442

Merged

Conversation

allencloud
Copy link
Collaborator

@allencloud allencloud commented Nov 7, 2018

Signed-off-by: Allen Sun [email protected]

Ⅰ. Describe what this PR did

Since #2426 has been merged, then we could add deadcode checking in gometalinter CIs.

If we directly use the following command without --skip test,

gometalinter --disable-all --skip vendor  -E deadcode ./...

It will show result like

test/util_api.go:34:1:warning: CreateBusyboxContainerOk is unused (deadcode)
test/util_api.go:68:1:warning: CreateBusybox125Container is unused (deadcode)
test/util_api.go:85:1:warning: StartContainerOk is unused (deadcode)
test/util_api.go:94:1:warning: StopContainerOk is unused (deadcode)
test/util_api.go:103:1:warning: CheckContainerStatus is unused (deadcode)
test/util_api.go:115:1:warning: CheckContainerRunning is unused (deadcode)
test/util_api.go:136:1:warning: PauseContainerOk is unused (deadcode)
test/util_api.go:145:1:warning: UnpauseContainerOk is unused (deadcode)
test/util_api.go:153:1:warning: DelContainerForceMultyTime is unused (deadcode)

But the function CreateBusyboxContainerOk and so on is not deadcode at all. We checked that in test code. So, I am wondering if tool deadcode does not work well in test directory.

So I add --skip test.

Ⅱ. Does this pull request fix one issue?

none, but related with #2423.

Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗?)

no need

Ⅳ. Describe how to verify it

none

Ⅴ. Special notes for reviews

none

@codecov
Copy link

codecov bot commented Nov 7, 2018

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #2442      +/-   ##
=========================================
+ Coverage   64.46%   68.6%   +4.13%     
=========================================
  Files         272     277       +5     
  Lines       18179   18367     +188     
=========================================
+ Hits        11720   12600     +880     
+ Misses       5180    4332     -848     
- Partials     1279    1435     +156
Flag Coverage Δ
#criv1alpha1test 31.39% <ø> (?)
#criv1alpha2test 35.75% <ø> (ø) ⬆️
#integrationtest 40.18% <ø> (+0.02%) ⬆️
#nodee2etest 33.02% <ø> (+0.15%) ⬆️
#unittest 26.52% <ø> (+0.01%) ⬆️
Impacted Files Coverage Δ
ctrd/utils.go 100% <ø> (+9.58%) ⬆️
cri/ocicni/cni_manager.go 59.18% <0%> (-12.25%) ⬇️
apis/server/system_bridge.go 37.19% <0%> (-10.59%) ⬇️
cri/v1alpha2/cri_wrapper.go 62.4% <0%> (ø) ⬆️
cri/v1alpha2/cri_utils.go 91.21% <0%> (ø) ⬆️
pkg/utils/identify.go 0% <0%> (ø)
apis/server/types/handler.go 0% <0%> (ø)
apis/server/cri_bridge.go 0% <0%> (ø)
hookplugins/APIPlugin.go 100% <0%> (ø)
hookplugins/apiplugin/apiplugin.go 100% <0%> (ø)
... and 14 more

@ZYecho
Copy link
Contributor

ZYecho commented Nov 7, 2018

My bad, I remove the func ociImageToPouchImage which reference rootFSToAPIType in #2426, which result another deadcode rootFSToAPIType but not include the reports in #2423. Could you help to remove in this pr? @allencloud Sorry for lack of double-check.

Copy link
Contributor

@fuweid fuweid left a comment

Choose a reason for hiding this comment

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

LGTM

@allencloud allencloud merged commit 0ecdefb into AliyunContainerService:master Nov 8, 2018
@allencloud allencloud deleted the add-deadcode-checking branch November 8, 2018 02:45
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.

4 participants