Add tests for tsdb cli tool #673

obitech · 2019-08-04T14:15:59Z

This PR introduces tests for the TSDB CLI tool, verifying the output is correct.

A requirement for this is to refactor out certain utility functions such as createBlock (-> CreateBlock) and genSeries (-> GenSeries) into a new file, mocks.go, so they can be imported by package main. Certain other types and functions that are required by those test functions are moved out of the respective test files, where they were defined previously, into mocks.go.

In order to work around annoying \t expansion in os.Stdout, all white space is stripped from expected and actual in main_test.go

Resolves #615

Test `ls`

printBlocks() now takes an io.Writer which can be tested with a bytes.Buffer.

Test `analyze`

analyzeBlock() now takse an io.Writer which can be tests with a bytes.Buffer.
The functionality of retrieving a block by ID, out of a []BlockReader has been factored out into the function extractBlock(), in order to increase testability.

TODO

Test ls
Test analyze
Delete original references of factored-out util functions & adjust imports in existing files

obitech · 2019-08-04T14:16:16Z

/cc @krasi-georgiev

obitech · 2019-08-04T14:31:37Z

I've just realized that there are now circular imports due to factoring out utility functions. Will have to deal with this first.

Edit: Fixed

krasi-georgiev

Yep very good work indeed.
Still looks a bit hacky and looks like it will brake quite often, but time will tell if it would be easy to maintain and useful to catch bugs.

krasi-georgiev · 2019-08-05T20:51:37Z

testutil/db/db.go

