Skip to content

vendor: add github.com/stretchr/testify test dependency#15457

Merged
karalabe merged 1 commit into
ethereum:masterfrom
robert-zaremba:testify
Nov 13, 2017
Merged

vendor: add github.com/stretchr/testify test dependency#15457
karalabe merged 1 commit into
ethereum:masterfrom
robert-zaremba:testify

Conversation

@robert-zaremba
Copy link
Copy Markdown
Contributor

github.com/stretchr/testify is a useful library for doing
assertion in tests. It makes assertions in test more less verbose and
more comfortable to read and use.

Testify plays nice with default Go testing package and includes three
convenient packages: assert, require, and mock.

This PR contains only the addition to of testify package to dependency
in order to use it in future tests.

github.com/stretchr/testify is a useful library for doing
assertion in tests. It makes assertions in test more less verbose and
more comfortable to read and use.
@lmars
Copy link
Copy Markdown
Contributor

lmars commented Nov 12, 2017

It seems odd to me to vendor a package which isn't actually being used. Is there a specific test which would benefit from this? Perhaps an example showing the benefits of this package would be useful.

@robert-zaremba
Copy link
Copy Markdown
Contributor Author

@lmars , Initially I've added this package as a part of another PR: #15452 . However they asked me to introduce it separately. So here I am to propose inclusion of it. Then I will update my tests and add few more.

The advantage of testify is that your tests are more compact - you don't have that many if.., it provides default messages for different checks (Equal, Nil, True, WithinDuration, InEpsilon, InSet ...) and you don't have to repeat coding this logic. Which is just easier and more convenient. Please look at the documentation or some examples.

@karalabe
Copy link
Copy Markdown
Member

I concur with @lmars on this one. There's no point to pull in unused code into the repository. If a PR needs it, then that PR needs to pull in the necessary dependencies.

That being said, I'd like a discussion to happen first on bringing in a new testing framework before there's even code doing it. A new test framework means that we either need to rewrite all of our tests, or all of a sudden we'll have two different test styles.

I'm willing to accept such a new framework as long as there's consensus with @Arachnid and @fjl (and myself). Why is this a good thing?

Copy link
Copy Markdown
Member

@karalabe karalabe left a comment

Choose a reason for hiding this comment

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

Pending inclusion decision.

@robert-zaremba
Copy link
Copy Markdown
Contributor Author

To clarify: testify is not a framework. It's a library which handles comparisons / checks and messages pretty printing. It won't change the workflow. It will only make the checks / comparisons more friendly. Simple example (writing it from hand without checking with compiler):

if assert.NotNil(t, err) {
    unsubscribe()
}
assert.WithinDuration(t, createdOn, time.Now(), 2*time.Second)

vs

if err != nil {
    t.Error("Should be able to convert the number", err)
    unsubscribe()
}
var elapsed = createdOn.Sub(time.Now())
var maxElapsed = 2*time.Second
if elapsed < -maxElapsed || elapsed > maxElapsed {
    t.Error("createdOn should be within 2 seconds from now, but is %v", elapsed)
}

@Arachnid
Copy link
Copy Markdown
Contributor

I would love to see more concise tests.

Copy link
Copy Markdown
Member

@karalabe karalabe left a comment

Choose a reason for hiding this comment

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

Shorter it is!

@karalabe karalabe added this to the 1.7.3 milestone Nov 13, 2017
@karalabe karalabe merged commit 9b97f98 into ethereum:master Nov 13, 2017
@robert-zaremba robert-zaremba deleted the testify branch November 13, 2017 12:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants