-
Notifications
You must be signed in to change notification settings - Fork 899
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
chore(utils): test sanitizing empty addr #2998
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is cool, lets go a step further, and create a named var in the address.go file (ErrInvalidIP
maybe) then instead of the expErr being a bool, we can make it an error and give it nil or the expected error and test require.EqualErorr
or similar as part of the test, then we can skip the !tt.expErr and just test that its nil or the ErrInvalidIP
require.Equal(t, tt.want, got) | ||
if tt.err != nil { | ||
require.Error(t, err) | ||
require.True(t, errors.Is(err, tt.err)) | ||
} else { | ||
require.NoError(t, err) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wanted this
require.Equal(t, tt.want, got)
require.ErrorIs(t, tt.err, err)
But couldn't get it to work
Error: Target error should be in err chain:
expected: "invalid IP address or hostname given: "
in chain: "invalid IP address or hostname given"
Test: TestSanitizeAddr/#00
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2998 +/- ##
==========================================
- Coverage 50.96% 50.76% -0.21%
==========================================
Files 168 168
Lines 11038 11022 -16
==========================================
- Hits 5626 5595 -31
- Misses 4908 4928 +20
+ Partials 504 499 -5 ☔ View full report in Codecov by Sentry. |
if tt.err != nil { | ||
require.Error(t, err) | ||
require.True(t, errors.Is(err, tt.err)) | ||
} else { | ||
require.NoError(t, err) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can simplify this by just doing assert.ErrorIs(t, err, tt.err)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<!-- Thank you for submitting a pull request! Please make sure you have reviewed our contributors guide before submitting your first PR. Please ensure you've addressed or included references to any related issues. Tips: - Use keywords like "closes" or "fixes" followed by an issue number to automatically close related issues when the PR is merged (e.g., "closes #123" or "fixes #123"). - Describe the changes made in the PR. - Ensure the PR has one of the required tags (kind:fix, kind:misc, kind:break!, kind:refactor, kind:feat, kind:deps, kind:docs, kind:ci, kind:chore, kind:testing) --> in kind replacement of #2998 with the simplified require assertion --------- Co-authored-by: Håvard Anda Estensen <haavard.ae@gmail.com>
<!-- Thank you for submitting a pull request! Please make sure you have reviewed our contributors guide before submitting your first PR. Please ensure you've addressed or included references to any related issues. Tips: - Use keywords like "closes" or "fixes" followed by an issue number to automatically close related issues when the PR is merged (e.g., "closes #123" or "fixes #123"). - Describe the changes made in the PR. - Ensure the PR has one of the required tags (kind:fix, kind:misc, kind:break!, kind:refactor, kind:feat, kind:deps, kind:docs, kind:ci, kind:chore, kind:testing) --> in kind replacement of #2998 with the simplified require assertion --------- Co-authored-by: Håvard Anda Estensen <haavard.ae@gmail.com>
<!-- Thank you for submitting a pull request! Please make sure you have reviewed our contributors guide before submitting your first PR. Please ensure you've addressed or included references to any related issues. Tips: - Use keywords like "closes" or "fixes" followed by an issue number to automatically close related issues when the PR is merged (e.g., "closes #123" or "fixes #123"). - Describe the changes made in the PR. - Ensure the PR has one of the required tags (kind:fix, kind:misc, kind:break!, kind:refactor, kind:feat, kind:deps, kind:docs, kind:ci, kind:chore, kind:testing) --> in kind replacement of celestiaorg#2998 with the simplified require assertion --------- Co-authored-by: Håvard Anda Estensen <haavard.ae@gmail.com>
<!-- Thank you for submitting a pull request! Please make sure you have reviewed our contributors guide before submitting your first PR. Please ensure you've addressed or included references to any related issues. Tips: - Use keywords like "closes" or "fixes" followed by an issue number to automatically close related issues when the PR is merged (e.g., "closes #123" or "fixes #123"). - Describe the changes made in the PR. - Ensure the PR has one of the required tags (kind:fix, kind:misc, kind:break!, kind:refactor, kind:feat, kind:deps, kind:docs, kind:ci, kind:chore, kind:testing) --> in kind replacement of celestiaorg#2998 with the simplified require assertion --------- Co-authored-by: Håvard Anda Estensen <haavard.ae@gmail.com>
Add coverage to the non-happy path