deis / builder

Git server and application builder for Deis Workflow
https://deis.com
MIT License
40 stars 44 forks source link

tests(pkg): beef up unit tests #437

Closed bacongobbler closed 7 years ago

bacongobbler commented 7 years ago

Using this PR along with codecov to display how much of the codebase is covered as I go along.

closes #436

deis-bot commented 7 years ago

@kmala, @arschles and @Joshua-Anderson are potential reviewers of this pull request based on my analysis of git blame information. Thanks @bacongobbler!

bacongobbler commented 7 years ago

relies on https://github.com/deis/pkg/pull/27 being merged

bacongobbler commented 7 years ago

I think that's going to be the end of the 6-day hiatus on bringing up test coverage. Pretty sure we're above 55% right now (an increase over ~10% test coverage than before), but I'm not sure we'll ever be able to hit anything higher than that without some serious refactoring, and the intention here is to add tests to existing infrastructure rather than remove it ;)

bacongobbler commented 7 years ago

FYI codecov is reporting on old commits. Let me see if I can fix it or come up with a better coverage report than what codecov is giving us.

bacongobbler commented 7 years ago

Here's where we stand in comparison to master as shown in #436:

?       github.com/deis/builder/pkg [no test files]
ok      github.com/deis/builder/pkg/cleaner 0.011s  coverage: 62.2% of statements
ok      github.com/deis/builder/pkg/conf    0.004s  coverage: 89.3% of statements
ok      github.com/deis/builder/pkg/controller  0.006s  coverage: 100.0% of statements
ok      github.com/deis/builder/pkg/git 0.004s  coverage: 15.4% of statements
ok      github.com/deis/builder/pkg/gitreceive  0.019s  coverage: 50.1% of statements
ok      github.com/deis/builder/pkg/healthsrv   0.012s  coverage: 79.7% of statements
?       github.com/deis/builder/pkg/k8s [no test files]
ok      github.com/deis/builder/pkg/sshd    1.868s  coverage: 68.5% of statements
ok      github.com/deis/builder/pkg/storage 0.006s  coverage: 66.7% of statements
ok      github.com/deis/builder/pkg/sys 0.003s  coverage: 100.0% of statements
?       github.com/deis/builder [no test files]

TL;DR 2 packages that had 0% coverage now have 100% test coverage, pkg/gitreceive (our biggest package) saw a 13% increase in test coverage, everything else was bumped up to at least 60% test coverage other than pkg/git and pkg/k8s. Asides pkg/controller and pkg/sys, pkg/conf saw the biggest improvement overall with a 44% increase in code coverage.

codecov-io commented 7 years ago

Current coverage is 56.82% (diff: 80.95%)

Merging #437 into master will increase coverage by 9.73%

@@             master       #437   diff @@
==========================================
  Files            27         29     +2   
  Lines          1134       1165    +31   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits            534        662   +128   
+ Misses          568        467   -101   
- Partials         32         36     +4   

Powered by Codecov. Last update d62f9a2...d4373c3

bacongobbler commented 7 years ago

ping @sgoings, do you think the existing test coverage is good enough to satisfy the current target or would you like to see a larger refactor of the codebase?

I've been trying to limit myself to just writing tests against what exists today rather than refactor legacy code and unintentionally break something. I don't think we're going to see any more gains in builder unless we start mocking the controller, kubernetes and s3 interactions, which means we'd be missing the original intent of tacking on unit tests to the existing infrastructure.

mboersma commented 7 years ago

I've been trying to limit myself to just writing tests against what exists today rather than refactor legacy code

This is awesome IMHO, and it makes the coverage much better. We should revisit in the future to see if refactoring can allow more unit tests, but I think this is good enough to claim victory for this current dev cycle.

Although in the interests of full disclosure, your easter egg went right over my head. 😃

bacongobbler commented 7 years ago

Although in the interests of full disclosure, your easter egg went right over my head. :smiley:

It's the sha of the very first commit to deis/deis! :)

https://github.com/deis/deis/commit/0462cef5812ce31fe12f25596ff68dc614c708af

mboersma commented 7 years ago

It's the sha of the very first commit to deis/deis

"Chef-based application fabric." Lol! Good times.