[WIP] Add basic heartbeat job tests #7551
Conversation
andrewvc
added
in progress
| req, err := http.NewRequest("GET", server.URL, nil) | ||
| assert.Nil(t, err) | ||
|
|
||
| var validatorResp *http.Response = nil |
There was a problem hiding this comment.
should drop = nil from declaration of var validatorResp; it is the zero value
andrewvc
changed the title
There was a problem hiding this comment.
As you said it's tricky to find the right balance for the tests. I personally think the biggest value we get out of the tests which run against an endpoint as you did with the http endpoint. There we test directly all the pieces at once. The unit tests can be very useful for developing / adding new features to confirm things work but they can also slow us down if we check too many details. In general I think we should always try to use table test which makes it very easy to add additional checks or modify existing ones.
For the http endpoint tests: I would focus first on getting them running and as soon as we have several test implementations we can still figure out where they fit best and how we can simplify the tests further.
It seems your changes broke the system tests. I also see these as very valuable as they test in the end how the user will interact with the beat and how all pieces play together.
| @@ -31,6 +31,8 @@ import ( | |||
| "github.com/elastic/beats/libbeat/common" | |||
| "github.com/elastic/beats/libbeat/outputs/transport" | |||
|
|
|||
| "io" | |||
There was a problem hiding this comment.
Nit: Should go on the top list of imports.
| @@ -218,41 +220,69 @@ func execPing( | |||
| body []byte, | |||
| timeout time.Duration, | |||
| validator func(*http.Response) error, | |||
| ) (time.Time, time.Time, common.MapStr, reason.Reason) { | |||
| ) (start time.Time, end time.Time, event common.MapStr, errReason reason.Reason) { | |||
There was a problem hiding this comment.
Not really used to name returns but I see that it increases the readability of the code here. Glad to see that below you don't use a "naked" return.
| defer closeIfPresent(&req.Body) | ||
|
|
||
| start, end, resp, errReason := execRequest(client, req, validator) | ||
| if resp != nil { // If above errors, the response will be nil |
There was a problem hiding this comment.
What about moving it after the error check in 234? You write that in case of error it is nil. Is also the opposite true that it's not nil in case of no error?
| resp, err := client.Do(req) | ||
| end := time.Now() | ||
| end = time.Now() |
There was a problem hiding this comment.
Where do you close the response body now?
| if err != nil { | ||
| return start, end, nil, reason.IOFailed(err) | ||
| } | ||
| defer resp.Body.Close() | ||
|
|
||
| err = validator(resp) | ||
| end = time.Now() |
There was a problem hiding this comment.
This was here before but I'm trying to understand why we move the end time after the validator.
There was a problem hiding this comment.
Good question! Any idea @urso ? I can't think of why we'd want to include the validator in the time. That said, I think the difference is negligible. Most validators should be dead simple.
| @@ -23,59 +23,174 @@ import ( | |||
| "net/url" | |||
| "reflect" | |||
| "testing" | |||
|
|
|||
| t.Errorf("Expected %T but got %T", err, test.expectedError) | ||
| test := test | ||
|
|
||
| t.Run(test.name, func(t2 *testing.T) { |
|
|
||
| if err != nil { | ||
| if test.expectedError == nil { | ||
| t.Error(err) |
There was a problem hiding this comment.
You name it above t2 but in here you use t. I think you could just stick to t as name also above.
There was a problem hiding this comment.
That's what blindly obeying your IDE gets you ;) Will fix
|
Something still seems to break a system test: https://beats-ci.elastic.co/job/elastic+beats+pull-request+multijob-linux/5429/beat=heartbeat,label=ubuntu/testReport/junit/test_monitor/Test/test_http_1_404/ Let me know if you need some help to execute it locally to see all the logs etc. |
|
Thanks for the heads up. At the moment I'm reworking these tests into a
pseudo-framework. All of the end to end tests nest so to speak. An http
check should include a tcp check in the event.
I'll push up some larger changes EOD and take a look then
…On Wed, Jul 11, 2018, 4:56 AM Nicolas Ruflin ***@***.***> wrote:
Something still seems to break a system test:
https://beats-ci.elastic.co/job/elastic+beats+pull-request+multijob-linux/5429/beat=heartbeat,label=ubuntu/testReport/junit/test_monitor/Test/test_http_1_404/
Let me know if you need some help to execute it locally to see all the logs
etc.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#7551 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAIBY1ajFjEQkFuFqAAosA-Tu34OD6Xuks5uFcvAgaJpZM4VItSE>
.
|
heartbeat/testcommon/testcommon.go
Outdated
|
|
||
| type MapCheckDef common.MapStr | ||
|
|
||
| func DeepMapStrCheck(t *testing.T, expected MapCheckDef, actual common.MapStr) { |
There was a problem hiding this comment.
exported function DeepMapStrCheck should have comment or be unexported
heartbeat/testcommon/testcommon.go
Outdated
|
|
||
| type MapLeafTest = func(t *testing.T, actual interface{}) | ||
|
|
||
| type MapCheckDef common.MapStr |
There was a problem hiding this comment.
exported type MapCheckDef should have comment or be unexported
heartbeat/testcommon/testcommon.go
Outdated
| assert.True(t, ok) | ||
| } | ||
|
|
||
| type MapLeafTest = func(t *testing.T, actual interface{}) |
There was a problem hiding this comment.
exported type MapLeafTest should have comment or be unexported
heartbeat/testcommon/testcommon.go
Outdated
| assert.Nil(t, actual) | ||
| } | ||
|
|
||
| var IsString = func(t *testing.T, actual interface{}) { |
There was a problem hiding this comment.
exported var IsString should have comment or be unexported
heartbeat/testcommon/testcommon.go
Outdated
| assert.True(t, converted >= 0) | ||
| } | ||
|
|
||
| var IsNil = func(t *testing.T, actual interface{}) { |
There was a problem hiding this comment.
exported var IsNil should have comment or be unexported
heartbeat/testcommon/testcommon.go
Outdated
| io.WriteString(w, BadGatewayBody) | ||
| }) | ||
|
|
||
| var ExactlyEqual = func(expected interface{}) func(t *testing.T, actual interface{}) { |
There was a problem hiding this comment.
exported var ExactlyEqual should have comment or be unexported
heartbeat/testcommon/testcommon.go
Outdated
|
|
||
| var BadGatewayBody = "Bad Gateway" | ||
|
|
||
| var BadGatewayHandler = http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { |
There was a problem hiding this comment.
exported var BadGatewayHandler should have comment or be unexported
heartbeat/testcommon/testcommon.go
Outdated
| io.WriteString(w, HelloWorldBody) | ||
| }) | ||
|
|
||
| var BadGatewayBody = "Bad Gateway" |
There was a problem hiding this comment.
exported var BadGatewayBody should have comment or be unexported
heartbeat/testcommon/testcommon.go
Outdated
|
|
||
| var HelloWorldBody = "hello, world!" | ||
|
|
||
| var HelloWorldHandler = http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { |
There was a problem hiding this comment.
exported var HelloWorldHandler should have comment or be unexported
heartbeat/testcommon/testcommon.go
Outdated
| "github.com/elastic/beats/libbeat/common" | ||
| ) | ||
|
|
||
| var HelloWorldBody = "hello, world!" |
There was a problem hiding this comment.
exported var HelloWorldBody should have comment or be unexported
heartbeat/mapscheme/mapscheme.go
Outdated
| return validateInternal(t, expected, actual, actual, []string{}) | ||
| } | ||
|
|
||
| type WalkObserver func( |
There was a problem hiding this comment.
exported type WalkObserver should have comment or be unexported
heartbeat/mapscheme/mapscheme.go
Outdated
| } | ||
| } | ||
|
|
||
| func Validate(t *testing.T, expected MapCheckDef, actual common.MapStr) *ValidationResult { |
There was a problem hiding this comment.
exported function Validate should have comment or be unexported
heartbeat/mapscheme/mapscheme.go
Outdated
| } | ||
| } | ||
|
|
||
| func Scheme(expected MapCheckDef) Validator { |
There was a problem hiding this comment.
exported function Scheme should have comment or be unexported
heartbeat/mapscheme/mapscheme.go
Outdated
| } | ||
| } | ||
|
|
||
| func Strict(validator Validator) Validator { |
There was a problem hiding this comment.
exported function Strict should have comment or be unexported
heartbeat/mapscheme/mapscheme.go
Outdated
| return &output | ||
| } | ||
|
|
||
| func Compose(validators ...Validator) Validator { |
There was a problem hiding this comment.
exported function Compose should have comment or be unexported
heartbeat/mapscheme/mapscheme.go
Outdated
| assert.True(t, converted >= 0) | ||
| } | ||
|
|
||
| var IsNil = func(t *testing.T, _ bool, actual interface{}) { |
There was a problem hiding this comment.
exported var IsNil should have comment or be unexported
heartbeat/mapscheme/mapscheme.go
Outdated
| } | ||
| } | ||
|
|
||
| var IsDuration = func(t *testing.T, _ bool, actual interface{}) { |
There was a problem hiding this comment.
exported var IsDuration should have comment or be unexported
heartbeat/mapscheme/mapscheme.go
Outdated
| } | ||
| } | ||
|
|
||
| func TcpChecks(port uint16) MapCheckDef { |
There was a problem hiding this comment.
exported function TcpChecks should have comment or be unexported
func TcpChecks should be TCPChecks
heartbeat/mapscheme/mapscheme.go
Outdated
|
|
||
| // Functions for testing maps in complex ways | ||
|
|
||
| func MonitorChecks(id string, ip string, scheme string, status string) MapCheckDef { |
There was a problem hiding this comment.
exported function MonitorChecks should have comment or be unexported
heartbeat/mapscheme/mapscheme.go
Outdated
| } | ||
| } | ||
|
|
||
| func ServerPort(server *httptest.Server) (uint16, error) { |
There was a problem hiding this comment.
exported function ServerPort should have comment or be unexported
heartbeat/valschema/validators.go
Outdated
| assert.True(t, exists) | ||
| } | ||
|
|
||
| var DoesNotExist = func(t *testing.T, exists bool, actual interface{}) { |
There was a problem hiding this comment.
exported var DoesNotExist should have comment or be unexported
heartbeat/valschema/validators.go
Outdated
| assert.True(t, ok) | ||
| } | ||
|
|
||
| var DoesExist = func(t *testing.T, exists bool, actual interface{}) { |
There was a problem hiding this comment.
exported var DoesExist should have comment or be unexported
heartbeat/valschema/validators.go
Outdated
| assert.Nil(t, actual) | ||
| } | ||
|
|
||
| var IsString = func(t *testing.T, _ bool, actual interface{}) { |
There was a problem hiding this comment.
exported var IsString should have comment or be unexported
heartbeat/valschema/validators.go
Outdated
| assert.True(t, converted >= 0) | ||
| } | ||
|
|
||
| var IsNil = func(t *testing.T, _ bool, actual interface{}) { |
There was a problem hiding this comment.
exported var IsNil should have comment or be unexported
heartbeat/valschema/validators.go
Outdated
| "github.com/stretchr/testify/assert" | ||
| ) | ||
|
|
||
| var IsDuration = func(t *testing.T, _ bool, actual interface{}) { |
There was a problem hiding this comment.
exported var IsDuration should have comment or be unexported
heartbeat/valschema/core.go
Outdated
|
|
||
| type Validator func(*testing.T, common.MapStr) *ValidationResult | ||
|
|
||
| type ValidationResult struct { |
There was a problem hiding this comment.
exported type ValidationResult should have comment or be unexported
heartbeat/valschema/core.go
Outdated
|
|
||
| type StrictMap Map | ||
|
|
||
| type Validator func(*testing.T, common.MapStr) *ValidationResult |
There was a problem hiding this comment.
exported type Validator should have comment or be unexported
heartbeat/valschema/core.go
Outdated
|
|
||
| type Map map[string]interface{} | ||
|
|
||
| type StrictMap Map |
There was a problem hiding this comment.
exported type StrictMap should have comment or be unexported
heartbeat/valschema/core.go
Outdated
|
|
||
| type ValueValidator = func(t *testing.T, keyExists bool, actual interface{}) | ||
|
|
||
| type Map map[string]interface{} |
There was a problem hiding this comment.
exported type Map should have comment or be unexported
heartbeat/valschema/core.go
Outdated
| "github.com/elastic/beats/libbeat/common" | ||
| ) | ||
|
|
||
| type ValueValidator = func(t *testing.T, keyExists bool, actual interface{}) |
There was a problem hiding this comment.
exported type ValueValidator should have comment or be unexported
| }) | ||
| } | ||
|
|
||
| func TcpChecks(port uint16) skima.Validator { |
There was a problem hiding this comment.
exported function TcpChecks should have comment or be unexported
func TcpChecks should be TCPChecks
| return uint16(p), nil | ||
| } | ||
|
|
||
| func MonitorChecks(id string, ip string, scheme string, status string) skima.Validator { |
There was a problem hiding this comment.
exported function MonitorChecks should have comment or be unexported
| io.WriteString(w, BadGatewayBody) | ||
| }) | ||
|
|
||
| func ServerPort(server *httptest.Server) (uint16, error) { |
There was a problem hiding this comment.
exported function ServerPort should have comment or be unexported
|
|
||
| var BadGatewayBody = "Bad Gateway" | ||
|
|
||
| var BadGatewayHandler = http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { |
There was a problem hiding this comment.
exported var BadGatewayHandler should have comment or be unexported
| io.WriteString(w, HelloWorldBody) | ||
| }) | ||
|
|
||
| var BadGatewayBody = "Bad Gateway" |
There was a problem hiding this comment.
exported var BadGatewayBody should have comment or be unexported
heartbeat/skima/validators.go
Outdated
| "github.com/stretchr/testify/assert" | ||
| ) | ||
|
|
||
| var IsDuration = func(t *testing.T, _ bool, actual interface{}, dotPath string) { |
There was a problem hiding this comment.
exported var IsDuration should have comment or be unexported
heartbeat/skima/core.go
Outdated
| return walkValidate(t, expected, actual) | ||
| } | ||
|
|
||
| type WalkObserver func( |
There was a problem hiding this comment.
exported type WalkObserver should have comment or be unexported
heartbeat/skima/core.go
Outdated
| } | ||
| } | ||
|
|
||
| func Validate(t *testing.T, expected Map, actual common.MapStr) *ValidationResult { |
There was a problem hiding this comment.
exported function Validate should have comment or be unexported
heartbeat/skima/core.go
Outdated
| } | ||
| } | ||
|
|
||
| func Schema(expected Map) Validator { |
There was a problem hiding this comment.
exported function Schema should have comment or be unexported
| } | ||
| } | ||
|
|
||
| func Strict(validator Validator) Validator { |
There was a problem hiding this comment.
exported function Strict should have comment or be unexported
|
OK, so for the latest round of changes to the schema validation the code could definitely use a few more passes by myself before the internals are nice, but I'd love feedback on the externals at this point. How's this feel as a schema validation library from a user standpoint? So, I got pretty far on the validation DSL that lets you match a nested map of validators to a MapStr. @urso and I agreed that this would be very useful for the sort of tests heartbeat needs. The following test suite tests the validation framework: https://github.com/andrewvc/beats/blob/add_hb_http_task_tests/heartbeat/skima/core_test.go You can see how useful it is for heartbeat in the new basic tests I added for the http and tcp endpoints: This obviously exceeds the original scope of this PR. I'll probably shut this one down soon and open a new one. On @urso’s advice I modified the schema from my original procedural style to be more FP style and be oriented around function composition. I really like the result! There’s some weirdness with typing ‘strict’ operations (validate that this map passes all checks and also doesn’t have additional keys). I implemented it two ways for now, you can either do it by composing |
andrewvc
changed the title
| } | ||
| } | ||
|
|
||
| func IsIntGt(than int) IsDef { |
There was a problem hiding this comment.
exported function IsIntGt should have comment or be unexported
| }) | ||
| } | ||
|
|
||
| var IsNil = Is("is nil", func(v interface{}) ValueResult { |
There was a problem hiding this comment.
exported var IsNil should have comment or be unexported
| } | ||
| }) | ||
|
|
||
| func IsEqual(to interface{}) IsDef { |
There was a problem hiding this comment.
exported function IsEqual should have comment or be unexported
| "fmt" | ||
| ) | ||
|
|
||
| var IsDuration = Is("is a duration", func(v interface{}) ValueResult { |
There was a problem hiding this comment.
exported var IsDuration should have comment or be unexported
| return walkValidate(expected, actual) | ||
| } | ||
|
|
||
| type WalkObserver func( |
There was a problem hiding this comment.
exported type WalkObserver should have comment or be unexported
| } | ||
| } | ||
|
|
||
| func Validate(expected Map, actual common.MapStr) MapResults { |
There was a problem hiding this comment.
exported function Validate should have comment or be unexported
| }) | ||
| } | ||
|
|
||
| func Schema(expected Map) Validator { |
There was a problem hiding this comment.
exported function Schema should have comment or be unexported
| } | ||
| */ | ||
|
|
||
| func Test(t *testing.T, r MapResults) { |
There was a problem hiding this comment.
exported function Test should have comment or be unexported
| return errors | ||
| } | ||
|
|
||
| func (r MapResults) Valid() bool { |
There was a problem hiding this comment.
exported method MapResults.Valid should have comment or be unexported
| } | ||
| } | ||
|
|
||
| func IsIntGt(than int) IsDef { |
There was a problem hiding this comment.
exported function IsIntGt should have comment or be unexported
| }) | ||
| } | ||
|
|
||
| var IsNil = Is("is nil", func(v interface{}) ValueResult { |
There was a problem hiding this comment.
exported var IsNil should have comment or be unexported
| } | ||
| }) | ||
|
|
||
| func IsEqual(to interface{}) IsDef { |
There was a problem hiding this comment.
exported function IsEqual should have comment or be unexported
| "fmt" | ||
| ) | ||
|
|
||
| var IsDuration = Is("is a duration", func(v interface{}) ValueResult { |
There was a problem hiding this comment.
exported var IsDuration should have comment or be unexported
| return walkValidate(expected, actual) | ||
| } | ||
|
|
||
| type WalkObserver func( |
There was a problem hiding this comment.
exported type WalkObserver should have comment or be unexported
| } | ||
| } | ||
|
|
||
| func Validate(expected Map, actual common.MapStr) MapResults { |
There was a problem hiding this comment.
exported function Validate should have comment or be unexported
| }) | ||
| } | ||
|
|
||
| func Schema(expected Map) Validator { |
There was a problem hiding this comment.
exported function Schema should have comment or be unexported
| } | ||
| */ | ||
|
|
||
| func Test(t *testing.T, r MapResults) { |
There was a problem hiding this comment.
exported function Test should have comment or be unexported
| return errors | ||
| } | ||
|
|
||
| func (r MapResults) Valid() bool { |
There was a problem hiding this comment.
exported method MapResults.Valid should have comment or be unexported
|
This thread is too long. Moved to: #7587 |
EDIT: The scope here is now much wider
This PR is now about testing both HTTP and TCP, and introducing a new test schema DSL to do so. It's getting kinda big.
Original below
This PR adds some initial tests for the HTTP task. An open question is how comprehensive we want to be here and where we want to target these tests. It also breaks up one of the longer functions to make reasoning about it easier (and to make understanding the defer behavior more clear)
I think that thorough testing of each intermediate function is a bit overkill, so, I think the question is where to strike the balance. I would love your feedback @urso / @ruflin .
My sense is that we definitely want some acceptance tests for each protocol that test that configuring them correctly does the right thing.
We probably also want unit tests for most helper functions. For the actual IO however, I'm thinking we probably should only setup HTTP servers once in the tests simply to not waste time redundantly testing things. I don't think the spot I'm doing it in these examples is the best one. It probably makes more sense to set that same server up at the 'job level and test the
monitors.Jobobject against it, including thorough tests of all expected fields.Thoughts?