docker / app

Make your Docker Compose applications reusable, and share them on Docker Hub
Apache License 2.0
1.57k stars 177 forks source link

Fix coverage step #465

Closed silvin-lubecki closed 5 years ago

silvin-lubecki commented 5 years ago

The coverage (and so the unit tests) was running as detached, so if one test fails the target won't fail because of it, but just because it can't find the coverage files.

Fixed this running the tests in attached mode + renaming Coverage step in the jenkinsfile to a more obvious step Unit Coverage.

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

ijc commented 5 years ago

docker run -v /var/run:/var/run:ro --name docker-app-cov-jenkins-docker-app-PR-465-1-coverage --network="host" -ti docker-app-dev:jenkins-docker-app-PR-465-1-coverage make COMMIT=db96f39 TAG=jenkins-docker-app-PR-465-1-coverage EXPERIMENTAL=off coverage the input device is not a TTY docker.Makefile:91: recipe for target 'coverage' failed

I suppose is the reason it was like this. I reckon you should remove the -i as well as the -d.

If we have tests which require stdio to be a tty then a) I'd be surprised and b) those tests are IMHO broken.

codecov[bot] commented 5 years ago

Codecov Report

Merging #465 into master will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #465   +/-   ##
=======================================
  Coverage   61.43%   61.43%           
=======================================
  Files          52       52           
  Lines        2754     2754           
=======================================
  Hits         1692     1692           
  Misses        851      851           
  Partials      211      211

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 c1d0527...4329e70. Read the comment docs.

silvin-lubecki commented 5 years ago

PTAL @ijc @chris-crone