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

Add ErrorAssertionFunc creators #1556

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

JERHAV
Copy link

@JERHAV JERHAV commented Mar 1, 2024

Summary

To clean up table driven test with often repeated functions we can add a simple function creator that generates a ErrorAssertionFunc.

// ErrorIsFor returns an [ErrorAssertionFunc] which tests if the error wraps target.
func ErrorIsFor(target error) ErrorAssertionFunc

// ErrorAsFor returns an [ErrorAssertionFunc] which tests if the any error in err's tree matches target and if so, assigns it to target.
// The returned function panics if target is not a non-nil pointer to either a type that implements error, or to any interface type.
func ErrorAsFor(target interface{}) ErrorAssertionFunc

// ErrorTypeFor returns an [ErrorAssertionFunc] which tests whether the type of the error matches the targets type.
func ErrorTypeFor(target error) ErrorAssertionFunc

Changes

  • Add creators for errorAssertionFunctions for ErrorIs and ErrorAs to be able to use these functions simmilar to NoError and Error in test tables

Motivation

errorAssertionFunc: require.CreateErrorIsAssertionFunc(testedpkg.ExpectedError)

@brackendawson
Copy link
Collaborator

brackendawson commented Mar 4, 2024

This is quite a terse issue, but I actually think I know what you mean because I've done it. I'd also like some high-order functions to create assertion functions for ErrorIs and ErrorAs which match the signature of Error and NoError.

Example:

package kata_test

import (
	"testing"

	"github.com/brackendawson/kata"
	"github.com/stretchr/testify/assert"
)

func TestIfy(t *testing.T) {
	for name, test := range map[string]struct {
		expectErr func(assert.TestingT, error, ...interface{}) bool
	}{
		"should work": {
			expectErr: assert.NoError,
		},
		"should return any error": {
			expectErr: assert.Error,
		},
		"should return a specific error": {
			expectErr: errorIs(kata.SmellyError),
		},
	} {
		t.Run(name, func(t *testing.T) {
			err := kata.Funk()
			test.expectErr(t, err)
		})
	}
}

// errorIs could be part of testify, same for errorAs
func errorIs(target error) func(assert.TestingT, error, ...interface{}) bool {
	return func(t assert.TestingT, err error, msgAndArgs ...interface{}) bool {
		return assert.ErrorIs(t, err, target, msgAndArgs...)
	}
}

PS: the function returned by errorIs needs to call t.Helper()

Copy link
Collaborator

@brackendawson brackendawson left a comment

Choose a reason for hiding this comment

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

I think this needs an example, somewhere, perhaps:

func TestIfy(t *testing.T) {
	for name, test := range map[string]struct {
		expectErr assert.ErrorAssertionFunc
	}{
		"returns no error": {
			expectErr: assert.NoError,
		},
		"returns error wrapping io.EOF": {
			expectErr: assert.ErrorIsFor(io.EOF),
		},
		"returns error wrapping fs.PathError": {
			expectErr: assert.ErrorAsFor(&fs.PathError{}),
		},
	} {
		t.Run(name, func(t *testing.T) {
			err := Do()
			test.expectErr(t, err)
		})
	}
}

In practice, for the ErrorAs case I've also wanted to make assertions against the error's fields, can ErrorAsFor have a function argument for this?

func TestIfy(t *testing.T) {
	for name, test := range map[string]struct {
		expectErr assert.ErrorAssertionFunc
	}{
		"returns error wrapping fs.PathError": {
			expectErr: assert.ErrorAsFor(func(t assert.TestingT, err fs.PathError) { // testify can infer the type from the function signature 
				assert.Equal(t, "filename.txt", err.Path)
			}),
		},
	} {
		t.Run(name, func(t *testing.T) {
			err := Do()
			test.expectErr(t, err)
		})
	}
}

assert/assertions.go Outdated Show resolved Hide resolved
@@ -48,6 +48,20 @@ type ErrorAssertionFunc func(TestingT, error, ...interface{}) bool
// Comparison is a custom function that returns true on success and false on failure
type Comparison func() (success bool)

