Run relevant unit tests on both indexes #8685

e-dard · 2017-08-10T12:38:33Z

Required for all non-trivial PRs

Rebased/mergable
Tests pass

This PR ensure that where appropriate, unit tests are run on both indexes using sub-tests. Benchmarks have also been updated in a similar way.

In terms of the tests package tests, I found that the test output was too noisy when using Go's verbose -v mode, which itself is useful for seeing the sub-test result output. Therefore I've added a new -vv flag for that package, if the server logs are desirable output. /cc @mark-rushakoff @aanthony1243.

Finally, regarding the tests package, all queries for each test will now run as sub-tests.

go test -v github.com/influxdata/influxdb/tests -vv

jwilder · 2017-08-10T15:32:43Z

=== RUN   TestStore_Cardinality_Compactions/inmem
[I] 2017-08-10T15:21:33Z Using data dir: /tmp/influxdb-tsdb-960455470 service=store
panic: test executed panic(nil) or runtime.Goexit

goroutine 87907 [running]:
testing.tRunner.func1(0xc42056f110)
	/usr/local/go/src/testing/testing.go:622 +0x55f
runtime.Goexit()
	/usr/local/go/src/runtime/panic.go:366 +0x15c
testing.(*common).SkipNow(0xc42005f860)
	/usr/local/go/src/testing/testing.go:555 +0x59
testing.(*common).Skip(0xc42005f860, 0xc420479e60, 0x1, 0x1)
	/usr/local/go/src/testing/testing.go:536 +0x7d
github.com/influxdata/influxdb/tsdb_test.testStoreCardinalityCompactions(0xc42005f860, 0xc4201300d0)
	/root/go/src/github.com/influxdata/influxdb/tsdb/store_test.go:766 +0x142
github.com/influxdata/influxdb/tsdb_test.TestStore_Cardinality_Compactions.func1(0xc85fad, 0x5)
	/root/go/src/github.com/influxdata/influxdb/tsdb/store_test.go:823 +0x155
github.com/influxdata/influxdb/tsdb_test.TestStore_Cardinality_Compactions.func2(0xc42056f110)
	/root/go/src/github.com/influxdata/influxdb/tsdb/store_test.go:827 +0x72
testing.tRunner(0xc42056f110, 0xc4205b1c00)
	/usr/local/go/src/testing/testing.go:657 +0x108
created by testing.(*T).Run
	/usr/local/go/src/testing/testing.go:697 +0x544
FAIL	github.com/influxdata/influxdb/tsdb	78.467s

e-dard · 2017-08-10T15:33:30Z

@jwilder that must be a flaky test

benbjohnson

Some comments on usage. I can review further depending on what consensus we come to.

benbjohnson · 2017-08-10T15:33:29Z

tests/server_test.go

-		}
+			if query.skip {
+				t.Skipf("SKIP:: %s", query.name)
+			}


This doesn't have to be done in this PR but we can take out all the .skip fields now that we're using t.Run().

You mean the skip fields on the query type? How would we skip tests we don't want to run? I'll admit I have no idea why we're skipping any of them in the first place :)

I believe the skip field was there to allow someone to isolate a test instead of commenting out the other tests. With subtests we can isolate with -run.

I think we still need skip. We've marked things as skip in the past because the tests ended up being flaky and need to be be disabled (and looked at later). If we just comment them out, the output in the log is lost so we can't easily just search the output to see if any tests happened to be skipped.

We can use t.Skip() on the subtest.

benbjohnson · 2017-08-10T15:35:53Z

tsdb/engine/tsm1/engine_test.go

+			// Create a few points.
+			p1 := MustParsePointString("cpu,host=A value=1.1 1000000000")
+			p2 := MustParsePointString("cpu,host=B value=1.2 2000000000")
+			p3 := MustParsePointString("cpu,host=A sum=1.3 3000000000")


Can we use a helper function (e.g. RunForEachIndex(func(*testing.T))) that loops the registered indexes?

Here or everywhere? Are you concerned about indentation?

I thought it was being used in a bunch of places but it seems to be less than I thought. Indentation was one concern. Another concern was DRY and consistency.

benbjohnson · 2017-08-10T15:37:05Z

tsdb/engine/tsm1/engine_test.go

