-
-
Notifications
You must be signed in to change notification settings - Fork 161
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
Use Docker for isolated testing #353
Conversation
Looks good to me, but I only scanned over your changes! Will have a deeper look into it the next couple of days - maybe even tomorrow. Thanks for the helpful explanations! First thoughts: Maybe we could add some parameter to the (non)existing |
I have no idea what you mean by bin/test-script. The current Ideally, I'd get Aruba using a real TMP directory for testing, like /tmp. It would be great if the source directory could be read-only, because then you could paralellize tests using multiple containers. Of course this only makes sense locally. There are lots of improvements possible here, but the above is worthwhile standalone as-is. E.g. it should be easier to select JRuby, although it isn't a bad idea to just change that in the Also, the "local docker" script also supports parameters (see the changes in Contributing.md file), so they're interchangable. I wouldn't add parameters, because it would make running commands more confusing and error problem. (I use args for shelling to a bash login session already). |
Mostly, this makes local tests green - with no other changes to the codebase. So at the very least it will help contributors working on both 1.x and pre-1.x codebases - by making their tests green regardless of setup. |
I put together another You may want to have a look at this commit. Work done here is not finished yet: Working with proxies and gnupg is quite a hassle. Maybe I find some time tonight to finish this. Feel free to take over whatever you want. |
Including all rubies will actually make the image huge, so it defeats the purpose. Also the base image should allow further "development". Just like alpine is the base here, it's best to have an "aruba testing environment" as the base, and installing requirements (rubies, gems, pythong, jdk, extra tools) as part of "bootstrapping", which shouldn't happen in the actual Docker image. Also, RVM doesn't support Alpine, so this is a deal-breaker. The idea is to prioritize developer satisfaction over file size. Alpine is based on busybox, so installing anything else is pretty much like slitting your wrists. It means tens of hours (if not hundreds) just trying to get a tool to install there. That's why I picked Ubuntu - either every tool you'd ever want (strace, gems with compiled extensions, editors, profiling, etc.) is installable, or Docker workarounds are well known. Just to give you a sense of the workload: if you expect RVM to work flawlessly with Alpine before there's a possibility to get docker support in testing Aruba ... then my use_gemset PR will have to wait another year until RVM catches up to using Alpine. Also, Ubuntu isn't huge at all: 187MB (Alpine: 5MB to start, but 131MB without RVM yet). My aruba_tests image is currently 700MB, but ...
So the Aruba-specific part is already almost 300 MB (!). Next:
So that's almost 600 MB for a 800MB image! Which adds, up, because the Ubuntu image is 180MB. Relatively "tiny" by comparison. And it's very likely that anyone with a Docker installation ALREADY has the Ubuntu images available locally. The idea is also not to replicate Travis, but to allow the developer flexibility to do what Travis can't do, which include:
If someone really wants to save space or one-time bandwidth ... then just using Travis is a better alternative. Also, the more flexibility, the better - because it doesn't make sense to add "Dockerfile maintenance" to the list of things to do in Aruba. So my suggestion: unless there's something horribly wrong with this PR, it would be great if you can just merge it. (Other people can add pull requests later). The biggest "optimization" for now is: having an official base image uploaded (so Aruba doesn't have to rely on my own - though, I wouldn't mind if it did). The reasons:
Also, docker-compose is simply more convenient for developing and tweaking. (Because it's "smart", unlike running docker commands directly). Especially since this is a new feature, it's detrimental to try and make it "perfect" on the first shot. (Meanwhile, tests are still failing locally without this PR being merged - so "optimizing" the Dockerfile is absolutely not a priority at this point). A 500MB base image based on Ubuntu means only 300MB more if Ubuntu image is already present (and likely will be). This is cheap to download - and still much faster than waiting for Travis. Even an Aruba git checkout can take a bit. Here's what I took into account when creating this image:
Also, some tests can detect Docker and just clobber whatever they want without any regards for safety. E.g. Removing home directory? Not an issue. Files created in wrong directory? Not an issue. The worst case scenario is losing uncommited changes in sources. |
Out of curiosity I checked out: https://github.com/nicdoye/alpine-rvm-gcc (It shows some of the wrist-slitting necessary to get things done). Result of running it: 570MB image. (RVM install alone took a whopping 430MB - probably because everything has to compiled for Alpine, since nothing is binarily compatible with it. Conclusion: Alpine is great for routers and microservices ... but ill-suited for any kind of development. |
@@ -36,3 +36,4 @@ cucumber-pro.log | |||
|
|||
# Temp files of yar | |||
.yardoc | |||
docker.gemrc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't make sense. I would like to be able to build the image locally and this file is required by `Dockerfile.
What I suggest:
Create a directory .docker/latest
(latest -> docker tag) and place the file there. This is practise I normally use to have files for each tag available.
Thanks for your work and comments. They are quite helpful. I now think using ubuntu is a good choice. Plus: Have a look at the news there's some partnership between canonical and microsoft which will result in running ubuntu natively on windows. Please address my comments and I'm going to merge your PR. |
@@ -0,0 +1,5 @@ | |||
tests: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need docker compose for that? This add another dependency for new developers. This should be simple enough to just have a rake task for that. Feel free to modify the task I added to master recently.
88f662e
to
4c82b46
Compare
I've reworked things. This is still a work in progress.
Issues:
|
Two more things I didn't address maybe:
|
# 2. Less internet bandwidth used | ||
# 3. Lesser load on rubygems.org servers | ||
# | ||
# Info: http://guides.rubygems.org/run-your-own-gem-server/# |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'm using some derivative of geminabox right now. (as a Docker micro-service, of course).
328dd5a
to
84c9133
Compare
|
||
STDOUT.puts "Running Docker with arguments:" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
STDOUT is required? STDOUT.puts should be the same as puts.
@e2 Can you rebase I changed quite a lot in Regarding the Mmmh. Also I see the good things docker compose brings in, I'm not fully convinced since it's an extra dependency. For now I would like to leave it away. Regarding the dockerfiles: Rest of changes: |
I'd rather convince you. "rake people" wont care about docker - unless they're getting failures. So they don't care. Meanwhile, "docker people" will only benefit from docker-compose. Thirdly, docker-compose helps Windows users get started - docker-compose means a lot less cryptic errors for people who don't get docker much. Also, editing a docker-compose.yml is much more intuitive than tampering with a Rakefile. Ideally, I'd say the Rakefile should process the docker-compose.yml to get variables. Also, the Rakefile isn't too helpful in providing custom options to docker. Meanwhile, the docker-compose.yml is very well documented. The reason for splitting files in two is: the base one is intended to be an official, downloadable image to speed up preparing a checkout for testing - because the download should be a lot faster that building that base image from scratch. Also, once there's a official base image, the whole Your argument about "simplicity" could be applied to docker-compose - way simpler than docker or tweaking the Rakefile. And it isn't a huge dependency in size, either. Installing is trivial too. Docker-compose isn't a default/"forced" dependency here. To me it would be like having a .rubocop.yml file without Rubocop gem dependency in development. In 2016 when human time is more precious than ever, installing docker compose and tweaking the docker-compose.yml will always take less time than tweaking the Rakefile and potentially making mistakes (which I've done myself). Not to mention: passing arguments to a Rake take is the most shocking surprise ever... |
I'll rebase when I get back home. |
84c9133
to
549d6cf
Compare
e519335
to
73046cc
Compare
Changes:
|
Note: I haven't tested the Here's the "intended" workflow overview for clarity:
|
73046cc
to
5480872
Compare
Thanks @e2 I'm going to merge this, but I will merge the Docker*.files into one. The "testing.md" is not part of this PR, correct? |
I added the TESTING.md file (happened probably because it isn't needed for tests/specs to work).
It may be seem "less complex", I get that. But, if you make the base image public, you can just remove the Dockerfile.base altogether. (It will be available there anyway). Just like you don't need to build the So if you create a The only reason I didn't do this myself is: I don't have access to create and publish an "official" docker image for the base. That's the ONLY reason. |
(Partly) incorporated by #382. |
Summary
Allows locally running test in a fully-provisioned Docker container.
Details
Added
script/run_tests_in_docker
which should pretty much do everything out of the box, including:Also, I updated the bootstrap script:
Motivation and Context
This mostly avoids any and every issue caused by custom environments on a typical desktop. (E.g. locale, shell configuration, app versions, missing requirements before tests start, failures on travis, multiple Ruby versions, etc).
And it's just as fast as on the host, so it makes little sense not to use it. (Except maybe disk space on laptops with tiny SSD drives).
This is why I marked using Docker for tests as "recommended" in CONTRIBUTING.md.
There's no downside, especially since now Windows users can quickly tests their development tree in a Docker container (via a VM, but still much faster and convenient that through a PR+Travis).
(It may even be possible to transparently run tests in Windows - e.g. run a silent install of Ruby inside a Wine docker image - or maybe even on Windows containers, but that's beyond my interests).
How Has This Been Tested?
Should work on any Linux system with Docker and Docker Compose installed.
Environment should more or less resemble that of Travis. If not, any future differences can be easily accounted for in the Dockerfile.
Types of changes
Checklist: