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

IPAM tests #2104

Merged
merged 2 commits into from
Mar 9, 2018
Merged

IPAM tests #2104

merged 2 commits into from
Mar 9, 2018

Conversation

fcrisciani
Copy link

Added tests for swarm mode and also some new parallel tests with various subnets

Signed-off-by: Flavio Crisciani flavio.crisciani@docker.com

@fcrisciani fcrisciani requested a review from abhi March 8, 2018 18:19
@fcrisciani
Copy link
Author

Tests are expected to fail

@fcrisciani fcrisciani force-pushed the test-ipam branch 2 times, most recently from 8686170 to 5ce03c1 Compare March 8, 2018 18:35
@abhi
Copy link
Contributor

abhi commented Mar 8, 2018

@mghazizadeh @antonybichon17 @andrewhsu This is one of the test scenarios that reproduces the issue. The fix and additional tests are available at #2105.
Each of these test and the ones in #2105 should ideally fail consistently without the fix.

@fcrisciani
Copy link
Author

To not lose the failure on master: https://circleci.com/gh/docker/libnetwork/5992

@fcrisciani fcrisciani force-pushed the test-ipam branch 3 times, most recently from 51039f2 to 36d4b8e Compare March 8, 2018 22:10
@codecov-io
Copy link

codecov-io commented Mar 8, 2018 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (master@834b391). Click here to learn what that means.
The diff coverage is 84.84%.

Impacted file tree graph

@@           Coverage Diff            @@
##             master   #2104   +/-   ##
========================================
  Coverage          ?   40.4%           
========================================
  Files             ?     139           
  Lines             ?   22376           
  Branches          ?       0           
========================================
  Hits              ?    9040           
  Misses            ?   12008           
  Partials          ?    1328
Impacted Files Coverage Δ
ipam/allocator.go 70.84% <75%> (ø)
bitseq/sequence.go 83.33% <86.2%> (ø)

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 834b391...fb561e9. Read the comment docs.

ddebroy
ddebroy reviewed Mar 9, 2018
View reviewed changes
Copy link
Contributor

@ddebroy ddebroy left a comment •
edited
Loading

Choose a reason for hiding this comment

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

For the sake of unit testing completeness and coverage, will it make sense to add some tests to TestSequenceGetAvailableBit with sequence blocks with holes in the middle like say 0xffcfffff, from set to somewhere in the middle beyond which there are no bits like 8 and expect invalidPos, invalidPos to cover the new full-detection in getAvailableBit in PR 2105?

@abhi
Copy link
Contributor

abhi commented Mar 9, 2018

@ddebroy I have added that scenario implicitly in that test where it moves on to the next block because of the invalidPos. Are you suggesting we explicitly add it as well ?

@ddebroy
Copy link
Contributor

ddebroy commented Mar 9, 2018

Right ... that was my suggestion. Not a high priority and can be added later for completeness since it's implicitly covered right now as you mentioned.

abhi
abhi approved these changes Mar 9, 2018
View reviewed changes
Copy link
Contributor

@abhi abhi left a comment

Choose a reason for hiding this comment

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

LGTM

Flavio Crisciani added 2 commits March 9, 2018 11:07
Vendor golang/sync
e46e781 
Signed-off-by: Flavio Crisciani <flavio.crisciani@docker.com>
IPAM tests
fb561e9 
Added tests for swarm mode and also some new parallel tests

Signed-off-by: Flavio Crisciani <flavio.crisciani@docker.com>
@fcrisciani fcrisciani merged commit 2112cac into moby:master Mar 9, 2018
@fcrisciani fcrisciani deleted the test-ipam branch March 9, 2018 19:31
This was referenced Mar 12, 2018
Merged
Merged
Merged
thaJeztah
thaJeztah reviewed Mar 14, 2018
View reviewed changes
vendor.conf
@@ -50,5 +50,6 @@ github.com/vishvananda/netns 604eaf189ee867d8c147fafc28def2394e878d25
golang.org/x/crypto 558b6879de74bc843225cde5686419267ff707ca
golang.org/x/net 7dcfb8076726a3fdd9353b6b8a1f1b6be6811bd6
golang.org/x/sys 07c182904dbd53199946ba614a412c61d3c548f5
github.com/golang/sync fd80eb99c8f653c847d294a001bdf2a3a6f768f5
Copy link
Member

Choose a reason for hiding this comment

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

This uses the wrong (non-canonical) import path; see #2111 (and https://github.com/moby/moby/pull/36589/files#r174434213)

@thaJeztah thaJeztah mentioned this pull request Mar 14, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thaJeztah thaJeztah thaJeztah left review comments

@ddebroy ddebroy ddebroy left review comments

@abhi abhi abhi approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@fcrisciani @abhi @codecov-io @ddebroy @thaJeztah