-		err := e.WritePoints(pp)
-		if err != nil {
-			b.Fatal(err)
+			b.Run(fmt.Sprintf("%s_%d", index, sz), func(b *testing.B) {


Can this be two nested b.Run() calls instead of concatenating index & sz?

I didn't know that was an option. Neat.

Hmmm:

BenchmarkEngine_WritePoints/10/inmem-4 20000 77541 ns/op 2701 B/op 48 allocs/op BenchmarkEngine_WritePoints/10#01/tsi1-4 20000 75302 ns/op 2702 B/op 48 allocs/op

Not exactly as nice.

Does it work if you invert index & sz? Does it change it to:

BenchmarkEngine_WritePoints/inmem/10-4 20000 77541 ns/op 2701 B/op 48 allocs/op BenchmarkEngine_WritePoints/tsi1/10/-4 20000 75302 ns/op 2702 B/op 48 allocs/op

benbjohnson · 2017-08-10T15:37:24Z

tsdb/engine/tsm1/engine_test.go

-				if err != nil {
-					errC <- err
-					return
+			b.Run(fmt.Sprintf("%s_%d", index, sz), func(b *testing.B) {


Nest b.Run() here as well.

benbjohnson · 2017-08-10T15:38:43Z

tsdb/shard_test.go

+	var (
+		sh  *Shard
+		itr influxql.Iterator
+	)


I personally loathe the block var declaration. :)

Why use four lines for what can be declared in two?

benbjohnson · 2017-08-10T15:40:30Z

tsdb/shard_test.go

+	}
+
+	for _, index := range tsdb.RegisteredIndexes() {
+		t.Run(index, func(t *testing.T) { test(index) })


Why is this calling an variable function instead of being nested inside Run?

I was concerned about the indentation of nesting everything inside of t.Run

This commit reduces noise in the test logs by adding a -vv flag, and silencing server log output, even when verbose testing mode is enabled. Verbose testing mode (-v) is useful for seeing where sub-tests may be failing, but it's currently too noisy with the server logs. The -vv flag can now be used to see all server output. The flag should be placed _after_ the package you're testing, e.g., go test github.com/influxdata/influxdb/tests -vv

e-dard requested review from benbjohnson and jwilder August 10, 2017 12:38

benbjohnson suggested changes Aug 10, 2017

View reviewed changes

e-dard force-pushed the er-8663-tsi1-tests branch from 64705eb to ee16983 Compare August 11, 2017 14:35

e-dard mentioned this pull request Aug 14, 2017

Allow tag filtering when using DELETE with tsi1 #8696

Merged

3 tasks

e-dard added 6 commits August 22, 2017 17:39

Convert tests to use sub-tests

aacdb9b

Ensure all tests are ran for both indexes

f64f6c5

Run most tests for both indexes

bca4393

Ensure TSI tests run with TSI index

dd808bb

Update benchmarks to use sub-benchmarks

e732cb7

benbjohnson approved these changes Aug 22, 2017

View reviewed changes

e-dard force-pushed the er-8663-tsi1-tests branch 2 times, most recently from fafe675 to 673cbcd Compare August 22, 2017 18:14

jwilder approved these changes Aug 22, 2017

View reviewed changes

e-dard force-pushed the er-8663-tsi1-tests branch from 673cbcd to 0c97d6b Compare August 22, 2017 18:38

e-dard added 5 commits August 23, 2017 10:46

Ensure shard tests run with both indexes

9c12607

Run relevant engine tests on both indexes

9be7c5a

CHANGELOG

74297a3

Ensure Skip is called in test goroutine

a5f4b92

Address feedback

d011e43

e-dard force-pushed the er-8663-tsi1-tests branch from 0c97d6b to d011e43 Compare August 23, 2017 09:47

e-dard merged commit 36ed3cb into master Aug 23, 2017

e-dard deleted the er-8663-tsi1-tests branch August 23, 2017 11:40

e-dard mentioned this pull request Aug 23, 2017

Ensure all appropriate unit tests and benchmarks run on both indexes #8663

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Run relevant unit tests on both indexes #8685

Run relevant unit tests on both indexes #8685

e-dard commented Aug 10, 2017 •

edited

Loading

jwilder commented Aug 10, 2017

e-dard commented Aug 10, 2017

benbjohnson left a comment

benbjohnson Aug 10, 2017

e-dard Aug 10, 2017

benbjohnson Aug 10, 2017

jwilder Aug 10, 2017 •

edited

Loading

benbjohnson Aug 10, 2017

benbjohnson Aug 10, 2017

e-dard Aug 10, 2017

benbjohnson Aug 10, 2017

benbjohnson Aug 10, 2017

e-dard Aug 10, 2017

e-dard Aug 11, 2017

benbjohnson Aug 11, 2017

benbjohnson Aug 10, 2017

benbjohnson Aug 10, 2017

benbjohnson Aug 10, 2017

e-dard Aug 10, 2017

Run relevant unit tests on both indexes #8685

Run relevant unit tests on both indexes #8685

Conversation

e-dard commented Aug 10, 2017 • edited Loading

Required for all non-trivial PRs

jwilder commented Aug 10, 2017

e-dard commented Aug 10, 2017

benbjohnson 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

jwilder Aug 10, 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

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

e-dard commented Aug 10, 2017 •

edited

Loading

jwilder Aug 10, 2017 •

edited

Loading