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

proposal: testing: Error Cases Helper #41494

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

proposal: testing: Error Cases Helper #41494

Kashomon opened this issue Sep 19, 2020 · 2 comments
Labels
Projects
Milestone

Comments

@Kashomon
Copy link

@Kashomon Kashomon commented Sep 19, 2020

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
@gopherbot gopherbot added the Proposal label Sep 19, 2020
@Kashomon Kashomon changed the title Proposal: Testing Error Substring Helper Proposal: testing: Error Substring Helper Sep 19, 2020
@Kashomon Kashomon changed the title Proposal: testing: Error Substring Helper proposal: testing: Error Substring Helper Sep 19, 2020
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Sep 19, 2020

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 Sep 19, 2020
@Kashomon Kashomon changed the title proposal: testing: Error Substring Helper proposal: testing: Error Cases Helper Sep 19, 2020
@Kashomon
Copy link
Author

@Kashomon Kashomon commented Sep 20, 2020

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:

@Kashomon Kashomon closed this Sep 20, 2020
@rsc rsc moved this from Incoming to Declined in Proposals Sep 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Declined
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.