deis / controller

Deis Workflow Controller (API)
https://deis.com
MIT License
41 stars 53 forks source link

feat(Makefile,rootfs): run unit and style tests in container #1271

Closed mboersma closed 7 years ago

mboersma commented 7 years ago

Coerces make test-style and make test-unit to run in containers, which avoids the need to have Python 3.5 and the packages in dev_requirements.txt on your local workstation. Also updates the fork of requests-mock to that in the deis org.

Closes #18. Closes #1049.

See also deis/docker-python-dev#11 and deis/dockerbuilder#120. If that approach is acceptable, this could be refactored similarly for make test-style at least.

deis-bot commented 7 years ago

@bacongobbler, @seanknox and @krancour are potential reviewers of this pull request based on my analysis of git blame information. Thanks @mboersma!

vdice commented 7 years ago

Excited for this!

Should I be able to now run make test right off the bat w/o performing any of the classic, python-related setup? I'm seeing:

docker run -v `pwd`/rootfs:/app quay.io/vdice/controller:git-b39ee95.test /app/bin/test-style
docker run --rm -v /Users/vaughndice/work/go/src/github.com/deis/controller:/workdir -w /workdir quay.io/deis/shell-dev shellcheck rootfs/bin/boot rootfs/bin/reload rootfs/bin/test-style rootfs/bin/test-unit  _scripts/deploy.sh
cd rootfs && python manage.py check
Traceback (most recent call last):
  File "manage.py", line 10, in <module>
    from django.core.management import execute_from_command_line
ModuleNotFoundError: No module named 'django'
make: *** [test-check] Error 1

Full output here

bacongobbler commented 7 years ago

Not trying to poopoo on this PR, but have you considered using docker-compose to run the unit tests instead of bundling everything inside one big container, or does that miss the mark in some form?

I am also excited about this :)

mboersma commented 7 years ago

have you considered using docker-compose

I did actually, but the impetus for this was to avoid installing tools and libraries locally. I initially tried to rebase and polish up #1049 but it was too rusty and had some unrelated changes in it.

It's a bit hacky, and the biggest downside I can think of is that Dockerfile and Dockerfile.test could diverge if we aren't paying attention. Also it would be nice to solve this in a general way using our [python-dev]() image, but that was turning out to be a bit painful, most of the changes end up being specific to controller rather than common, and this is the only Python component that has unit tests.

mboersma commented 7 years ago

Should I be able to now run make test right off the bat...?

Yes, except I hadn't cleaned up that target yet, only make test-style and make test-unit. Should be good now.

bacongobbler commented 7 years ago

but the impetus for this was to avoid installing tools and libraries locally.

Very good point. I guess that just would bring us back to the root issue: hacking on the controller is significantly different than any other Workflow component and requires very specific tooling unique from the rest of the platform (python, python-dev, postgres-dev, libyaml etc.). Everything else is glide and go test.

As far as Dockerfile.test changes go, we can improve it over time. This should get us through the door. Thanks for indulging!

seanknox commented 7 years ago

very much dig this! runs goodly for me on a fresh repo clone after the make test fix.

mboersma commented 7 years ago

I guess that just would bring us back to the root issue

Maybe, unless we want to require docker-compose, which doesn't seem too onerous. This isn't blocking #1243, strictly speaking, so I'm glad to take a different approach if that's the consensus.

mboersma commented 7 years ago

Looks like codecov.io hasn't reported on this PR. Maybe we broke that? I will check it out...

bacongobbler commented 7 years ago

not a moment too soon... python-ldap can't be compiled on Windows and they don't provide a compiled wheel for python 3.5. I'll give this a spin.

mboersma commented 7 years ago

I fixed the issue with codecov.io reporting--it should be commenting here, he said hopefully.

Update: sigh. Looks like everything is working, but codecov delivers an empty report to codecov.io for some reason.

mboersma commented 7 years ago

Coverage reporting on this PR seems to work now at codecov.io.

codecov-io commented 7 years ago

Codecov Report

Merging #1271 into master will increase coverage by <.01%. The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1271      +/-   ##
==========================================
+ Coverage   86.65%   86.66%   +<.01%     
==========================================
  Files          45       45              
  Lines        3928     3929       +1     
  Branches      681      681              
==========================================
+ Hits         3404     3405       +1     
  Misses        355      355              
  Partials      169      169

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 90bd2c6...e8e6f58. Read the comment docs.