Add docker tests, updated travis CI to use docker #715
Conversation
|
Any news on this PR? I'd be happy to help if any adjustments might be required to get it into master. |
1 similar comment
|
Any news on this PR? I'd be happy to help if any adjustments might be required to get it into master. |
|
When you say "run the tests successfully" did you see a failure with the existing setup? I'm curious about the motivation to dockerize the travis setup. |
|
|
||
| script: | ||
| - invoke test | ||
| - make test | ||
|
|
||
| env: |
There was a problem hiding this comment.
I believe the env vars below encode a github key that avoids the tests hitting the rate limit of anonymous calls to the github api. These would need to be forwarded into the container for the tests to use.
There was a problem hiding this comment.
it captures it from the local environment, rather than having it passed in
Dockerfile.tests
Outdated
| @@ -0,0 +1,9 @@ | |||
| FROM nbviewer_nbviewer | |||
There was a problem hiding this comment.
Instead of relying on the default image name here chosen by docker-compose, it might be better to specify an image name explicitly in the compose YAML and use it here.
There was a problem hiding this comment.
Yes, that is probably a good idea. I'll look into that shortly.
|
|
||
| test: build | ||
| docker build -t nbviewer_test -f Dockerfile.tests . | ||
| docker run -e GITHUB_API_TOKEN nbviewer_test invoke test |
There was a problem hiding this comment.
Does this successfully pass the token from the travis environment into the container?
There was a problem hiding this comment.
It passes any GITHUB_API_TOKEN that is in the outside environment into the container, yes. It does appear to work with the encrypted token set up currently on github.
|
It's been a little while, but I believe my problem was that while I could use docker to run the viewer without having to install any depedencies on my local machine, I had to do so to run the tests, at least as it was suggested in the README. |
|
Thanks for your assistance @parente! I have updated this PR to include an explicit name for the nbviewer image in the docker-compose.yml, and base the testing image on that. I've also had to add a fix for #730 to make it run in travis as less 3.0.0 is still broken. I you merge this PR, you can close that issue, but you may want to keep an eye on less, to see if the pinning can be removed at some point in the future when they fix less/less.js#3116. |
|
Hi @parente, I'd love to get this into master. Anything I can do to move this forward? |
|
Sorry for not seeing this. While I think it's a good idea to document / allow running the tests in a docker container, I don't think this is what we should do on Travis. We lose the Python test matrix, and add an extra layer of indirection. |
|
After running into a Python 2 specific issue in a recent PR, I agree with @minrk that the Travis build matrix of multiple versions has value over a dockerized, single version build and test. |
This PR has added a testing docker image that can run the test cases successfully, and has updated the README and the travis.yml use the docker-based testing image.
I'm happy to modify this PR as needed to help it get to master.
Thanks!