// CreateErrorIsAssertionFunc returns an [ErrorAssertionFunc] which tests if the error is a specific value.
func CreateErrorIsAssertionFunc(expectedError error) ErrorAssertionFunc {
return func(t TestingT, err error, i ...interface{}) bool {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This function needs to be a helper, ie it should call t.Helper()

Also:

Suggested change
return func(t TestingT, err error, i ...interface{}) bool {
return func(t TestingT, err error, msgAndArgs ...interface{}) bool {

Copy link
Author

Choose a reason for hiding this comment

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

It can't call t.Helper, there is no helper in this interface.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Add this:

	if h, ok := t.(tHelper); ok {
		h.Helper()
	}

assert/assertions.go Outdated Show resolved Hide resolved
assert/assertions.go Outdated Show resolved Hide resolved
@@ -48,6 +48,20 @@ type ErrorAssertionFunc func(TestingT, error, ...interface{}) bool
// Comparison is a custom function that returns true on success and false on failure
type Comparison func() (success bool)

// CreateErrorIsAssertionFunc returns an [ErrorAssertionFunc] which tests if the error is a specific value.
func CreateErrorIsAssertionFunc(expectedError error) ErrorAssertionFunc {
return func(t TestingT, err error, i ...interface{}) bool {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add this:

	if h, ok := t.(tHelper); ok {
		h.Helper()
	}

require/requirements.go Outdated Show resolved Hide resolved
require/requirements.go Outdated Show resolved Hide resolved
@JERHAV JERHAV force-pushed the errorAssertionFunctionsCreators branch from e4ac99d to 9262a71 Compare March 4, 2024 17:05
// ErrorAsFunc returns an [ErrorAssertionFunc] which tests if the any error in err's tree matches target and if so, assigns it to target.
// The returned function panics if target is not a non-nil pointer to either a type that implements error, or to any interface type.
func ErrorAsFor(expectedInterface interface{}) ErrorAssertionFunc {
return func(t TestingT, err error, i ...interface{}) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Rename i to msgAndArgs.

@@ -26,4 +26,27 @@ type BoolAssertionFunc func(TestingT, bool, ...interface{})
// for table driven tests.
type ErrorAssertionFunc func(TestingT, error, ...interface{})

// ErrorIsFunc returns an [ErrorAssertionFunc] which tests if the error wraps target.
func ErrorIsFor(expectedError error) ErrorAssertionFunc {
return func(t TestingT, err error, i ...interface{}) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Rename i to msgAndArgs.

@dolmen
Copy link
Collaborator

dolmen commented Mar 4, 2024

I'm not convinced by the need for ErrorAsFor. I need to see a real use case.

@dolmen dolmen changed the title Create errorAssertionFunction creators Add ErrorAssertionFunc creators Mar 4, 2024
@dolmen dolmen added enhancement pkg-assert Change related to package testify/assert pkg-require Change related to package testify/require labels Mar 4, 2024
@JERHAV
Copy link
Author

JERHAV commented Mar 4, 2024

I agree with the ErrorAsFor being less useful, but sometimes you have a function that returns something like:

type ErrNotOKStatusCode struct{
    StatusCode int
}

in which case if I then want to use this error in a higher up function I'd test if it is the correct interface.

@JERHAV JERHAV force-pushed the errorAssertionFunctionsCreators branch 2 times, most recently from fd08514 to 519f696 Compare March 4, 2024 18:25
@brackendawson
Copy link
Collaborator

brackendawson commented Mar 4, 2024

I think ErrorAsFor can be useful, but it needs a different signature so that you can make assertions against the type you asserted:

// ErrorAsFor returns an [ErrorAssertionFunc] which tests if any error in err's tree matches target in the given fn
// with signature func(t TestingT, target <error_type>) and if so, calls fn with target set to that error.
func ErrorAsFor(fn interface{}) ErrorAssertionFunc
func TestIfy(t *testing.T) {
	for name, test := range map[string]struct {
		expectErr assert.ErrorAssertionFunc
	}{
		"returns error wrapping fs.PathError": {
			expectErr: assert.ErrorAsFor(func(t assert.TestingT, err fs.PathError) {
				assert.Equal(t, "filename.txt", err.Path)
			}),
		},
	} {
		t.Run(name, func(t *testing.T) {
			err := Do()
			test.expectErr(t, err)
		})
	}
}

See mock.MatchedBy for how to get the type of an argument to a function. This would need reflection.

This also gets rid of the common panic because the user can't call it without using a pointer, which causes people to sometimes raise issues about ErrorAs.

@JERHAV
Copy link
Author

JERHAV commented Mar 4, 2024

I do not exactly know what you mean, I have added a simple type assertion function in the fixup commit. But I'm not sure that is what you mean.

assert/assertions.go Outdated Show resolved Hide resolved
require/requirements.go Outdated Show resolved Hide resolved
require/requirements_test.go Show resolved Hide resolved
require/requirements_test.go Outdated Show resolved Hide resolved
@dolmen
Copy link
Collaborator

dolmen commented Mar 5, 2024

@brackendawson Here is a rewrite of your example of ErrorAsFor that I find much more readable (it doesn't require the reader to know the special function ErrorAsFor that this PR adds):

func TestIfy(t *testing.T) {
	for name, test := range map[string]struct {
		expectErr require.ErrorAssertionFunc
	}{
		"returns error wrapping fs.PathError": {
			expectErr: func(t require.TestingT, err error, _ ...interface{}) {
				var errPath *fs.PathError
				require.ErrorAs(t, err, &errPath)
				require.Equal(t, "filename.txt", errPath.Path)
			},
		},
	} {
		t.Run(name, func(t *testing.T) {
			err := Do()
			test.expectErr(t, err)
		})
	}
}

On the Go Playground.

@JERHAV JERHAV force-pushed the errorAssertionFunctionsCreators branch 2 times, most recently from 3f0a0fc to b44a5f3 Compare March 5, 2024 21:17
@brackendawson
Copy link
Collaborator

Yes, that's a better approach. Let's use this PR to add ErrorIsFor, because this function increases readability of people's tests.

We can do ErrorAsFor later if there is a good API design. The implementation here does not encourage good testing practice because it can only be used to check that an error in err's chain is assignable to target. In good practice a developer testing for state should assert that the fields or behaviour of that error are also as intended.

To clean up table driven test with often repeated functions we can add a
simple function creator that generates a errorAssertionFunction
@JERHAV JERHAV force-pushed the errorAssertionFunctionsCreators branch from b44a5f3 to 1b432fb Compare March 7, 2024 17:51
Copy link
Collaborator

@brackendawson brackendawson left a comment

Choose a reason for hiding this comment

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

LGTM

@JERHAV JERHAV requested a review from dolmen April 12, 2024 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement pkg-assert Change related to package testify/assert pkg-require Change related to package testify/require
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants