fix: handle StringFromCharset against empty charset and add Unit test for rand package - #494
Conversation
…r rand.go Signed-off-by: Mrinmoy Matilal <mrinmoymatilal1315@gmail.com>
|
Welcome to the Microcks community! 💖 Thanks and congrats 🎉 for opening your first pull request here! Be sure to follow the pull request template or please update it accordingly. Hope you have a great time there! |
There was a problem hiding this comment.
Pull request overview
This PR fixes a panic in pkg/util/rand.StringFromCharset when called with an empty charset (by returning an error instead) and introduces the first unit tests for the pkg/util/rand package to cover key behaviors and edge cases.
Changes:
- Add an input guard to prevent
crypto/rand.Intfrom panicking on empty charset whenn > 0. - Add table-driven tests for
String/StringFromCharset, including the previously panicking empty-charset case.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/util/rand/rand.go | Adds empty-charset validation to prevent a panic and return a descriptive error. |
| pkg/util/rand/rand_test.go | Introduces initial unit test coverage for string generation and charset constraints. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Mrinmoy Matilal <mrinmoymatilal1315@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
pkg/util/rand/rand_test.go:108
StringFromCharsetnow has an explicit negative-length error path. Add unit tests for negativen(bothStringFromCharsetandString) so this behavior doesn’t regress back to panicking onmake([]byte, n).
func TestStringFromCharsetEmptyCharset(t *testing.T) {
t.Parallel()
got, err := StringFromCharset(5, "")
if err == nil {
|
@Vaishnav88sk @lbroudoux @yada @Harsh4902 requesting to review and suggest changes if needed |
Signed-off-by: Mrinmoy Matilal <mrinmoymatilal1315@gmail.com>
|
LGTM @Harsh4902 |
|
+1 — verified the guards cover both reported panics: the empty-charset case, and via the n<0 check the String(-1) case from #359 (String delegates to StringFromCharset). One nit: gofmt the new guard lines (if n<0{). When this merges, #359, #493 and #360 can all close against it — #360 predates this with the same idea, so a credit mention there would be a kind touch. |
|
You are now a Microcks community contributor! 💖 Thanks and congrats 🚀 on merging your first pull request! We are delighted and very proud of you! 👏 📢 If you're using Microcks in your organization, please add your company name to this list. 🙏 It really helps the project to gain momentum and credibility. It's a small contribution back to the project with a big impact. If you need to know why and how to add yourself to the list, please read the blog post "Join the Microcks Adopters list and Empower the vibrant open source Community 🙌" Kudos and please keep going, we need you 🙌 |
|
Thankyou Everyone!!...Looking forward to making more meaningful and quality contributions at MicrocksCLI... |
What
Fixes a panic in
StringFromCharset(n, charset)— it panicked withcrypto/rand: argument to Int is <= 0when called with an empty charset andn > 0, instead of returning an error the way its other failure paths already do. Also adds the first unit test coverage forpkg/util/rand, which previously had none.Why
Closes #493
Changes
pkg/util/rand/rand.go: guard against an empty charset before callingcrypto/rand.Int, returning a descriptive error instead of panicking.pkg/util/rand/rand_test.go: table-driven tests forStringandStringFromCharset, covering:Testing
go test ./pkg/util/rand/... -v— all cases pass, including the previously-panicking empty-charset case.go test ./...— full suite passes.Notes
No behavior change for any existing valid input —
String()and existing callers ofStringFromCharset()with a non-empty charset are unaffected. This only changes behavior for the previously-panicking empty-charset edge case, and adds coverage that didn't exist before.