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

Don't panic and fail test early #156

Open
urso opened this issue Apr 8, 2020 · 3 comments
Open

Don't panic and fail test early #156

urso opened this issue Apr 8, 2020 · 3 comments

Comments

@urso
Copy link

urso commented Apr 8, 2020

The tests in go-ucfg use the .../testify/assert package for validation, but sometimes also for checking on intermediate errors. The assert package only logs an error to *testing.T, but continues the test run. If there is a dependency on an operation to succeed (e.g. calling NewFrom), then the test should fail immediately pointing out the failing line in code. This can be achieved by using t.Fatal(f), use .../testify/require instead of assert, or use constructors that can panic like MustNewFrom instead of NewFrom.

By not failing and returning the test early, there might be a chance that validation/operations will panic. In this case the errors/logs stored in *testing.T will not be written to console, and the actual failure can't be inspected.

@urso urso added enhancement Team:Elastic-Agent Label for the Agent team labels Apr 8, 2020
@urso urso changed the title Don't panic, and fail test early Don't panic and fail test early Apr 8, 2020
@alrs
Copy link
Contributor

alrs commented Apr 8, 2020

@urso I've jumped in to this.

Is https://github.com/davecgh/go-spew approved for elastic projects? It makes for much more readable test errors.

@urso
Copy link
Author

urso commented Apr 9, 2020

go-spew is fine.

@urso
Copy link
Author

urso commented Oct 22, 2020

Note: testify package is basically everywhere. Not just in go-ucfg, but it is also used by many other projects. For sake of consistency we should switch to require.X instead of trying to get rid or replace testify.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants