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

Use Assert package in the test files #186

Closed
rexposadas opened this issue Mar 20, 2023 · 7 comments
Closed

Use Assert package in the test files #186

rexposadas opened this issue Mar 20, 2023 · 7 comments

Comments

@rexposadas
Copy link
Contributor

Rationale

I suggest using the assert package in the tests. See: https://github.com/stretchr/testify

It'll make the code cleaner. For example in api_test.go we have:

	if err == nil {
		t.Fatal("ListEngines did not fail")
	}

That can be changed to:

assert.NoErrorf(t, err, "ListEngines did not fail")

Implementation

I've implemented this feature for production level code and it makes the test code look a lot cleaner.

If you agree with the suggestion, I am willing to make the PR for it.

@sashabaranov
Copy link
Owner

@rexposadas Thank you for the suggestion! Our library aims to have no dependencies for as long as possible. While the testify library has some helpful features, they are not enough to make it a dependency. Having said that — we can add the single-line error checking in tests to the library. Your contribution would be greatly appreciated, along with any code coverage improvements 🙌🏻

@rexposadas
Copy link
Contributor Author

I see. No problem.

we can add the single-line error checking in tests to the library

Which library are you referring to? Would it be this one? https://github.com/sashabaranov/go-fastapi

@sashabaranov
Copy link
Owner

sashabaranov commented Mar 20, 2023

@rexposadas I mean to this library, just as a function e.g.

func checkNoError(t *testing.T, err error, message string) {
	if err != nil {
		t.Errorf(message)
	}
}

@rexposadas
Copy link
Contributor Author

@sashabaranov But, to keep the code consistent would mean making other functions such as:

checkHasError (..), checkErrorIs(..)

You're OK with that?

@sashabaranov
Copy link
Owner

@rexposadas sounds good! It might also make sense to put those into internal/test package, but I trust your judgment here.

@rexposadas
Copy link
Contributor Author

Great. I'll work on the PR.

@rexposadas
Copy link
Contributor Author

PR for this has been merged.

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

No branches or pull requests

2 participants