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

optimize slow tests #21151

Merged
merged 2 commits into from Mar 14, 2016
Merged

optimize slow tests #21151

merged 2 commits into from Mar 14, 2016

Conversation

mountkin
Copy link
Contributor

- What I did
Optimize 2 slow tests.

Before:

PASS: docker_cli_run_test.go:2751: DockerSuite.TestRunContainerWithReadonlyRootfs 16.561s
PASS: docker_cli_run_unix_test.go:910: DockerSuite.TestRunApparmorProcDirectory 19.075s

After:

PASS: docker_cli_run_test.go:2751: DockerSuite.TestRunContainerWithReadonlyRootfs   2.261s
PASS: docker_cli_run_unix_test.go:910: DockerSuite.TestRunApparmorProcDirectory 2.250s

- How I did it

  1. Merge multiple docker run command into one for TestRunContainerWithReadonlyRootfs
  2. Change the image from debian:jessie to busybox for TestRunApparmorProcDirectory

- How to verify it
make test-integration-cli

- A picture of a cute animal (not mandatory but encouraged)
1c767db5c9ea15ce0f140dbcb5003af33b87b25e

Signed-off-by: Shijiang Wei mountkin@gmail.com

@cyphar
Copy link
Contributor

cyphar commented Mar 12, 2016

I'm wondering whether this invalidates the test (since the old code was trying to make sure that all of them fail, but the current code only makes sure that at least one of them fails). For example, what happens if we have a regression with /sys becoming rw even with --read-only?

Maybe if you made it a for loop in the shell script then you could check the output for each file -- that should increase the speed without removing the validity of the test. Something like:

docker run --read-only sh -c 'for i in <list of files> { touch $i; echo "$? $i"; }'

Or similar (you could even use the error output from touch.

👍 on switching from debian:jessie to busybox. Maybe we should look through the tests for more occurrences of that.

Also, can you split your two commits into a commit for each test? Just so that it's easier to bisect.

@mountkin
Copy link
Contributor Author

I'm wondering whether this invalidates the test (since the old code was trying to make sure that all of them fail, but the current code only makes sure that at least one of them fails). For example, what happens if we have a regression with /sys becoming rw even with --read-only?

Oh it's really a problem.
Thanks for pointing out this @cyphar 👍
I'll update the code accordingly.

@mountkin mountkin force-pushed the optimize-test branch 2 times, most recently from 682e35c to e9d3ebd Compare March 12, 2016 10:53
Signed-off-by: Shijiang Wei <mountkin@gmail.com>
Signed-off-by: Shijiang Wei <mountkin@gmail.com>
@calavera
Copy link
Contributor

LGTM

1 similar comment
@cpuguy83
Copy link
Member

LGTM

cpuguy83 added a commit that referenced this pull request Mar 14, 2016
@cpuguy83 cpuguy83 merged commit 01f1651 into moby:master Mar 14, 2016
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.

None yet

6 participants