Increase test coverage(#6) #22
Conversation
|
|
Codecov Report
@@ Coverage Diff @@
## master #22 +/- ##
==========================================
+ Coverage 53.38% 55.42% +2.03%
==========================================
Files 4 4
Lines 826 821 -5
==========================================
+ Hits 441 455 +14
+ Misses 338 317 -21
- Partials 47 49 +2
Continue to review full report at Codecov.
|
|
Thanks @pratyushprakash! Related to #6. |
|
Hi @pratyushprakash |
pkg/reaper/podreaper/podreaper.go
Outdated
| @@ -29,7 +32,7 @@ import ( | |||
| var log = logrus.New() | |||
|
|
|||
| // Run is the main runner function for pod-reaper, and will initialize and start the pod-reaper | |||
| func Run(args *Args) error { | |||
| func Run(args *Args, fakePods ...[]FakePod) error { | |||
There was a problem hiding this comment.
We should not make this change, adding faking at this level is not a correct refactor in my opinion.
Run should simply take in kubernetes.Interface and a fake client could be passed in at a higher level
There was a problem hiding this comment.
The problem here is that a Reaper Context is built in the Run function. Could we maybe pass the context in its entirety instead?
There was a problem hiding this comment.
There was a problem hiding this comment.
I agree that we should keep mocks only in testing. How would we bypass validateArguments in this scenario.
There was a problem hiding this comment.
I think a refactor should move both the validate and client instantiation out to the cli package that runs.
We can then pass some struct like ReaperContext into Run() -> however, this is a big change and should probably be part a separate PR.
If you want to do this, we can discuss further about how to re-design this.. but essentially you can look at the design of https://github.com/keikoproj/lifecycle-manager/blob/master/cmd/serve.go vs. https://github.com/keikoproj/lifecycle-manager/blob/master/pkg/service/server.go#L33 - in this project we do exactly that for testability and it just makes testing much better as far as clients/mocking/faking goes.
pkg/reaper/podreaper/podreaper.go
Outdated
| @@ -41,6 +44,11 @@ func Run(args *Args) error { | |||
| return err | |||
| } | |||
|
|
|||
| if len(fakePods) > 0 && args.K8sConfigPath == "fake" { | |||
There was a problem hiding this comment.
Same for this, all mocking/faking should happen within the test code.
pkg/reaper/podreaper/podreaper.go
Outdated
| @@ -217,3 +225,81 @@ func (ctx *ReaperContext) validateArguments(args *Args) error { | |||
|
|
|||
| return nil | |||
| } | |||
|
|
|||
| func loadFakeAPI(ctx *ReaperContext) { | |||
There was a problem hiding this comment.
Why do we need this moved to podreaper.go it's the same package, we can keep this code under podreaper_test.go
pkg/reaper/podreaper/podreaper.go
Outdated
| } | ||
| } | ||
|
|
||
| func createFakePods(pods []FakePod, ctx *ReaperContext) { |
There was a problem hiding this comment.
Why do we need this moved to podreaper.go it's the same package, we can keep this code under podreaper_test.go
pkg/reaper/podreaper/types.go
Outdated
| @@ -40,3 +42,15 @@ type Args struct { | |||
| SoftReap bool | |||
| DryRun bool | |||
| } | |||
|
|
|||
| // FakePod is the pod struct used for testing | |||
| type FakePod struct { | |||
There was a problem hiding this comment.
Why do we need this moved to types.go it's the same package, we can keep this code under podreaper_test.go
* Refactor `Run` to take in `ReaperContext` * Refactor podreaper command line to build a `ReaperContext`
pratyushprakash
force-pushed
the
issue6
branch
from
a2c0d38
to
1710590
Compare
|
@eytan-avisror Could you take a look at this again? |
|
Thanks @pratyushprakash this change looks much better. |
Refactor
Runto take inReaperContextRefactor podreaper command line to build a
ReaperContext