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

Jump to bottom

proposal: testing: Error Cases Helper #41494

Closed
artasparks opened this issue Sep 19, 2020 · 2 comments
Closed

proposal: testing: Error Cases Helper #41494

artasparks opened this issue Sep 19, 2020 · 2 comments
Labels
FrozenDueToAge Proposal
Projects
Proposals (old)
Milestone
Proposal

Comments

@artasparks
Copy link

I would like to propose adding a helper method to the standard library (in the testing pkg, probably) for testing whether an error contains a substring in the context of unit-testing. It's pattern I use frequently, and tend to copy around between libraries.

When testing whether or not an error is expected based on an expected substring, there are four cases to check. {err, no err} X {expErr, no expErr}. This is a helper to check all four cases.

Generally, I use such a helper in table tests:

...
		{
			desc: "Point out of range",
			in: &Point{x: 100, y: 200},
			expErrSubstr: "out of range x or y",
		},
...
	for _, tc := range testCases {
		t.Run(tc.desc, func(t *testing.T) {
			out, err := New(tc.in.x, tc.in.y).Convert()
			cerr := errcheck.CheckCases(err, tc.expErrSubstr)
			if cerr != nil {
				t.Error(cerr)
				return
			}
			if err != nil {
				// Error is non-nil, but expected. Can't do any more assertions. 
				return
			}
			// More assertions here.

Here's the sort of method method I end up writing:

func CheckCases(err error, expErrSubstr string) error {
	if err == nil && expErrSubstr != "" {
		return fmt.Errorf("got no error but expected error containing %q", expErrSubstr)
	} else if err != nil && expErrSubstr == "" {
		return fmt.Errorf("got error %q but expected no error", err.Error())
	} else if err != nil && !strings.Contains(err.Error(), expErrSubstr) {
		return fmt.Errorf("got error %q but expected it to contain %q", err.Error(), expErrSubstr)
	}
	return nil
}
@gopherbot gopherbot added this to the Proposal milestone Sep 19, 2020
@artasparks artasparks changed the title Proposal: Testing Error Substring Helper Proposal: testing: Error Substring Helper Sep 19, 2020
@artasparks artasparks changed the title Proposal: testing: Error Substring Helper proposal: testing: Error Substring Helper Sep 19, 2020
@ianlancetaylor
Copy link
Contributor

In cases where the user of a package is expected to be able to test what kind of error they received, we would normally recommend supporting errors.Is or errors.As. Then your test would use those mechanisms.

In cases where the user is just expected to test err != nil, it's often best for the test to work that way as well. Testing for a specific error by examining the error message is not always wrong, but it's not typically best practice. Often a test should be checking what the user code will check, so it suffices to check whether an error is returned or not. Checking for a particular message string can turn the test into a change detector rather than a useful test (see https://testing.googleblog.com/2015/01/testing-on-toilet-change-detector-tests.html).

So it's not immediately obvious to me that this is a feature we would want to add the testing package. Of course you can always write it yourself, as you are doing today. I'm only commenting on whether it should go into the standard library.

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) Sep 19, 2020
@artasparks artasparks changed the title proposal: testing: Error Substring Helper proposal: testing: Error Cases Helper Sep 19, 2020
@artasparks
Copy link
Author

artasparks commented Sep 20, 2020 •
edited

I think I didn't frame this quite right: My proposal was less around Error-substring testing and more around error-case checking in the context of table-tests. I actually rather like the idea of using Errors.Is instead of strings.Contains

That being said, errors.Is works as I want it to (https://play.golang.org/p/MeEnmeAl8CP):

	err := fmt.Errorf("%w addendum", SomeError)
	fmt.Printf("IsErr: Case(nil, !nil): %t\n", errors.Is(nil, SomeError))	
	fmt.Printf("IsErr: Case(!nil, nil): %t\n", errors.Is(err, nil))
	fmt.Printf("IsErr: Case(nil, nil): %t\n", errors.Is(nil, nil))
	fmt.Printf("IsErr: Case(!nil, !nil match): %t\n", errors.Is(err, SomeError))
IsErr: Case(nil, !nil): false
IsErr: Case(!nil, nil): false
IsErr: Case(nil, nil): true
IsErr: Case(!nil, !nil match): true

So I'm satisfied with using errors.Is since it has the same behavior as the helper I was proposing:

@rsc rsc moved this from Incoming to Declined in Proposals (old) Sep 23, 2020
@golang golang locked and limited conversation to collaborators Sep 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge Proposal
Projects
No open projects
Proposals (old)
Declined
Milestone
Proposal
Development

No branches or pull requests
3 participants
@artasparks @ianlancetaylor @gopherbot