Add Storage Integration Testing #236

mh-park · 2017-06-28T15:30:21Z

w/ an example on ElasticSearch

…integration test

yurishkuro · 2017-06-28T15:33:38Z

plugin/storage/integration/domain_trace_compare_test.go

+	"github.com/uber/jaeger/model"
+)
+
+type TraceByKey []*model.Trace


did you mean "by trace id"?

yurishkuro · 2017-06-28T15:36:11Z

plugin/storage/integration/domain_trace_compare_test.go

+
+func compareModelTags(t *testing.T, expected []model.KeyValue, actual []model.KeyValue) {
+	sort.Sort(TagByKey(expected))
+	sort.Sort(TagByKey(actual))


does this mean ES backend does not preserve the order of tags in the Span document?

I think ES does, but this is for any general backend.

fair enough. I would suggest having a single "normalize" function that does all the sorting on a given trace, and after that ideally just use assert.Equal on the whole object. If you run into issues with pointers being slightly different, you can convert the whole thing to a JSON and do equals on strings.

the downside to doing that would be that it would be difficult to see where the traces differ.

not necessarily. First, you can use pretty to print the diffs, as done here. Another trick I used was to write out the JSON to files (when the test fails) so that they can be compared with other tools like diff.

just kidding, i think assert.EqualValues tells us where

yurishkuro · 2017-06-28T15:36:44Z

plugin/storage/integration/domain_trace_compare_test.go

+// this function exists solely to make it easier for developer to find out where the difference is
+func compareModelLogs(t *testing.T, expected []model.Log, actual []model.Log) {
+	sort.Sort(LogByTimestamp(expected))
+	sort.Sort(LogByTimestamp(actual))


same question - why doesn't the backend simply preserve the order of logs?

yurishkuro · 2017-06-28T15:37:15Z

plugin/storage/integration/integration_test.go

+
+const (
+	byService QueryType = iota
+	bysNoN


yurishkuro · 2017-06-28T15:40:19Z

plugin/storage/integration/integration_test.go

+	bysNTags
+	bysNDuration
+	byNumOfTraces
+	// Add more queryTypes here


why do we need these enums instead of simply having distinct test cases as data or as test methods? Each scenario requires special data to be inserted into storage before querying.

you're right. I think I used this earlier when I needed the enums, but i changed the format and we no longer need it. i'll refactor

coveralls · 2017-06-28T17:52:20Z

Coverage remained the same at 100.0% when pulling d9ca512 on es-itest into 3c5b63c on master.

coveralls · 2017-06-28T23:11:57Z

Coverage remained the same at 100.0% when pulling 8a12bc7 on es-itest into 3c5b63c on master.

coveralls · 2017-06-29T15:59:30Z

Coverage remained the same at 100.0% when pulling b854c66 on es-itest into 3c5b63c on master.

mh-park · 2017-06-29T22:23:26Z

there might be unused stuff that i haven't caught, let me know if you find anything such

coveralls · 2017-06-29T22:58:13Z

Coverage remained the same at 100.0% when pulling aa38aba on es-itest into 3c5b63c on master.

coveralls · 2017-06-29T23:13:07Z

Coverage remained the same at 100.0% when pulling aa38aba on es-itest into 3c5b63c on master.

coveralls · 2017-06-30T14:58:09Z

Coverage remained the same at 100.0% when pulling d4077b5 on es-itest into 3c5b63c on master.

coveralls · 2017-06-30T15:13:17Z

Coverage remained the same at 100.0% when pulling eb14aad on es-itest into 3c5b63c on master.

coveralls · 2017-06-30T15:25:51Z

Coverage remained the same at 100.0% when pulling eb14aad on es-itest into 3c5b63c on master.

coveralls · 2017-07-05T17:04:52Z

Coverage remained the same at 100.0% when pulling fea22a5 on es-itest into 3c5b63c on master.

black-adder · 2017-07-05T18:43:55Z

revert idl

black-adder · 2017-07-05T18:46:04Z

model/sort_traces.go

+func (s spanBySpanID) Less(i, j int) bool { return s[i].SpanID < s[j].SpanID }
+
+// SortTraces checks two traces' length and sorts their spans.
+func SortTraces(expected *Trace, actual *Trace) error {


I was more thinking sort just one trace and leave the comparisons to domain_trace_compare_test.go

Hmm, so I did this before, and the issue with this was that I would get index out of bound error with the assert.Equals, and the error message wouldn't be very helpful. the current way catches these because they'll do a length check on the way

sort the traces separately but still do the length checks in domain_trace_compare_test.go?

black-adder · 2017-07-05T18:47:20Z

plugin/storage/integration/es_integration_test.go

+	return errors.New("elastic search is not ready")
+}
+
+// DO NOT RUN IF YOU HAVE IMPORTANT SPANS IN ELASTICSEARCH


is this comment still valid? hopefully people arent stupid enough to run integration tests in production

never trust people

coveralls · 2017-07-05T19:25:48Z

Coverage remained the same at 100.0% when pulling 25c106b on es-itest into 3c5b63c on master.

coveralls · 2017-07-05T20:26:09Z

Coverage remained the same at 100.0% when pulling ed0a89d on es-itest into 3c5b63c on master.

coveralls · 2017-07-05T22:30:21Z

Coverage remained the same at 100.0% when pulling b7a5877 on es-itest into 3c5b63c on master.

black-adder

I forgot one test case where we return more than one trace, tis the last one I swear

black-adder · 2017-07-06T14:36:25Z

model/sort.go

+func (s spanBySpanID) Swap(i, j int)      { s[i], s[j] = s[j], s[i] }
+func (s spanBySpanID) Less(i, j int) bool { return s[i].SpanID < s[j].SpanID }
+
+// SortTrace checks two traces' length and sorts their spans.


update comment

black-adder · 2017-07-06T14:42:37Z

plugin/storage/integration/domain_trace_compare_test.go

+	"github.com/uber/jaeger/model"
+)
+
+type TraceByTraceID []*model.Trace


Why not move this to model as well?

black-adder · 2017-07-06T14:44:56Z

plugin/storage/integration/domain_trace_compare_test.go

+		expectedSpan := expected.Spans[i]
+		actualSpan := actual.Spans[i]
+		require.True(t, len(expectedSpan.Tags) == len(actualSpan.Tags))
+		require.True(t, len(expectedSpan.Logs) == len(actualSpan.Logs))


compare process tag lengths

black-adder · 2017-07-06T14:51:25Z

plugin/storage/integration/integration_test.go

+func (s *StorageIntegration) IntegrationTestGetServices(t *testing.T) {
+	t.Log("Testing GetServices ...")
+
+	// TODO


black-adder · 2017-07-06T14:53:56Z

plugin/storage/integration/integration_test.go

+func (s *StorageIntegration) IntegrationTestGetOperations(t *testing.T) {
+	t.Log("Testing GetOperations ...")
+
+	// TODO


black-adder · 2017-07-06T15:02:55Z

plugin/storage/integration/integration_test.go

+}
+
+func (s *StorageIntegration) integrationTestFindTracesByQuery(t *testing.T, query *spanstore.TraceQueryParameters, expectedFixture string) {
+	expectedTrace, err := getTraceFixture(expectedFixture)


these 3 lines seem to be repeated in every test

black-adder · 2017-07-06T15:07:35Z

plugin/storage/integration/integration_test.go

+
+	var found bool
+	var actual []*model.Trace
+	for i := 0; i < 30; i++ {


const 30 to iterations or something

black-adder · 2017-07-06T15:07:49Z

plugin/storage/integration/integration_test.go

+	var found bool
+	var actual []*model.Trace
+	for i := 0; i < 30; i++ {
+		s.logger.Info(fmt.Sprintf("Waiting for storage backend to update documents, iteration %d out of %d", i+1, 30))


this comment repeated everywhere, DRY it

black-adder · 2017-07-06T15:13:21Z

plugin/storage/integration/integration_test.go

+		s.logger.Info(fmt.Sprintf("Waiting for storage backend to update documents, iteration %d out of %d", i+1, 30))
+		actual, err = s.reader.FindTraces(query)
+		if err == nil {
+			if len(actual) == query.NumTraces {


but NumTrace in your queries are always 1000, so this never gets hit

black-adder · 2017-07-06T15:16:19Z

plugin/storage/integration/integration_test.go

+		s.logger.Info(fmt.Sprintf("Waiting for storage backend to update documents, iteration %d out of %d", i+1, 30))
+		actual, err = s.reader.GetTrace(traceID)
+		if err == nil {
+			found = len(actual.Spans) == len(expected.Spans)


ok, this one you can inline

coveralls · 2017-07-06T16:53:23Z

Coverage remained the same at 100.0% when pulling b1650b4 on es-itest into 3c5b63c on master.

black-adder

this doesn't include multiple traces test yet?

black-adder · 2017-07-06T17:21:20Z

model/sort.go

+func (s traceByTraceID) Len() int      { return len(s) }
+func (s traceByTraceID) Swap(i, j int) { s[i], s[j] = s[j], s[i] }
+func (s traceByTraceID) Less(i, j int) bool {
+	if len(s[i].Spans) != len(s[j].Spans) {


number of spans should have no bearing on the order of the traces unless the traceIDs are identical

black-adder · 2017-07-06T17:26:44Z

plugin/storage/integration/integration_test.go

+	t.Log("Testing GetTrace ...")
+
+	expected := s.getBasicFixture(t)
+	expectedTraceID := model.TraceID{Low: 17}


so you cant get this from expected instead of hard coding it?

oh thats what you meant ok i can do that

black-adder · 2017-07-06T17:28:07Z

plugin/storage/integration/integration_test.go

+		s.logger.Info(fmt.Sprintf(waitForBackendComment, i+1, iterations))
+		actual, err := s.reader.GetTrace(expectedTraceID)
+		if err == nil {
+			if found = len(actual.Spans) == len(expected.Spans); found {


you don't need the found variable, you can just do the length check here and L148

black-adder · 2017-07-06T17:30:36Z

plugin/storage/integration/integration_test.go

+		s.logger.Info(fmt.Sprintf(waitForBackendComment, i+1, iterations))
+		actual, err = s.reader.FindTraces(query)
+		if err == nil {
+			if len(actual) == query.NumTraces {


so we dont need this right?

black-adder · 2017-07-06T17:36:05Z

plugin/storage/integration/integration_test.go

+				found = true
+				break
+			}
+			found = tracesMatch(actual, expected)


why can't we do:

if tracesMatch(actual, expected) { break }

and then CompareSliceOfTraces below after the for loop once? Given CompareSliceOfTraces uses require will stop execution anyway for bad traces, just do it once

coveralls · 2017-07-06T18:20:26Z

Coverage remained the same at 100.0% when pulling e7a97ec on es-itest into 3c5b63c on master.

black-adder · 2017-07-06T19:14:33Z

model/sort.go

+
+func (s traceByTraceID) Len() int      { return len(s) }
+func (s traceByTraceID) Swap(i, j int) { s[i], s[j] = s[j], s[i] }
+func (s traceByTraceID) Less(i, j int) bool {


s[i].Spans[0].TraceID.Low < s[j].Spans[0].TraceID.Low is the only check you need now thinking about it. You can't (shouldn't) have two traces with the same id so using the traceid as the tie breaker is all you need.

black-adder · 2017-07-06T19:17:13Z

plugin/storage/integration/integration_test.go

+		s.logger.Info(fmt.Sprintf(waitForBackendComment, i+1, iterations))
+		var err error
+		actual, err = s.reader.GetTrace(expectedTraceID)
+		if err == nil {


can combine these two if stmts

black-adder · 2017-07-06T19:20:52Z

plugin/storage/integration/integration_test.go

+	expected, err := getTraceFixtures(expectedFixtures)
+	require.NoError(t, err)
+	require.NoError(t, s.writeTraces(expected))
+	require.NoError(t, s.refresh())


require.NoError(t, s.refresh()) here but the other tests dont do require.

coveralls · 2017-07-06T19:23:17Z

Coverage remained the same at 100.0% when pulling 61bcbad on es-itest into 3c5b63c on master.

coveralls · 2017-07-06T20:12:31Z

Coverage remained the same at 100.0% when pulling ff11a78 on es-itest into 3c5b63c on master.

* create integration test framework for storage + create elasticsearch integration test * refactor queries format * rename comparison fns to fit its behavior * fix rand.Uint64() (does not exist in go1.7) * revert env var check * refactor trace w/ query checker to separate file + test * import list reorder * change itest direction * change integration test format * make fmt/lint * make fmt/lint * prettify output when test fails * fmt * delete query to trace checker * remove unnecessary code * add integration test script * fix syntax * add comment about fixtures and queries * fix travis yml to run es integ test * remove es integ test from install travis * fix typo * fix err check bug * fmt * move sorting fns to model * fix imports * fix code reviews * change trace fixtures to describe the trace * revert idl * trust people * sort refactored * fix trace comparison test + raise timeout for es-integration test * code review * remove numtraces check * add multiple-trace test case * code review

create integration test framework for storage + create elasticsearch …

e821d3f

…integration test

mh-park requested a review from black-adder June 28, 2017 15:30

yurishkuro reviewed Jun 28, 2017

View reviewed changes

plugin/storage/integration/integration_test.go Outdated

const (

byService QueryType = iota

bysNoN

Copy link

Member

yurishkuro Jun 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

yurishkuro reviewed Jun 28, 2017

View reviewed changes

mh-park added 4 commits June 28, 2017 11:50

refactor queries format

135f5fb

rename comparison fns to fit its behavior

8d4ea4b

fix rand.Uint64() (does not exist in go1.7)

757e234

revert env var check

d9ca512

refactor trace w/ query checker to separate file + test

8a12bc7

import list reorder

b854c66

mh-park added 3 commits June 29, 2017 14:28

change itest direction

6891103

change integration test format

d86d63f

make fmt/lint

53a4edd

make fmt/lint

aa38aba

prettify output when test fails

d4077b5

mh-park added 2 commits June 30, 2017 11:02

fmt

e3c8638

delete query to trace checker

eb14aad

remove unnecessary code

c5991fc

fix code reviews

fea22a5

change trace fixtures to describe the trace

25c106b

black-adder reviewed Jul 5, 2017

View reviewed changes

mh-park added 2 commits July 5, 2017 15:28

revert idl

305c46f

trust people

ed0a89d

mh-park added 2 commits July 5, 2017 18:00

sort refactored

d9a63fc

fix trace comparison test + raise timeout for es-integration test

b7a5877

black-adder reviewed Jul 6, 2017

View reviewed changes

code review

b1650b4

black-adder reviewed Jul 6, 2017

View reviewed changes

remove numtraces check

e7a97ec

add multiple-trace test case

61bcbad

black-adder reviewed Jul 6, 2017

View reviewed changes

code review

ff11a78

black-adder approved these changes Jul 10, 2017

View reviewed changes

Merge branch 'master' into es-itest

f3c74dc

black-adder requested review from oibe and vprithvi as code owners July 10, 2017 19:12

mh-park merged commit 433b7e0 into master Jul 10, 2017

mh-park deleted the es-itest branch July 17, 2017 15:25

Add Storage Integration Testing #236

Add Storage Integration Testing #236

Conversation

mh-park commented Jun 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro Jun 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jun 28, 2017

coveralls commented Jun 28, 2017

coveralls commented Jun 29, 2017

mh-park commented Jun 29, 2017

coveralls commented Jun 29, 2017

coveralls commented Jun 29, 2017

coveralls commented Jun 30, 2017

coveralls commented Jun 30, 2017

coveralls commented Jun 30, 2017

coveralls commented Jul 5, 2017

black-adder commented Jul 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jul 5, 2017

coveralls commented Jul 5, 2017

coveralls commented Jul 5, 2017

black-adder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jul 6, 2017

black-adder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jul 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jul 6, 2017

coveralls commented Jul 6, 2017

mh-park commented Jun 28, 2017 •

edited

Loading

yurishkuro Jun 28, 2017 •

edited

Loading