Fix missing configuration options panics #105
Conversation
|
It does NOT fixes #103 |
|
@janisz can you explain why? |
AlfredBroda
force-pushed
the
bugfix/use-mesosConfig-as-read-by-mesos-go
branch
from
b5e486c
to
e801906
Compare
Codecov Report
@@ Coverage Diff @@
## master #105 +/- ##
==========================================
+ Coverage 69.25% 73.28% +4.02%
==========================================
Files 34 34
Lines 1415 1430 +15
==========================================
+ Hits 980 1048 +68
+ Misses 363 307 -56
- Partials 72 75 +3
Continue to review full report at Codecov.
|
|
I do not see any tests added. |
cmd/executor/main_test.go
Outdated
|
|
||
| // then | ||
| assert.Error(t, err) | ||
| assert.Equal(t, "failed to re-establish subscription with agent within 1s, aborting", err.Error()) |
There was a problem hiding this comment.
How do we know that it will take exactly one second to produce error? In addition, why do we make an assertion for an error message? Changing the text is supposed to fail the tests?
There was a problem hiding this comment.
Ok, will remove this. The 1s in the error is the configured amount, so it does not change.
cmd/executor/main_test.go
Outdated
| "github.com/stretchr/testify/assert" | ||
| ) | ||
|
|
||
| func TestExecutorStartsWithoutCheckpointConfig(t *testing.T) { |
There was a problem hiding this comment.
TestIf will look more sensible.
cmd/executor/main_test.go
Outdated
| @@ -0,0 +1,28 @@ | |||
| package main | |||
There was a problem hiding this comment.
Add test case for no config.
There was a problem hiding this comment.
Actually, the case you mention covers this error too, so I will make the existing test more general (ie. test a no config situation).
cmd/executor/main.go
Outdated
| @@ -101,8 +101,34 @@ func readConfiguration(config interface{}) { | |||
| } | |||
| } | |||
|
|
|||
| func startExecutor(conf executor.Config) error { | |||
There was a problem hiding this comment.
Move this code to the executor package.
There was a problem hiding this comment.
This would create an import cycle. Unless the hook code stays here...
cmd/executor/main_test.go
Outdated
| err := startExecutor(conf) | ||
|
|
||
| // then | ||
| assert.Error(t, err) |
There was a problem hiding this comment.
I think we need also a NotPanics assertion here - we fixing bug that code is panicking when some config options are missing.
AlfredBroda
force-pushed
the
bugfix/use-mesosConfig-as-read-by-mesos-go
branch
2 times, most recently
from
444199b
to
9a06ada
Compare
AlfredBroda
changed the title
AlfredBroda
force-pushed
the
bugfix/use-mesosConfig-as-read-by-mesos-go
branch
from
9a06ada
to
34e58da
Compare
| @@ -382,6 +381,12 @@ func TestIfNotPanicsWhenKillWithoutLaunch(t *testing.T) { | |||
| }) | |||
| } | |||
|
|
|||
| func TestIfExecutorStartsWithoutConfig(t *testing.T) { | |||
There was a problem hiding this comment.
It should just start and don't panic or return error also?
There was a problem hiding this comment.
Well, my guess is it just should not panic. The reason for what Janisz reported was that the backoff notifier swaps minWait and maxWait if it compares them and finds maxWait to be lesser of the two. So we just had to make sure that we pass in the correct value for SubscriptionBackoffMax. This part of Config had no rules defining the defaults, and the minWait is hardcoded to time.Second - so I just check if max is less and assign a default value.
Fixes #103.