+)
+
+// CreateBlock creates a block with given set of series and returns its dir.
+func CreateBlock(tb testing.TB, dir string, series []tsdb.Series) string {


hm , we need to find a way to avoid duplicating all this code

I agree! This is going to be a bigger change though since just using the new function in the old test leads to import cycles (tsdb -> testutil/db -> tsdb). I'll work on testing the analyze function first, might be more functions that need to be moved out. I'll try to think of a solution after that, most likely involving some interface. Let me know if you have any suggestions!

in the past I have used this. Not sure if it will help here.

I looked at that possibility but moving the existing tests into a new package would require moving lots of stuff around which seemed very nasty. The easiest way seems to be to just move CreateBlock and GenSeries back into the tsdb package (new file: mocks.go ) but mark them clearly as test functions: MockCreateBlock and MockGenSeries. I have also moved dependent functions and types into mocks.go and deleted them where they were defined previously.

cmd/tsdb/main_test.go

krasi-georgiev · 2019-08-05T20:56:07Z

cmd/tsdb/main_test.go

+	safeDBOptions.RetentionDuration = 0
+
+	testutildb.CreateBlock(nil, tmpdir, testutildb.GenSeries(1, 1, 0, 1))
+	db, err := tsdb.Open(tmpdir, nil, nil, &safeDBOptions)


why do you need to open and close a db before you open it as read only?

Forgot to remove this after refactoring, fixed now.

cmd/tsdb/main_test.go

- Moving necessary functions into different package (only copy) - Output not checked yet Signed-off-by: obitech <[email protected]>

Signed-off-by: obitech <[email protected]>

Co-Authored-By: Krasi Georgiev <[email protected]> Signed-off-by: obitech <[email protected]>

Signed-off-by: obitech <[email protected]>

- Factor out `extractBlock()` to retrieve specific block from db.Blocks() - Write tests for said function - Simple, non-output test for `analyzeBlock()` command Signed-off-by: obitech <[email protected]>

Signed-off-by: obitech <[email protected]>

obitech · 2019-08-09T19:47:55Z

@krasi-georgiev Tests for the analyze cmd are done.

… existing test files Signed-off-by: obitech <[email protected]>

krasi-georgiev · 2019-08-11T10:35:35Z

mocks.go

+
+// MockCreateBlock creates a block with given set of series and returns its dir.
+// Intended for testing purposes.
+func MockCreateBlock(tb testing.TB, dir string, series []Series) string {


I prefer the original name. just need to find a good package name or put it one of the existing packages.

Suggested change

func MockCreateBlock(tb testing.TB, dir string, series []Series) string {

func CreateBlock(tb testing.TB, dir string, series []Series) string {

Sure, I renamed it back to CreateBlock.

krasi-georgiev · 2019-08-11T10:35:49Z

mocks.go

+
+// MockGenSeries generates series with a given number of labels and values.
+// Intended for testing purposes.
+func MockGenSeries(totalSeries, labelCount int, mint, maxt int64) []Series {


Suggested change

func MockGenSeries(totalSeries, labelCount int, mint, maxt int64) []Series {

func GenSeries(totalSeries, labelCount int, mint, maxt int64) []Series {

Signed-off-by: obitech <[email protected]>

obitech · 2019-08-11T11:32:16Z

Should I squash into a single commit?

krasi-georgiev · 2019-08-11T12:09:52Z

mocks.go

+
+// MockCreateBlock creates a block with given set of series and returns its dir.
+// Intended for testing purposes.
+func CreateBlock(tb testing.TB, dir string, series []Series) string {


No way to put all these in tsdbutil package ?

Not without massive refactoring I think. A lot of the functionality in mocks.go depends on stuff from the tsdb package: for example CreateBlock calls createHead which calls Head() from head.go. Putting all these into tsdbutil would mean we have to import tsdb into tsdbutil, which is a circular import (since tsdb imports tsdbutil in a lot of places).

To avoid this I can see three options:

Come up with some clever interfaces that we can implement in both package tsdb (+ tests), as well as in the tests of package main.

Move all existing tests into a new package, then import from both tsdb and tsdbutil.

Export CreateBlock and GenSeries from tsdb.

I think option 1 and 2 require bigger changes in regards to architecture and refactoring and would increase the size of this PR by around x5 which might be out of scope for this PR. So I thought just leaving those functions in tsdb and exporting them is the least-intrusive way forward for this. What do you think?

I am not sure,
would be interested to see what @codesome @gouthamve @bwplotka think.

Signed-off-by: obitech <[email protected]>

krasi-georgiev · 2019-08-11T13:28:34Z

Should I squash into a single commit?

no need

obitech force-pushed the tsdb_cli_tests branch from 370bc8e to 6c5bd84 Compare August 4, 2019 18:28

krasi-georgiev reviewed Aug 5, 2019

View reviewed changes

obitech force-pushed the tsdb_cli_tests branch from 8a53e2d to 92f160d Compare August 9, 2019 15:22

obitech and others added 10 commits August 9, 2019 21:45

Primitive printing of block in CLI unit test

dd0ee20

- Moving necessary functions into different package (only copy) - Output not checked yet Signed-off-by: obitech <[email protected]>

Setup Example output test

0957af6

Signed-off-by: obitech <[email protected]>

Let printBlocks() take a writer for better testability, write unit test

4cad92c

Signed-off-by: obitech <[email protected]>

Adjust syntax according to newer commits

c18ea5e

Signed-off-by: obitech <[email protected]>

Fix gofmt in main_test

58712f9

Signed-off-by: obitech <[email protected]>

Fix circular import

eebcb60

Signed-off-by: obitech <[email protected]>

Apply suggestions from code review

e5c9d53

Co-Authored-By: Krasi Georgiev <[email protected]> Signed-off-by: obitech <[email protected]>

Remove uncessary db.Open(), add full stops in comments

661fa1c

Signed-off-by: obitech <[email protected]>

Better testability of analyzeBlock command, start tests

7a61b29

- Factor out `extractBlock()` to retrieve specific block from db.Blocks() - Write tests for said function - Simple, non-output test for `analyzeBlock()` command Signed-off-by: obitech <[email protected]>

Add tests for analyzeBlock CLI command

67c92d8

Signed-off-by: obitech <[email protected]>

obitech force-pushed the tsdb_cli_tests branch from eba0f8a to 67c92d8 Compare August 9, 2019 19:45

Move testing functions back into tsdb package, remove duplicates from…

4dd7e7f

… existing test files Signed-off-by: obitech <[email protected]>

krasi-georgiev reviewed Aug 11, 2019

View reviewed changes

Rename Mock{CreateBlock,GenSeries} to CreateBlock, GenSeries

5dad441

Signed-off-by: obitech <[email protected]>

obitech changed the title ~~WIP: Add tests for tsdb cli tool~~ Add tests for tsdb cli tool Aug 11, 2019

krasi-georgiev reviewed Aug 11, 2019

View reviewed changes

Fix comments for CreateBlock and GenSeries

c6f75f5

Signed-off-by: obitech <[email protected]>

obitech mentioned this pull request Aug 20, 2019

Add tests for tsdb CLI commands prometheus/prometheus#5927

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tests for tsdb cli tool #673

Add tests for tsdb cli tool #673

obitech commented Aug 4, 2019 •

edited

Loading

obitech commented Aug 4, 2019

obitech commented Aug 4, 2019 •

edited

Loading

krasi-georgiev left a comment

krasi-georgiev Aug 5, 2019

obitech Aug 9, 2019

krasi-georgiev Aug 10, 2019

obitech Aug 11, 2019

krasi-georgiev Aug 5, 2019

obitech Aug 9, 2019

obitech commented Aug 9, 2019

krasi-georgiev Aug 11, 2019

obitech Aug 11, 2019

krasi-georgiev Aug 11, 2019

obitech Aug 11, 2019

obitech commented Aug 11, 2019

krasi-georgiev Aug 11, 2019 •

edited

Loading

obitech Aug 11, 2019 •

edited

Loading

krasi-georgiev Aug 11, 2019

krasi-georgiev commented Aug 11, 2019

	func MockCreateBlock(tb testing.TB, dir string, series []Series) string {
	func CreateBlock(tb testing.TB, dir string, series []Series) string {

	func MockGenSeries(totalSeries, labelCount int, mint, maxt int64) []Series {
	func GenSeries(totalSeries, labelCount int, mint, maxt int64) []Series {

Add tests for tsdb cli tool #673

Are you sure you want to change the base?

Add tests for tsdb cli tool #673

Conversation

obitech commented Aug 4, 2019 • edited Loading

Test ls

Test analyze

TODO

obitech commented Aug 4, 2019

obitech commented Aug 4, 2019 • edited Loading

krasi-georgiev 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

obitech commented Aug 9, 2019

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

obitech commented Aug 11, 2019

krasi-georgiev Aug 11, 2019 • edited Loading

Choose a reason for hiding this comment

obitech Aug 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krasi-georgiev commented Aug 11, 2019

obitech commented Aug 4, 2019 •

edited

Loading

Test `ls`

Test `analyze`

obitech commented Aug 4, 2019 •

edited

Loading

krasi-georgiev Aug 11, 2019 •

edited

Loading

obitech Aug 11, 2019 •

edited

Loading