tests: move tests to testutil

[gouthamve]

tsdb is using the require library of testify and its much less verbose and cleaner. Should we also migrate the tests everywhere?

/cc @brian-brazil @grobie @fabxc

[brian-brazil]

I'm strongly in favour.

[jlevesy]

Hey, I'm interested in working on this issue :)

[gouthamve]

Hey @jlevesy, good to know. This issue luckily requires little context but is also a little big.

Currently we have tests using just the standard lib which makes checking for failures/conditions verbose, for example: https://github.com/prometheus/prometheus/blob/master/storage/local/series_test.go#L38-L41 and https://github.com/prometheus/prometheus/blob/master/storage/local/series_test.go#L42-L44

But when using stretchr/testify/require, we can simply do require.NoError(t, s.dropChunks(10)) and require.Equal(t, 0, s.chunkDescs). All the tests here: https://github.com/prometheus/tsdb/ are based on testify (example).

Now we want to move all the tests from stdlib to testify in this repo, but as we have too many tests, I will be breaking it into each package in the repo:

• cmd
• config
• discovery
• notifier
• pkg
• promql
• relabel
• retrieval
• rules
• storage
• templace
• util
• web

(Just something I came up with the directory structure, please feel free to open more or less PRs per folder depending on the size 😄 )

Also, as we are soon going to merge the dev-2.0 branch into master, we prefer if the PRs are opened against the 2.0 branch, so make sure you work on the 2.0 branch.

If you have any more questions, please feel free to comment here or on IRC.

[jlevesy]

Thanks for the quick and clear answer.

I'll try to open a PR asap once I'm done with the web package, just to make sure I'm in the right direction :).

[brancz]

@jlevesy there is a discussion around this on IRC at the moment, I would suggest to hold off for a moment 🙂 . Feel free to join in Freenode #prometheus-dev .

[gouthamve]

@jlevesy Sorry about that, there was a discussion right after I posted here and we essentially just use a small subset of testify's features and donot need to import all of it. We decided to copy these three functions here: https://github.com/benbjohnson/testing into https://github.com/prometheus/prometheus/tree/dev-2.0/util/testutil and use them everywhere.

Essentially not much changes from the above description except require.NoError() --> testutil.OK(), require.Equal() --> testutil.Equals() and require.True/False() --> testutil.Assert(). Let me know if you have any questions.

[jlevesy]

Okay, sounds to me.

On Thu, 5 Oct 2017 at 13:26 Frederic Branczyk ***@***.***> wrote: @jlevesy <https://github.com/jlevesy> there is a discussion around this on IRC at the moment, I would suggest to hold off for a moment 🙂 . Feel free to join in Freenode #prometheus-dev . — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3242 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAbqkaPWsPuhyo8MKlDLoMTnaxn199uyks5spLzVgaJpZM4PulAv> .

-- Julien Levesy

[alcortesm]

Is there room for another helping hand?

I would like to help with the tests of the config package.

[cored]

@jlevesy @gouthamve do you need more hands on this? I would like to help out.

[gouthamve]

@alcortesm @cored Sure! But I think its best to wait for #3245 to get merged in and once that is done, each of you could pick a package and send in a PR :)

[jlevesy]

Is that issue really closed ?

[grobie]

Thanks @jlevesy for catching this. It got auto-closed.

[jlevesy]

I'm currently working on the discovery package, however I noticed stretchr/testify is already beeing used in some files, for instance here.

Am I supposed to get rid of strectchr/testify and replace it with testutil ? Maybe introducing some helpers (Nil and NotNil for instance) which seems to be useful !

[grobie]

Yes, we should only use one test library.

Personally I'd rather not have too many functions in the test package interface, but curious what @brancz and @fabxc think here.

[brancz]

Happy to extend our testutil package to our needs (to a reasonable degree), and also happy to get rid of the strectchr/testify dependency all together 🙂 .

[jlevesy]

Just opened #3292 so you can see what I have in mind :)

[smonv]

Hi, I'm interested to working on this issue too. Can I join and PR for other packages ?

[smonv]

Hi, I created #3695 for notifier package.

[elifkus]

This is an updated list of packages that need to be fixed, prepared by @gouthamve (above).

• cmd
• config
• discovery (?)
• notifier
• pkg
• promql
• relabel
• rules
• scrape (renamed from 'retrieval')
• storage
• templace
• util
• web

[smonv]

I'm trying to rewrite test for storage package but got error:

prometheus use_testutil_in_storage_package ~> go test github.com/prometheus/prometheus/storage
# github.com/prometheus/prometheus/storage
import cycle not allowed in test
package github.com/prometheus/prometheus/storage (test)
        imports github.com/prometheus/prometheus/util/testutil
        imports github.com/prometheus/prometheus/storage

FAIL    github.com/prometheus/prometheus/storage [setup failed]

In testutil/storage.go it import storage package so all tests in storage cannot import testutil again.

How about move testutil/storage.go to other location/package ? (like util/storageutil/testing.go and package name will be storagetestutil) I think storagetestutil won't be a best name for package but I've no other ideas.

[elifkus]

The same "import cycle not allowed in test" error happens in pkg/labels/matcher_test.go . Similar to the error @smonv received.

# github.com/prometheus/prometheus/pkg/labels
import cycle not allowed in test
package github.com/prometheus/prometheus/pkg/labels (test)
	imports github.com/prometheus/prometheus/util/testutil
	imports github.com/prometheus/prometheus/storage
	imports github.com/prometheus/prometheus/pkg/labels

FAIL	github.com/prometheus/prometheus/pkg/labels [setup failed]

[janickic]

I have started working on the discovery package.

[gouthamve]

So there are still some places which need to be update, namely:

• scrape/scrape_test.go
• storage/fanout_test.go
• storage/buffer_test.go
• storage/remote/codec_test.go
• promql/parse_test.go
• pkg/textparse/parse_test.go

Almost there :)