Stop scale down timer #70
Conversation
| @@ -136,6 +141,79 @@ func TestAddDeployment(t *testing.T) { | |||
| } | |||
| } | |||
|
|
|||
| func TestRemoveDeployment(t *testing.T) { | |||
There was a problem hiding this comment.
I would prefer that we write these tests utilizing as many of the exported methods as possible and written in a manner that follows expected usage. This allows the tests to be more flexible over time. One example: asserting on m.ResolveDeployment() as opposed to m.modelToDeployment. I also think this is a good case for not using table-driven tests. In this case a table driven design is really only reusing m.removeDeployment(req) across all of the tests but it becomes harder to read and covers less surface area per lines of test than a usage/story-driven approach.
There was a problem hiding this comment.
Thanks for the feedback! I can see the benefit of using method rather than messing with internal state. I also agree that the assert function is not great and values should be stored in the test spec. I like table test as a spec for the method. But I am so used to writing them, that I have a hard time to think how this test would look like in a classic fashion. Would you split it into multiple separate tests or work with a single manager and add/ remove state?
There was a problem hiding this comment.
I personally like the table tests as well but I like to limit them to data only and not logic. I think as a follow up we need to have tests for scaler.go and refactor it before we can fix #67 which is imo the highest priority issue right now.
If we think these tests on internal state are no longer productive, then we can simply remove the tests later as well. The issue with testing using internal state is that it makes refactoring the implementation harder later.
|
This won't fix #67 since in my case I never deleted a deployment yet still triggered the issue. So not sure if this patch is needed. That being said, I think this fix is helpful because otherwise the timers would just keep on going even after the scaler is deleted? This was an interesting read: https://medium.com/@oboturov/golang-time-after-is-not-garbage-collected-4cbc94740082 |
|
I will include this in #76 |
Fixes #73 and includes pr #70 and others from Alex * Remove compareScales function * Refactor SetDesiredScale that is simpler to understand by storing lastSuccessfulScale time * Improve test coverage for scaler.go Co-authored-by: Alex Peters <[email protected]>
See: #67
When a deployment is deleted on reconcile there may still be a scale down timer running that modifies k8s replica state later. With this PR, the timer is stopped.