Increase test coverage(#6) #22

pratyushprakash · 2019-10-17T17:13:04Z

Refactor Run to take in ReaperContext
Refactor podreaper command line to build a ReaperContext

CLAassistant · 2019-10-17T17:13:17Z

All committers have signed the CLA.

codecov · 2019-10-17T17:15:46Z

Codecov Report

Merging #22 into master will increase coverage by 2.03%.
The diff coverage is 100%.

@@            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

Impacted Files	Coverage Δ
pkg/reaper/podreaper/podreaper.go	`76.66% <100%> (+14.26%)`	⬆️

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 ca05516...52fc249. Read the comment docs.

tekenstam · 2019-10-17T19:22:00Z

Thanks @pratyushprakash! Related to #6.

eytan-avisror · 2019-10-17T19:26:42Z

Hi @pratyushprakash
Thanks.
I don't really understand the changes you are suggesting.
Can you provide an explanation on why we should change this?
Essentially it seems you moved some of the mocking code from the test files to the actual pod_reaper code.. how does this improve the coverage?

eytan-avisror

If the goal is to make Run() testable, we should instantiate the clients at a higher level and pass in an interface to Run() (or a struct of interfaces)
Then in the test code we can simply pass fake/mock clients instead of real ones

eytan-avisror · 2019-10-17T20:05:29Z

pkg/reaper/podreaper/podreaper.go

@@ -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 {


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

The problem here is that a Reaper Context is built in the Run function. Could we maybe pass the context in its entirety instead?

@eytan-avisror

I agree that we should keep mocks only in testing. How would we bypass validateArguments in this scenario.

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.

eytan-avisror · 2019-10-17T20:06:04Z

pkg/reaper/podreaper/podreaper.go

@@ -41,6 +44,11 @@ func Run(args *Args) error {
 		return err
 	}

+	if len(fakePods) > 0 && args.K8sConfigPath == "fake" {


Same for this, all mocking/faking should happen within the test code.

eytan-avisror · 2019-10-17T20:06:27Z

pkg/reaper/podreaper/podreaper.go

@@ -217,3 +225,81 @@ func (ctx *ReaperContext) validateArguments(args *Args) error {

 	return nil
 }
+
+func loadFakeAPI(ctx *ReaperContext) {


Why do we need this moved to podreaper.go it's the same package, we can keep this code under podreaper_test.go

eytan-avisror · 2019-10-17T20:06:38Z

pkg/reaper/podreaper/podreaper.go

+	}
+}
+
+func createFakePods(pods []FakePod, ctx *ReaperContext) {


Why do we need this moved to podreaper.go it's the same package, we can keep this code under podreaper_test.go

eytan-avisror · 2019-10-17T20:07:02Z

pkg/reaper/podreaper/types.go

@@ -40,3 +42,15 @@ type Args struct {
 	SoftReap      bool
 	DryRun        bool
 }
+
+// FakePod is the pod struct used for testing
+type FakePod struct {


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 · 2019-10-24T04:01:42Z

@eytan-avisror Could you take a look at this again?

eytan-avisror · 2019-10-29T17:00:07Z

Thanks @pratyushprakash this change looks much better.
I will do some tests and approve shortly if no issues

pratyushprakash requested a review from a team as a code owner October 17, 2019 17:13

eytan-avisror requested changes Oct 17, 2019

View reviewed changes

Increase test coverage for podreaper (keikoproj#6)

1710590

* 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 October 24, 2019 03:57

Merge branch 'master' into issue6

52fc249

eytan-avisror approved these changes Oct 29, 2019

View reviewed changes

eytan-avisror merged commit 0b5d58b into keikoproj:master Oct 29, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Increase test coverage(#6) #22

Increase test coverage(#6) #22

pratyushprakash commented Oct 17, 2019 •

edited

Loading

CLAassistant commented Oct 17, 2019 •

edited

Loading

codecov bot commented Oct 17, 2019 •

edited

Loading

tekenstam commented Oct 17, 2019 •

edited

Loading

eytan-avisror commented Oct 17, 2019

eytan-avisror left a comment

eytan-avisror Oct 17, 2019

pratyushprakash Oct 18, 2019

pratyushprakash Oct 18, 2019

pratyushprakash Oct 18, 2019

eytan-avisror Oct 18, 2019

eytan-avisror Oct 17, 2019

pratyushprakash Oct 18, 2019

eytan-avisror Oct 17, 2019

eytan-avisror Oct 17, 2019

eytan-avisror Oct 17, 2019

pratyushprakash commented Oct 24, 2019

eytan-avisror commented Oct 29, 2019

Increase test coverage(#6) #22

Increase test coverage(#6) #22

Conversation

pratyushprakash commented Oct 17, 2019 • edited Loading

CLAassistant commented Oct 17, 2019 • edited Loading

codecov bot commented Oct 17, 2019 • edited Loading

Codecov Report

tekenstam commented Oct 17, 2019 • edited Loading

eytan-avisror commented Oct 17, 2019

eytan-avisror left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pratyushprakash commented Oct 24, 2019

eytan-avisror commented Oct 29, 2019

pratyushprakash commented Oct 17, 2019 •

edited

Loading

CLAassistant commented Oct 17, 2019 •

edited

Loading

codecov bot commented Oct 17, 2019 •

edited

Loading

tekenstam commented Oct 17, 2019 •

edited

Loading