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.

Sign up for GitHub

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

Jump to bottom

Add integration test for linux and update go version from 1.16 to 1.17 #84

Merged
merged 1 commit into from Mar 15, 2022
Merged

Add integration test for linux and update go version from 1.16 to 1.17 #84

merged 1 commit into from Mar 15, 2022

Conversation

fuweid
Copy link
Member

@fuweid fuweid commented Feb 12, 2022 •
edited

  • Use real CNI plugins to test Setup/Remove API.
  • Update go version from 1.16.x to 1.17.x in CI

Signed-off-by: Wei Fu fuweid89@gmail.com

@fuweid fuweid force-pushed the add-test branch 4 times, most recently from df835e6 to 92c1186 Compare February 12, 2022 09:17
@codecov-commenter
Copy link

codecov-commenter commented Feb 12, 2022 •
edited

Codecov Report

Merging #84 (41d9753) into main (e559bd8) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main      #84   +/-   ##
=======================================
  Coverage   44.41%   44.41%           
=======================================
  Files           9        9           
  Lines         403      403           
=======================================
  Hits          179      179           
  Misses        190      190           
  Partials       34       34

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 e559bd8...41d9753. Read the comment docs.

@fuweid fuweid force-pushed the add-test branch 2 times, most recently from b6bd06d to 0eec5a2 Compare February 12, 2022 09:22
@fuweid fuweid changed the title add integration test in linux add integration test for linux Feb 12, 2022
@fuweid fuweid changed the title add integration test for linux Add integration test for linux and update go version from 1.16 to 1.17 Feb 12, 2022
AkihiroSuda
AkihiroSuda reviewed Feb 12, 2022
View reviewed changes
integration_linux_test.go Outdated
//
// NOTE:
//
// 1. It required that the both bridge and loopback CNI plugins are installed
Copy link
Member

Choose a reason for hiding this comment

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

t.Skip when the requirement is not satisfied

Copy link
Member Author

Choose a reason for hiding this comment

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

Currently, we only have mockCNI case in this repo. And the new case is based on real plugin, which can help us to detect potential issue, like data-race. I think it should fail if the requirement is not satisfied.

Copy link
Member

Choose a reason for hiding this comment

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

It should still be skippable. Just skipping based on a missing requirement isn't the best approach since an error in setup could just end up causing a skip. I might suggest either skipping on testing.Short or putting the integration test in a sub-package

@dmcgowan dmcgowan added this to Ready For Review in Code Review Feb 16, 2022
@fuweid fuweid force-pushed the add-test branch 2 times, most recently from 37e0682 to 911066e Compare February 20, 2022 05:15
@fuweid
Copy link
Member Author

fuweid commented Feb 20, 2022

@dmcgowan @AkihiroSuda Updated. PTAL. Thanks!

@fuweid fuweid force-pushed the add-test branch 2 times, most recently from 6334447 to 41d9753 Compare February 20, 2022 05:29
mikebrow
mikebrow approved these changes Feb 23, 2022
View reviewed changes
Copy link
Member

@mikebrow mikebrow left a comment

Choose a reason for hiding this comment

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

LGTM .. two nits to consider

  1. maybe remove coverage as was done for other projects?
  2. split the go-cni and go-cni/integration tests into two separate tests? in case these particular plugins are not avail for certain platforms going forward.. for example windows (or we could split later I suppose and refactor when we add here)

@fuweid
Copy link
Member Author

fuweid commented Feb 23, 2022 •
edited

Thanks Mike. I can remove coverage. But I don't quite understand the second part nit 😂 I will discuss with you when you are back. :)

@fuweid
Add integration test and update go version
49aa5ab 
* Use real CNI plugins to test Setup/Remove API.
* Update go version from 1.16.x to 1.17.x in CI.
* Remove coverage

Signed-off-by: Wei Fu <fuweid89@gmail.com>
@fuweid
Copy link
Member Author

fuweid commented Feb 23, 2022

Paste the result:

go test -v -race  -count=1 ./...
go: downloading github.com/stretchr/testify v1.6.1
go: downloading github.com/stretchr/objx v0.1.0
=== RUN   TestLibCNIType020
--- PASS: TestLibCNIType020 (0.01s)
=== RUN   TestLibCNIType040
--- PASS: TestLibCNIType040 (0.00s)
=== RUN   TestLibCNIType100
--- PASS: TestLibCNIType100 (0.01s)
PASS
ok  	github.com/containerd/go-cni	0.043s
bin/integration.test -test.v -test.count=1 -test.root  -test.parallel 8
=== RUN   TestBasicSetupAndRemove
    cni_setup_teardown_linux_test.go:168: [0] ip is 10.[88](https://github.com/containerd/go-cni/runs/5305913820?check_suite_focus=true#step:5:88).0.2
    cni_setup_teardown_linux_test.go:168: [1] ip is 10.88.0.3
--- PASS: TestBasicSetupAndRemove (3.63s)
PASS

estesp
estesp approved these changes Feb 23, 2022
View reviewed changes
Copy link
Member

@estesp estesp left a comment

Choose a reason for hiding this comment

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

LGTM

@dmcgowan dmcgowan merged commit 4d65d29 into containerd:main Mar 15, 2022
Code Review automation moved this from Ready For Review to Done Mar 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AkihiroSuda AkihiroSuda AkihiroSuda left review comments

@dmcgowan dmcgowan dmcgowan approved these changes

@estesp estesp estesp approved these changes

@mikebrow mikebrow mikebrow approved these changes

Assignees
No one assigned
Labels
None yet
Projects
Code Review
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@fuweid @codecov-commenter @dmcgowan @estesp @AkihiroSuda @mikebrow