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

Inappropriate uses of assert.NoError? #3125

Open
knrc opened this issue Nov 15, 2024 · 2 comments
Open

Inappropriate uses of assert.NoError? #3125

knrc opened this issue Nov 15, 2024 · 2 comments

Comments

@knrc
Copy link

knrc commented Nov 15, 2024

While completing a simple PR as part of the KubeCon contribfest I hit an issue where one test was causing a panic.

I tracked this down to the use of assert.NoError(t, err) for checking errors, but instead of being used as a guard for the subsequent test code it continued as if the error did not occur and dereferenced a nil. It looks as if this may occur often within the project tests.

@sayboras
Copy link
Member

Just wanna share the experience in cilium/cilium. I think the value of assert.NoError is to gather all the outputs/failures at once, so that developer can fix in one shot. However, it requires a careful thinking to make sure there is no side effect or misleading or even panic, hence in cilium/cilium most of assert.* is changed to require.* recently in main branch.

@kkourt
Copy link
Contributor

kkourt commented Dec 3, 2024

Yap, agreed: we should change those into require.NoError.

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

3 participants