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

Better testify usage #54

Merged
merged 5 commits into from
Mar 11, 2023
Merged

Better testify usage #54

merged 5 commits into from
Mar 11, 2023

Conversation

twz123
Copy link
Contributor

@twz123 twz123 commented Feb 8, 2023

What are you trying to accomplish?

Better UX in case of test failures:

  • Better assertion messages
  • No unneeded panics

Moreover: Make code related to test assertions a bit more concise.

What approach did you choose and why?

  • Use assert.NoError instead of assert.Nil
    This gives nicer error messages.

  • Swap arguments to assert.Equal
    The expectation comes first. Otherwise, error messages will be misleading.

  • Use assert.Error instead of assert.NotNil
    This gives nicer error messages.

  • Use assert.ErrorAs and assert.ErrorContains
    This simplifies the assertions and potentially gives better error messages.

  • Use require instead of assert and use assert.NotNil as guard
    This is to prevent panics in tests, when things get accessed which shouldn't be nil.

Anything you want to highlight for special attention from reviewers?

An example of a misleading assertion message (expected and actual are flipped):

=== RUN   TestDeleteWithIncorrectRepoForDeleteCaches
Error: authentication token not found for host github.com
	delete_test.go:56:
				Error Trace:    /build/source/cmd/delete_test.go:56
				Error:          Should be true
				Test:           TestDeleteWithIncorrectRepoForDeleteCaches
				Messages:       1 unmatched mocks: https://api.github.com/repos/testOrg/testRepo/actions/caches?key=cacheName
	delete_test.go:59:
				Error Trace:    /build/source/cmd/delete_test.go:59
				Error:          Not equal:
								expected: "authentication token not found for host github.com"
								actual  : "The given repo does not exist."

								Diff:
								--- Expected
								+++ Actual
								@@ -1 +1 @@
								-authentication token not found for host github.com
								+The given repo does not exist.
				Test:           TestDeleteWithIncorrectRepoForDeleteCaches
--- FAIL: TestDeleteWithIncorrectRepoForDeleteCaches (0.00s)

This gives nicer error messages.
The expectation comes first. Otherwise, error messages will be
misleading.

An example:

=== RUN   TestDeleteWithIncorrectRepoForDeleteCaches
Error: authentication token not found for host github.com
    delete_test.go:56:
                Error Trace:    /build/source/cmd/delete_test.go:56
                Error:          Should be true
                Test:           TestDeleteWithIncorrectRepoForDeleteCaches
                Messages:       1 unmatched mocks: https://api.github.com/repos/testOrg/testRepo/actions/caches?key=cacheName
    delete_test.go:59:
                Error Trace:    /build/source/cmd/delete_test.go:59
                Error:          Not equal:
                                expected: "authentication token not found for host github.com"
                                actual  : "The given repo does not exist."

                                Diff:
                                --- Expected
                                +++ Actual
                                @@ -1 +1 @@
                                -authentication token not found for host github.com
                                +The given repo does not exist.
                Test:           TestDeleteWithIncorrectRepoForDeleteCaches
--- FAIL: TestDeleteWithIncorrectRepoForDeleteCaches (0.00s)
This gives nicer error messages.
This simplifies the assertions and potentially gives better error
messages.
This is to prevent panics in tests, when things get accessed which
shouldn't be nil.
@twz123 twz123 requested a review from a team as a code owner February 8, 2023 12:50
@t-dedah
Copy link
Contributor

t-dedah commented Mar 11, 2023

Thank you @twz123 for the contribution. 💯

Copy link
Contributor

@t-dedah t-dedah left a comment

Choose a reason for hiding this comment

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

LGTM

@t-dedah t-dedah merged commit ed34fad into actions:main Mar 11, 2023
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