8000 feat(Makefile,rootfs): run unit and style tests in container by mboersma · Pull Request #1271 · deis/controller · GitHub
[go: up one dir, main page]
Skip to content
This repository was archived by the owner on May 6, 2020. It is now read-only.

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

Merged
mboersma merged 2 commits intodeis:masterfrom
mboersma:test-in-docker
Apr 3, 2017
Merged

feat(Makefile,rootfs): run unit and style tests in container#1271
mboersma merged 2 commits intodeis:masterfrom
mboersma:test-in-docker

Conversation

@mboersma
Copy link
Member
@mboersma mboersma commented Mar 22, 2017 •
edited
Loading

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.

@mboersma mboersma added this to the v2.13 milestone Mar 22, 2017
@mboersma mboersma self-assigned this Mar 22, 2017
@deis-bot
Copy link
deis-bot commented Mar 22, 2017

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

@mboersma mboersma changed the title Test in docker feat(Makefile,rootfs): run unit and style tests in container Mar 22, 2017
@vdice
Copy link
Member
vdice commented Mar 22, 2017

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
Copy link
Member
bacongobbler commented Mar 22, 2017

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
Copy link
Member Author
mboersma commented Mar 22, 2017

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
Copy link
Member Author
mboersma commented Mar 22, 2017 •
edited
Loading

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
Copy link
Member
bacongobbler commented Mar 22, 2017 •
edited
Loading

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
Copy link
Member
seanknox commented Mar 22, 2017

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

@mboersma
Copy link
Member Author
mboersma commented Mar 22, 2017

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.

@vdice vdice mentioned this pull request Mar 22, 2017
Closed
4 tasks
@mboersma
Copy link
Member Author
mboersma commented Mar 22, 2017

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

@bacongobbler
Copy link
Member
bacongobbler commented Mar 23, 2017

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 mboersma force-pushed the test-in-docker branch 5 times, most recently from 2dde908 to ebbc3f8 Compare March 29, 2017 01:23
@mboersma
Copy link
Member Author
mboersma commented Mar 29, 2017 •
edited
Loading

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 mboersma force-pushed the test-in-docker branch 2 times, most recently from 8ab2e4e to 9a0b9ef Compare March 30, 2017 17:07
@mboersma mboersma mentioned this pull request Mar 30, 2017
Closed
@mboersma mboersma force-pushed the test-in-docker branch 2 times, most recently from 23ecbdd to 3333a72 Compare March 31, 2017 21:21
@mboersma
Copy link
Member Author
mboersma commented Mar 31, 2017

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

@codecov-io
Copy link
codecov-io commented Mar 31, 2017 •
edited
Loading

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.

@vdice vdice added the LGTM1 label Mar 31, 2017
@mboersma mboersma merged commit 0ef67fa into deis:master Apr 3, 2017
@mboersma mboersma deleted the test-in-docker branch April 3, 2017 17:51
@mboersma mboersma mentioned this pull request Apr 7, 2017
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@vdice vdice vdice approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@mboersma mboersma

Labels

enhancement LGTM1 LGTM2

Projects

None yet

Milestone

v2.13

Development

Successfully merging this pull request may close these issues.

6 participants

@mboersma @deis-bot @vdice @bacongobbler @seanknox @codecov-io
0