-
Notifications
You must be signed in to change notification settings - Fork 31
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
plan: test, which illustrate "return nil, err" problem #336
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test scenario seems good; we should probably keep the test unmerged and rebase after the module API changes since AFAIK they should fix this.
Status string | ||
Level resource.StatusLevel | ||
Error error | ||
NilAndError bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it would make more sense to have a type NilTask struct
that just returns nil, error
to keep the code clean
175ed11
to
0cab19e
Compare
@rebeccaskinner I add commit with fix, ad can rework test, if you or @BrianHicks prefer it. This fix need to be merged before lvm branch |
@@ -108,6 +108,11 @@ func (g *pipelineGen) PlanNode(taski interface{}) (interface{}, error) { | |||
} | |||
status, err := twrapper.Task.Check(renderer) | |||
|
|||
// create empty Status structure, if it not created in .Check() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we use this pr, we probably need the same check in apply/pipeline.go. That is where I am panicking in #343
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ryane Will do, so don't panic ;)
0cab19e
to
0494335
Compare
return nil, errors.New("apply error") | ||
} | ||
|
||
// NilAndError return a FakeTask that will simulate `return nil, err` cawe |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
case
|
||
require.NoError(t, g.Validate()) | ||
|
||
// applying should return error, and not panicking |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Apply should return an error and not panic
@@ -105,6 +105,20 @@ func TestApplyStillChange(t *testing.T) { | |||
} | |||
} | |||
|
|||
func TestApplyNilError(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add comments on exported methods (will fix cc issues)
@@ -45,6 +45,21 @@ func TestPlanNoOp(t *testing.T) { | |||
assert.Equal(t, task, result.Task) | |||
} | |||
|
|||
func TestPlanNilAndError(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add comments on exported methods
@@ -65,6 +65,25 @@ func WillChange() *FakeTask { | |||
} | |||
} | |||
|
|||
// Task which always return (nil, error) tuple |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix comments on these. see https://codeclimate.com/github/asteris-llc/converge/pull/336
a73ec6b
to
0a1ffed
Compare
@rebeccaskinner @ryane all test are green now. |
0a1ffed
to
611ec9f
Compare
plan: test, which illustrate "return nil, err" problem
No description provided.