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

Fix TestInspectInt64 for kernels without swap limit capability #16986

Merged
merged 1 commit into from
Oct 19, 2015
Merged

Fix TestInspectInt64 for kernels without swap limit capability #16986

merged 1 commit into from
Oct 19, 2015

Conversation

clnperez
Copy link
Contributor

Instead of returning only the container ID, starting a container may
also return a warning:

"WARNING: Your kernel does not support swap
limit capabilities, memory limited without
swap.\nff6ebd9f7a8d035d17bb9a61eb9d3f0a5d563160cc43471a9d7ac9f71945d061"

Update the test requirement and move to a _unix_test.go file.

Signed-off-by: Christy Perez christy@linux.vnet.ibm.com

@cpuguy83
Copy link
Member

Instead this should have a testRequires(c, memoryLimitSupport) and move to a _unix_test.go file.

@clnperez clnperez force-pushed the test-inspect-warn-fix branch from 4c60dfc to 1437c92 Compare October 13, 2015 20:16
@thaJeztah
Copy link
Member

ping @cpuguy83 I see this PR was updated PTAL

vdemeester
vdemeester reviewed Oct 13, 2015
View reviewed changes
integration-cli/docker_cli_inspect_unix_test.go
)

func (s *DockerSuite) TestInspectInt64(c *check.C) {
testRequires(c, DaemonIsLinux)
Copy link
Member

Choose a reason for hiding this comment

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

You can chain the testRequires :

testRequires(c, DaemonIsLinux, swapMemorySupport)

@clnperez clnperez force-pushed the test-inspect-warn-fix branch from 1437c92 to 1ea1a48 Compare October 13, 2015 21:14
@clnperez
Copy link
Contributor Author

oops! thanks @vdemeester.

@clnperez clnperez force-pushed the test-inspect-warn-fix branch 3 times, most recently from ad348c2 to 8eca38f Compare October 13, 2015 22:50
@clnperez
Copy link
Contributor Author

Checks passed. @vdemeester and @cpuguy83, PTAL.

vdemeester
vdemeester reviewed Oct 14, 2015
View reviewed changes
integration-cli/docker_cli_inspect_unix_test.go
func (s *DockerSuite) TestInspectInt64(c *check.C) {
testRequires(c, DaemonIsLinux, swapMemorySupport)

out, _, err := dockerCmdWithError("run", "-d", "-m=300M", "busybox", "true")
Copy link
Member

Choose a reason for hiding this comment

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

You can use dockerCmd here instead too.

out, _ := dockerCmd("run", "-d", "-m=300M", "busybox", "true")

@clnperez
Copy link
Contributor Author

@vdemeester, sure. I just copy-pasted this over, but more than happy to improve. I'll wait a bit before resubmitting to see if anyone has other suggestions.

@clnperez
Copy link
Contributor Author

Actually, don't I want to keep the error so it can be checked?

@vdemeester
Copy link
Member

@clnperez dockerCmd actually check the error (see https://github.com/docker/docker/blob/master/pkg/integration/dockerCmd_utils.go#L35) 😉

@clnperez
Copy link
Contributor Author

Ooo. Reading the comments there, I do see that dockerCmdWithError is for commands that are supposed to fail. So, good change to make for sure.

@clnperez clnperez force-pushed the test-inspect-warn-fix branch 2 times, most recently from 07c920d to c55c2a7 Compare October 14, 2015 18:15
@duglin duglin mentioned this pull request Oct 15, 2015
Closed
@duglin
Copy link
Contributor

duglin commented Oct 15, 2015

@clnperez please see #17045 as I think that might be a better fix. I'm not sure what's going on, but on my system I get the warning and the test passes (when I split stdout/stderr) so I'd prefer if we didn't skip the test.

@clnperez
Copy link
Contributor Author

@duglin, agreed that we should still run this test, since it does just test a field. I'll update this. Thanks!

@clnperez clnperez force-pushed the test-inspect-warn-fix branch 2 times, most recently from ba43284 to 8031a8a Compare October 19, 2015 13:54
@clnperez
Fix TestInspectInt64 for run scenarios that return warnings
ae29bd4 
Instead of returning only the container ID, starting a container may
also return a warning:

"WARNING: Your kernel does not support swap
limit capabilities, memory limited without
swap.\nff6ebd9f7a8d035d17bb9a61eb9d3f0a5d563160cc43471a9d7ac9f71945d061"

The test assumes that only the container ID is returned and uses the
entire message as the name for the inspect command. To avoid the need to
parse the container ID from the output after the run command, give the
container a name and use that instead.

Signed-off-by: Christy Perez <christy@linux.vnet.ibm.com>
@clnperez clnperez force-pushed the test-inspect-warn-fix branch from 8031a8a to ae29bd4 Compare October 19, 2015 13:57
@clnperez
Copy link
Contributor Author

@duglin, @vdemeester, @cpuguy83 -- new patch for the same issue. PTAL. Thanks!

@duglin
Copy link
Contributor

duglin commented Oct 19, 2015

test works for me! And its much cleaner :-)
LGTM

@cpuguy83
Copy link
Member

lol, nice LGTM :)
Sorry about having you go back and forth on that one.

cpuguy83 added a commit that referenced this pull request Oct 19, 2015
@cpuguy83
Merge pull request #16986 from clnperez/test-inspect-warn-fix
d0b4e7a 
Fix TestInspectInt64 for kernels without swap limit capability
@cpuguy83 cpuguy83 merged commit d0b4e7a into moby:master Oct 19, 2015
@clnperez
Copy link
Contributor Author

Thanks all! And np, @cpuguy83. I should have paid more attention!

@clnperez clnperez deleted the test-inspect-warn-fix branch October 19, 2015 16:18
@tiborvass tiborvass added this to the 1.9.0 milestone Oct 21, 2015
@tiborvass tiborvass assigned tiborvass and unassigned tiborvass Oct 21, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
status/2-code-review
Projects
None yet
Milestone
1.9.0
Development

Successfully merging this pull request may close these issues.
7 participants
@clnperez @cpuguy83 @thaJeztah @vdemeester @duglin @tiborvass @GordonTheTurtle