Improve runner unit tests#1398
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1398 +/- ##
==========================================
- Coverage 44.69% 44.65% -0.05%
==========================================
Files 109 111 +2
Lines 4546 4546
==========================================
- Hits 2032 2030 -2
- Misses 2308 2311 +3
+ Partials 206 205 -1
Continue to review full report at Codecov.
|
dgageot
force-pushed
the
improve-runner-unit-tests
branch
3 times, most recently
from
511c857 to
767786c
Compare
|
ping @balopat This is the first iteration on fixing/adding tests on the Runner. I have more to come to test the sync. There's a lot of issues with this piece of code |
| // Stop stops the logger. | ||
| func (a *LogAggregator) Stop() { | ||
| a.cancel() | ||
| if a.cancel != nil { |
There was a problem hiding this comment.
now that we're touching this: a.cancel can be nil when it's not yet started, right?
Also, what happens when we call Stop() twice? I.e. the cancelFunc called twice?
What if instead of nil the "default" value of the cancel is a "noop" func, i.e cancel: func() {} in NewLogAggregator - and then still, Stopwill have to remember to set it back to func() {} - this way we don't have to check for nil.
Alternatively we can keep it as it is and have a.cancel=nil represent the Stopped state - but then we have to set a.cancel = nil here. WDYT?
There was a problem hiding this comment.
Let me handle that in another PR. Current state is that cancel can be called multiple time, which is a noop passed the first time.
pkg/skaffold/runner/dev.go
Outdated
| var ErrorConfigurationChanged = errors.New("configuration changed") | ||
|
|
||
| // Dev watches for changes and runs the skaffold build and deploy | ||
| // pipeline until interrrupted by the user. |
There was a problem hiding this comment.
| // pipeline until interrrupted by the user. | |
| // pipeline until interrupted by the user. |
balopat
left a comment
balopat
left a comment
There was a problem hiding this comment.
LGTM with a small comment on logaggregator.cancel!
Thank you for breaking it up, it looks better already!
Signed-off-by: David Gageot <david@gageot.net>
Signed-off-by: David Gageot <david@gageot.net>
Signed-off-by: David Gageot <david@gageot.net>
Signed-off-by: David Gageot <david@gageot.net>
dgageot
force-pushed
the
improve-runner-unit-tests
branch
from
767786c to
996c07f
Compare
3 participants
Test were broken and wouldn't test what we thought they'd test.