Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: introduce stretchr/testify package for better test development experience #28

Merged
merged 2 commits into from
Apr 23, 2023
Merged

test: introduce stretchr/testify package for better test development experience #28

merged 2 commits into from
Apr 23, 2023

Conversation

nekomeowww
Copy link
Contributor

Description

Improvements of #18

  • Replaced all test assert statement with github.com/stretchr/testify/assert and github.com/stretchr/testify/require packages for better DX and semantic

@tmc
Copy link
Owner

tmc commented Apr 20, 2023

I'm a bit skeptical here -> I've always found the stdlib testing capabilities just fine. Is this really worth the dependency add? not many projects I work with use this library.

@nekomeowww
Copy link
Contributor Author

nekomeowww commented Apr 20, 2023

I'm a bit skeptical here -> I've always found the stdlib testing capabilities just fine. Is this really worth the dependency add? not many projects I work with use this library.

Answers:

  • Yes the capabilities is fine and suitable. We can use it.
  • Since we will only need the stretchr/testify dependencies within the *_test.go files, there will be no affect on building artifacts, even though we implemented some utilities or mock helpers with stretchr/testify imported to help unit tests in the future, we can still state the build constraints in source code to prevent the tree-shaking not being worked properly.
  • I use stretchr/testify package all around my projects and also with my companies on work. It is stable and easy to use.
  • Many of the big Golang repositories use stretchr/testify to speed up the writing of unit tests:

Benefits from stretchr/testify:

  • stretchr/testify is easy to use and understand with semantic statement.
    • When using stretchr/testify/assert, assert means assert the values, objects, identifiers with functions predefined.
    • When using stretchr/testify/require, require means assert with t.FailNow() feature included, it's like to say, "hey, if the assert is not success, then you fail, the rest of the test requires it".
    • When you read or review the code, unlike the current situation you must read with the possible nested if...else... statement, you simply understand where the assertion seeks or requires the statements to be Equal or NotEqual, Nil or NotNil, NoError or ErrorIs just by reading the function names confidently.
  • stretchr/testify has a lot of underlying logicals to help assertion and predefined functions for assertion
    • It is not just comparing the values, it compares the types underneath it. Such as comparing int8(10) with int64(10) will produce assertion failure for developers to understand what is happened. Such as comparing the enums.
  • stretchr/testify provides detailed and nice output to tell you why and where the tests failed with the function names, file names and line numbers printed out for developers to understand.
    • Writing the assertion failure, error, fatal, log message for assertion result often copiable, and time wasting. When we developing and evolving quickly, we may have to refactor our code many times where the tests log messages would have to manually checked by developers and reviewers. With stretchr/testify, the messages will be generated for different functions, if you call assert.Equal(...), once the assertion is failed, it would print out the expected and the actual values pretty without efforts.
  • stretchr/testify provides more than just assert and log failures proprietary, it provides mock and suite for more structured testing for bigger projects.

Why:

  • I often find people write the tests incorrectly, such as the t.Fatal(...) or t.FailNow() would not be called at the situation where the testing should not continue, just like the situation you fixed in the previous PR. With stretchr/testify, require sub packages provide such abilities to help developers to get assertion correctly the way just like they use assert while keeping the t.FailNow() executed internally with no further coding efforts.
  • if ... else ... statement is tricky in unit tests. IMO, I think the assertion for if ... else ... is required to prevent some of the desired branch not tested, so when you need if ... else ... statements to not only help to assert, but also help on logical branching, it is easy to confuse developers and reduce the efficiency where it might lead to bugs and issues.
  • Just like what I said above, stretchr/testify can help developers and maintainers identify the failures easily. For the most important part, I don't think it's the developer's responsibility to manually maintain the unit test message and helper functions, nor the reviewer's responsibility to always keep a eye on how these functions and messages were being written.

Copy link
Owner

@tmc tmc left a comment

Choose a reason for hiding this comment

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

LGTM

@tmc tmc merged commit a24f965 into tmc:main Apr 23, 2023
@nekomeowww nekomeowww deleted the dev/tests branch April 24, 2023 06:08
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.

None yet

2 participants